-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
zos: fix build errors and test failures #4244
base: v1.x
Are you sure you want to change the base?
Conversation
https://github.com/libuv/libuv/blob/v1.x/CONTRIBUTING.md#COMMIT and please squash into logical commits (maybe they already are, I can't tell.) Some of the commit logs contain links to IBM's internal github instance. Not useful, please remove. |
d90e138
to
05634aa
Compare
@bnoordhuis Fixed |
@laijonathan can you rebase and fix the merge conflict? Cheers. |
@bnoordhuis Fixed |
@laijonathan can you rebase on top of v1.x instead of merge? Those Windows CI failures almost certainly mean you're missing some relevant changes. Another data point: automatic rebase doesn't work because of merge conflicts, so yeah. |
By setting CMAKE_C_STANDARD to 90, the -std=gnu90 flag gets used when building with the clang compiler on z/OS, which explicitly disables features that are not C90 such as stdbool.h. From the libuv maintainers, libuv follows C90 syntax, but it is actually a C11 project. Refs: libuv#4134
fixes test-get-passwd testcase
On z/OS, it is possible for one process to be in ASCII and another process to be in EBCDIC. In order for the two processes to communicate, it is necessary to use automatic code set conversion, which can be enabled by using the CCSID tag on the pipes. However, the same does not hold true for socket pairs, which has no support for auto conversion. So we create a z/OS specific process_init_stdio function that creates pipes instead of socket pairs for stdio.
There is an edge case where if one of the stdio file descriptor gets closed, then it is possible for a pipe to be reassigned 0, 1, or 2 as its fd when calling uv__process_init_stdio(). When this fd is then later closed uv__process_open_stream(), the assertion checking that fd > 2 in uv__close() will fail despite the fd being valid. So use uv__close_nocheckstdio() in uv__process_open_stream() instead of uv__close().
Co-authored-by: Wayne Zhang <[email protected]>
zos does not implement ifaddrs. Fixes build failure
24f9a70
to
70e14ab
Compare
@bnoordhuis Rebased |
The Visual Studio 16 CI is still failing for some reason. I suspect it has something to do with setting the C standard to C11 as Visual Studio 17 is passing. @bnoordhuis Any thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Windows build failure is almost certainly because of the C90 -> C11 change, yes. I guess you'll just have to revert that.
#if !defined(__MVS__) | ||
UV_EXTERN int uv_thread_getpriority(uv_thread_t tid, int* priority); | ||
UV_EXTERN int uv_thread_setpriority(uv_thread_t tid, int priority); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The symbols should always be available. You can make them return UV_ENOSYS to indicate it's not supported on zos. Update the thread_priority test to either call RETURN_SKIP or check for UV_ENOSYS.
@@ -224,6 +224,52 @@ static int uv__process_init_stdio(uv_stdio_container_t* container, int fds[2]) { | |||
} | |||
} | |||
|
|||
#ifdef __MVS__ | |||
/* TODO: Revisit once V2R5 Framework + CLIB Override lands in ZOSLIB. */ | |||
static int os390_process_init_stdio(uv_stdio_container_t* container, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want too much platform-specific code in this file so please move it to src/unix/os390.c and prepend uv__
to the function name.
/* Use nocheckstdio because it is possible for a process to close its stdio | ||
* fds, resulting in the pipefds to be reassigned to a stdio fd. | ||
*/ | ||
err = uv__close_nocheckstdio(pipefds[1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you actually hitting the assert? When/how?
#if defined(__CYGWIN__) || \ | ||
defined(__MVS__) || \ | ||
(defined(__HAIKU__) && B_HAIKU_VERSION < B_HAIKU_VERSION_1_PRE_BETA_5) || \ | ||
(defined(__sun) && !defined(__illumos__)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to guess you blindly copied these conditionals without putting much thought into whether they apply to other platforms... sloppy, please fix that and pay closer attention going forward.
I care 0% about zos so if IBM wants non-cross-platform behavior, that's fine by me, but it'd be best to fix up uv_fs_read() rather than the test suite.
Enables libuv to build on z/OS:
ifaddr.h
Resolves test failures: