fix macos portability issues (fifth batch) #6479

garlick · 2024-12-05T19:50:10Z

Last batch of macos fixes.

I think this gets everything compiled, including tests but there are still unit test failures to deal with. Anyway, this is as far as I got for now.

chu11 · 2024-12-05T20:16:12Z

src/common/libutil/test/fdwalk.c

 static int * get_open_fds (int maxfd)
 {
    /* Valgrind may show open fds > maxfd, so double the space
     *  allocated so we don't overflow when run under valgrind.
     */
    int * fds = calloc (maxfd * 2, sizeof (int));
    if (fds)
-        ok (fdwalk (set_fd, fds) == 0,
+        ok (fdwalk (set_fd_if_open, fds) == 0,


should we also do this in test_fdwalk_fallback()?

I'm not seeing much motivation for doing that, are you?

just wanted to make sure it wasn't missed by accident :-)

chu11 · 2024-12-05T20:22:04Z

src/common/libmissing/pipe2.c

+    int valid_flags = O_CLOEXEC | O_NONBLOCK;
+
+    if ((flags & ~valid_flags)) {
+        errno = EINVAL;
        return -1;
+    }
+    if ((flags & O_CLOEXEC)) {
+        if (fd_set_cloexec (fd) < 0)
+            return -1;
+    }
+    if ((flags & O_NONBLOCK)) {
+        if (fd_set_nonblocking (fd) < 0)
+            return -1;
+    }


what you did here doesn't match your commit message. Not sure if you changed mind on what to do and just need to update commit message? hopefully not a rebase mistake?

Those are just helper functions that do what it says in the commit message but I guess that could be made more clear.

Just pushed a tweak to the commit message

Ahh, you know what it is. Seeing the very generic prior implementation, the appearance is that more/other flags would have been supported.

But I now see that pipe2() only supports three flags, one of which appears to be linux only. so the new setflags() supports every flag for pipe2()

chu11

LGTM!

grondo

This LGTM. If compilation is working then we should consider adding a macos build to CI to avoid future PRs breaking the build.

grondo · 2024-12-05T21:00:21Z

src/shell/oom.c

@@ -20,7 +20,9 @@
 #endif
 #include <sys/types.h>
 #include <sys/stat.h>
+#ifdef HAVE_SYS_INOTIFY_H


It seems like this plugin should just be conditionally not compiled on macos, since we know cgroups are never supported there? However, perhaps this way is just easier since you don't have to modify the bulitin plugin list?

Well that seems obvious now that you mention it. Let me try that way.

Problem: the shell oom plugin won't compile on macos because it doesn't have inotify. Make compilation conditional on inotify.

Problem: there is an extra "to" in an error message. Fix it.

Problem: macos doesn't have /proc/self/exe so flux(1) cannot determine if it is installed or not. Provide an executable_selfdir() implementation for macos that uses _NSGetExecutablePath(). See also: dyld(3).

Problem: the fdwalk unit test fails on macos The failing test assumes that fdwalk visits only the open file descriptors, which is the case for linux but not when the fallback implementation is used. Modify the test to check the file descriptors before tallying.

Problem: the intree unit test uses pthread barriers, an optional POSIX feature that is missing on macos. Provide a simple alternate implementation within the test if barriers are unavailable.

Problem: the pipe2(2) libmissing implementation sets O_CLOEXEC incorrectly. Use helpers that call fcntl (F_SETFD, FD_CLOEXEC) instead of calling fcntl (F_SETFL, O_CLOEXEC), which is wrong. Adjust linkage to ensure that helper gets linked in where needed.

Problem: compilation fails on macos in various places becuase SOCK_NONBLOCK is undefined. Set O_NONBLOCK with fcntl(2) after socket creation if SOCK_NONBLOCK is undefined.

Problem: compilation fails in various places on macos due to undefined kill(2). Include signal.h where needed.

Problem: compilation fails on macos with 'environ' undefined. Just define it locally where needed.

Problem: compilation fails in some places where get_current_dir_name() is used. Use the fallback in libmissing where needed.

Problem: compilation of test program fails due to missing jansson headers. Add $(JANSSON_CFLAGS) to AM_CPPFLAGS.

Problem: compilation fails on macos when %ju or %jd is used with non-matching integer types. Cast them to [u]intmax_t.

garlick · 2024-12-05T23:13:58Z

OK, I reworked the oom plugin it's just not built if inotify is unavailable.

Perhaps we can add a macos builder another day? It would have to just do a make since make check-prep would require unit tests to pass, which they do not currently.

codecov · 2024-12-05T23:24:30Z

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.60%. Comparing base (2e7b064) to head (a941e1f).
Report is 13 commits behind head on master.

Files with missing lines	Patch %	Lines
src/cmd/flux.c	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6479      +/-   ##
==========================================
- Coverage   83.60%   83.60%   -0.01%     
==========================================
  Files         524      524              
  Lines       87617    87621       +4     
==========================================
+ Hits        73256    73257       +1     
- Misses      14361    14364       +3

Files with missing lines	Coverage Δ
src/common/libutil/intree.c	`93.61% <100.00%> (+0.59%)`	⬆️
src/shell/builtins.c	`92.30% <ø> (ø)`
src/cmd/flux.c	`85.32% <0.00%> (ø)`

... and 13 files with indirect coverage changes

garlick · 2024-12-05T23:41:49Z

Chicken mon dieu! This macos nightmare is finally over for now.

grondo · 2024-12-06T00:30:21Z

Perhaps we can add a macos builder another day? It would have to just do a make since make check-prep would require unit tests to pass, which they do not currently.

Ok. If/when we do that we'll probably have to fix portability issues we introduce between now and then. Since we have no macos users and it doesn't work anyway, that's pretty low priority IMO.

garlick · 2024-12-06T00:46:39Z

I'll open an issue on that one for now.

chu11 reviewed Dec 5, 2024

View reviewed changes

garlick force-pushed the macos5 branch from 1705a25 to ee40547 Compare December 5, 2024 20:37

chu11 approved these changes Dec 5, 2024

View reviewed changes

grondo approved these changes Dec 5, 2024

View reviewed changes

garlick added 12 commits December 5, 2024 14:55

shell/oom: skip building when inotify is unavailable

ff292bf

Problem: the shell oom plugin won't compile on macos because it doesn't have inotify. Make compilation conditional on inotify.

flux: fix error message

8a9d1ad

Problem: there is an extra "to" in an error message. Fix it.

libutil/intree: port to macos

169cc01

Problem: macos doesn't have /proc/self/exe so flux(1) cannot determine if it is installed or not. Provide an executable_selfdir() implementation for macos that uses _NSGetExecutablePath(). See also: dyld(3).

testsuite: avoid non-portable pthreads dependency

961c539

Problem: the intree unit test uses pthread barriers, an optional POSIX feature that is missing on macos. Provide a simple alternate implementation within the test if barriers are unavailable.

work around missing SOCK_NONBLOCK

756c1ed

Problem: compilation fails on macos in various places becuase SOCK_NONBLOCK is undefined. Set O_NONBLOCK with fcntl(2) after socket creation if SOCK_NONBLOCK is undefined.

include signal.h for kill(2)

0872748

Problem: compilation fails in various places on macos due to undefined kill(2). Include signal.h where needed.

define environ as an extern

044fd2f

Problem: compilation fails on macos with 'environ' undefined. Just define it locally where needed.

use libmissing get_current_dir_name()

d653f20

Problem: compilation fails in some places where get_current_dir_name() is used. Use the fallback in libmissing where needed.

build: add missing JANSSON_CFLAGS

e7d7bb3

Problem: compilation of test program fails due to missing jansson headers. Add $(JANSSON_CFLAGS) to AM_CPPFLAGS.

cast %j arguments to [u]intmax_t

a941e1f

Problem: compilation fails on macos when %ju or %jd is used with non-matching integer types. Cast them to [u]intmax_t.

garlick force-pushed the macos5 branch from ee40547 to a941e1f Compare December 5, 2024 22:58

garlick added the merge-when-passing label Dec 5, 2024

mergify bot merged commit 02cae96 into flux-framework:master Dec 5, 2024
34 checks passed

garlick deleted the macos5 branch December 6, 2024 00:05

garlick mentioned this pull request Dec 6, 2024

ci: add a macos builder #6484

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix macos portability issues (fifth batch) #6479

fix macos portability issues (fifth batch) #6479

garlick commented Dec 5, 2024

chu11 Dec 5, 2024

garlick Dec 5, 2024

chu11 Dec 5, 2024

chu11 Dec 5, 2024 •

edited

Loading

garlick Dec 5, 2024

garlick Dec 5, 2024

chu11 Dec 5, 2024

chu11 left a comment

grondo left a comment

grondo Dec 5, 2024

garlick Dec 5, 2024

garlick commented Dec 5, 2024

codecov bot commented Dec 5, 2024

garlick commented Dec 5, 2024

grondo commented Dec 6, 2024

garlick commented Dec 6, 2024

fix macos portability issues (fifth batch) #6479

fix macos portability issues (fifth batch) #6479

Conversation

garlick commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chu11 Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chu11 left a comment

Choose a reason for hiding this comment

grondo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garlick commented Dec 5, 2024

codecov bot commented Dec 5, 2024

Codecov Report

garlick commented Dec 5, 2024

grondo commented Dec 6, 2024

garlick commented Dec 6, 2024

chu11 Dec 5, 2024 •

edited

Loading