Macos fixes #6926
Macos fixes #6926
Conversation
There was a problem hiding this comment.
LGTM, although perhaps we can improve this commit message.
kill: define SIGRTMAX in terms of NSIG
problem: MacOS provides NSIG but not SIGRTMAX, which made the check fail
because our test uses NSIG and our code assumes 64 if SIGRTMAX is not
set
perhaps saying this fixes something in job-manager/kill and perhaps mention that a kill test in job-manager uses NSIG. The "our test uses ... " is a bit ambigous without context.
problem: MacOS/BSD lack a linux-compatible proc (usually) so we can't read routes from proc solution: On linux we'll continue to use /proc/net/route, while on macos it turns out you can have a default interface *that does not have a netmask of zero*. To get it reliably, I've added a version of `getprimary_interface4` that uses the CoreFoundation and SystemConfiguration frameworks to request the global IPv4 default interface name from macos directly. This is more macos specific than using a PF_ROUTE socket, but much _much_ simpler. If we want this to work on other BSDs (aside from FreeBSD with linux compat turned on) we'll want to do that but NETLINK/PF_ROUTE are really arcane and rather high overhead for something this simple. Anyway, this is using the classic C interface to CoreFoundation rather than any of the add-ons so there's no need to compile code as Objective-C or anything. As to why I switched to this from the getifaddrs approach, that seemed to work fine on linux with a small tweak, but not on macos, and if it doesn't work reliably cross-platform it seems better to keep what we had battle tested and add the more specific mac version like this.
problem: MacOS provides NSIG which is used in one of the kill unit tests but not SIGRTMAX, which made the check fail because our test uses NSIG and our code assumes 64 if SIGRTMAX is not set solution: if SIGRTMAX is not set, but NSIG is available, then define SIGRTMAX=(NSIG-1)
problem: mkostemp is only defined to accept O_APPEND, O_SYNC, and O_CLOEXEC. Setting other flags is allowed to cause failures, and does on MacOS solution: remove the unnecessary flag
|
Good point @chu11, it's reworded and rebased to match the PR it's stacked on. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6926 +/- ##
==========================================
- Coverage 83.91% 83.90% -0.01%
==========================================
Files 540 540
Lines 90526 90526
==========================================
- Hits 75967 75960 -7
- Misses 14559 14566 +7
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I feel like the ipaddr stuff needs a closer review to see whether the use of stderr is appropriate and whether the configure/ifdef logic is as simple as it can be, but I'm traveling today and probably won't have time for a bit. If this can wait I'll try to take a look this weekend.
| @@ -38,7 +38,11 @@ | |||
| #include "job-manager.h" | |||
|
|
|||
| #ifndef SIGRTMAX | |||
There was a problem hiding this comment.
Or just use NSIG.
That's what I did before in d51397b
There was a problem hiding this comment.
That's fair, I haven't found a system that lacks it yet. I'll make that change.
This is what I get for not completely rewriting the ChatGPT code for that function. I rewrote the CString handling to be pure C but forgot the error prints, will definitely fix.
I was going for something like what the linux kernel does, make it a simple selection of one complete thing or another complete thing, though I didn't do it in a header like they require. Would be happy to rework that if you prefer something else, mainly wanted to avoid needing to have OpenSTEP libs for CoreFoundation on linux (much as I actually really like their string class). |
|
Oh and it can definitely wait, there's a heisenbug in fileref of all things on GHA right now that's going to take a bit to work out. Thanks for looking it over! |
This is a couple more macos fixes on top of #6925. These are much smaller, dealing with missing defines or temp file creation. After these and the other one though, everything in src seems to pass except libsubprocess.