Skip to content
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

Open
wants to merge 12 commits into
base: v1.x
Choose a base branch
from

Conversation

laijonathan
Copy link
Contributor

@laijonathan laijonathan commented Dec 6, 2023

Enables libuv to build on z/OS:

Resolves test failures:

@laijonathan laijonathan marked this pull request as ready for review December 6, 2023 16:17
@bnoordhuis
Copy link
Member

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.

@laijonathan laijonathan force-pushed the zos-clang-upstream branch 3 times, most recently from d90e138 to 05634aa Compare December 11, 2023 18:47
@laijonathan
Copy link
Contributor Author

@bnoordhuis Fixed

@bnoordhuis
Copy link
Member

@laijonathan can you rebase and fix the merge conflict? Cheers.

@laijonathan
Copy link
Contributor Author

@bnoordhuis Fixed

@bnoordhuis
Copy link
Member

@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.

laijonathan and others added 12 commits January 6, 2024 13:05
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 <Shuowang.Zhang@ibm.com>
zos does not implement ifaddrs.  Fixes build failure
@laijonathan
Copy link
Contributor Author

@bnoordhuis Rebased

@laijonathan
Copy link
Contributor Author

laijonathan commented Jan 6, 2024

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?

Copy link
Member

@bnoordhuis bnoordhuis left a 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.

Comment on lines +1295 to +1298
#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
Copy link
Member

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,
Copy link
Member

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.

Comment on lines +282 to +285
/* 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]);
Copy link
Member

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?

Comment on lines +3214 to +3217
#if defined(__CYGWIN__) || \
defined(__MVS__) || \
(defined(__HAIKU__) && B_HAIKU_VERSION < B_HAIKU_VERSION_1_PRE_BETA_5) || \
(defined(__sun) && !defined(__illumos__))
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants