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

fix macos portability issues (third batch) #6473

Merged
merged 9 commits into from
Dec 4, 2024

Conversation

garlick
Copy link
Member

@garlick garlick commented Dec 4, 2024

This fixes some more macos portability issues.

Problem: get_current_dir_name() is a glibc extension that is not
available on macos.

Add a simple implementation to libmissing.
Update users.
Problem: prctl(2) is missing on macos

Skip setting the broker proc title if PR_SET_NAME is undefined.
Problem: the flux broker fails to compile on macos because
the wrong number of arguments are passed to pthread_setname_np().

Add an autoconf check for that.
Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@@ -18,6 +18,7 @@
#include "src/common/libmissing/argz.h"
#endif
#include <unistd.h>
#include <libgen.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message typo: undefiend

Comment on lines +22 to +26
char *basename_simple (const char *path)
{
char *p = strrchr (path, '/');
return p ? p + 1 : (char *)path;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Does this undo the need for 92c2ece if all uses of basename were updated to basename_simple()?

I know these are just portability updates, but perhaps we can update the code in shell/doom.c that does the same thing to use basename_simple.c instead:

flux-core/src/shell/doom.c

Lines 100 to 106 in 11ad605

static const char *get_jobspec_command_arg0 (struct shell_doom *doom)
{
json_t *s = json_array_get (doom->shell->info->jobspec->command, 0);
const char *path = json_string_value (s);
const char *p = strrchr (path, '/');
return p ? p + 1 : path;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sure, we can do that. I was trying to have a light touch but this makes sense.

Problem: macos fails to compile some printfs of timeval.tv_usec
because it's an int on macos.

Cast where appropriate.
Problem: the shell fails to compile on macos becuase GLOB_TILDE_CHECK
is a glob(3) extension unavailable on macos.

Just skip the flag for now if it is undefined.
Problem: ptrace code does not compile on macos because the request
names are different.

If the linux request names are undefined, try the BSD ones.
Problem: rlimit code doesn't compile on macos

Skip querying resource names that are not defined.
Problem: log_init() already calls basename(3) on its argument
but some users do another one.

Drop extra basename.
Problem: compilation fails on macos when we pass a const string
value to basename(3).

Add basename_simple() which does what GNU basename does and takes
a const argument.

Convert basename() users.
Convert a couple of local functions that were doing the same thing.
@garlick
Copy link
Member Author

garlick commented Dec 4, 2024

Thanks! Everything is using basename_simple() including the local function you pointed out and one more I found. I'll set MWP.

@mergify mergify bot merged commit 01f3097 into flux-framework:master Dec 4, 2024
34 checks passed
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

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

Project coverage is 83.63%. Comparing base (11ad605) to head (27abc8c).
Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
src/broker/module.c 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6473      +/-   ##
==========================================
- Coverage   83.64%   83.63%   -0.01%     
==========================================
  Files         523      524       +1     
  Lines       87600    87595       -5     
==========================================
- Hits        73271    73259      -12     
- Misses      14329    14336       +7     
Files with missing lines Coverage Δ
src/broker/broker.c 77.35% <100.00%> (-0.03%) ⬇️
src/broker/log.c 74.64% <100.00%> (ø)
src/cmd/builtin/dmesg.c 95.00% <100.00%> (+0.04%) ⬆️
src/cmd/flux-exec.c 81.69% <100.00%> (ø)
src/cmd/job/mpir.c 85.93% <100.00%> (ø)
src/common/libsdexec/unit.c 93.96% <100.00%> (-0.11%) ⬇️
src/common/libutil/basename.c 100.00% <100.00%> (ø)
src/common/libutil/dirwalk.c 90.64% <100.00%> (ø)
src/common/libutil/log.c 76.47% <100.00%> (ø)
src/modules/cron/cron.c 81.79% <ø> (ø)
... and 11 more

... and 11 files with indirect coverage changes

@garlick garlick deleted the macos3 branch December 4, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants