Add z/OS compatibility improvements#656
Conversation
There was a problem hiding this comment.
Why is this necessary? The existing code calls icharset("utf-8",1) unless MSDOS_COMPILER is not zero and not equal to WIN32C. How does the z/OS build set MSDOS_COMPILER?
There was a problem hiding this comment.
We wanted to default to UTF-8 on z/OS since the locale support is incomplete and setting LANG to the UTF8 locales breaks other tools.
The logic is as follows: HAVE_LOCALE is set to 1, so we enter the condition in line 453
453 #if HAVE_LOCALE
454 /*
455 * Get character definitions from locale functions,
456 * rather than from predefined charset entry.
457 */
458 ilocale();
459 #ifdef __MVS__
460 /*
461 * z/OS Locale does not properly support UTF-8. Default to "utf-8".
462 */
463 (void) icharset("utf-8", 1);
464 #endif
465 #else
466 #if MSDOS_COMPILER
467 #if MSDOS_COMPILER==WIN32C
468 (void) icharset("utf-8", 1);
469 #else
470 (void) icharset("dos", 1);
471 #endif
472 #else
473 (void) icharset("utf-8", 1);
474 #endif
475 #endif
There was a problem hiding this comment.
Could the z/OS build just set HAVE_POLL to 0, to avoid the need for an extra condition here? Does the z/OS build use configure?
There was a problem hiding this comment.
Do you recommend that we just add this to the CFLAGS? -DHAVE_POLL=0?
|
It would be useful to know how exactly you're building "less", as it can shed some light on macros/flags which might or might not be auto-detected, etc. |
Our build scripts are a little complex. We use a tool called |
|
Which defines.h do you use? Do you use one of the checked-in versions, or do you create your own by running configure or by some other process? |
I fail to see where it actually does the build. I see it sets export ZOPEN_CONFIGURE=skip
export ZOPEN_MAKE=skipwhich supposedly skips configure and make? if it does use make, which makefile is used? where is the actual invocation of the compiler? where's the list of files to compile and link? |
We run configure via the buildenv configuration - https://github.com/zopencommunity/lessport/blob/main/buildenv (zopen-build will run configure by default unless it's skipped)
You can ignore those - I'm just pointing out the default cflags that we pass via the command line. A sample compile invocation looks as like this: |
|
So it first runs |
Yep, that is correct |
|
Can you post the output of On linux it prints I'll let @gwsw continue, I gotta go for now. |
Yes, poll was detected: However, the poll implementation on z/OS has issues when paging up or down or when searching for text (likely some z/OS bug). That's why I disabled it in the code. |
|
Ok, we do have some special cases in the code to disable poll on a couple of systems (MSDOS & Windows, and some versions of MacOS). I think it's ok to add another one for z/OS but let me think about it a little and see if there might be a better way to handle this. Maybe an option to configure would be better. The change for SIGPIPE looks ok. For the utf-8 issue, could you set LESSCHARSET=utf-8 in your system environment? That should override the locale setting and wouldn't require a change to the code. |
|
Before the final comit is merged, it's worth explaining at the commit message why it does things the way it does, for instance, it should include something like "./configure on z/OS does detect that poll is supported, however, it's buggy on z/OS version XXX and earlier, so this commit disabled it explicitly", and similarly the reaons for any other changes which are not obvious. Maybe some comments at the code too where something is not obvious. Otherwise in the future it would not be possible to tell why HAVE_POLL is bypassed on z/OS, etc, and someone might remove this code because there's no apparent reason to bypass it. Or maybe the poll bug is fixed in z/OS at some stage, so the exception can be removed at the code (maybe), but it will never happen because no one could tell why it's there in the first place, so it becomes zombie code which no one understands why it's there. |
z/OS is reported to have some bug(s) in their poll() implementation, so disable it. Related to #656.
|
@IgorTodorovskiIBM I have implemented two of your three changes. Please let me know if it's possible for you to set LESSCHARSET=utf-8 in your environment to solve the third problem as I suggested above, or if there's some reason that that wouldn't work for you. |
Hi @gwsw yes that works. Thank you! |
This PR adds compatibility improvements to make the
lessutility work properly on z/OS systems. The changes address three specific issues that prevent proper operation on this platform.Changes
UTF-8 charset support: Added code to default to UTF-8 charset on z/OS systems since the z/OS locale implementation doesn't properly support UTF-8
SIGPIPE handling: Modified signal handling to conditionally check for SIGPIPE only when the signal is defined, as some systems (z/OS) may not define this signal
poll() functionality: Excluded z/OS systems from using poll which causes issues when going page up or down.
If you'd like to test these changes or improve upon them for z/OS, please let me know and I can get you access to a z/OS system.