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

musl_missing.h: re-add GNU basename macro for musl #271

Merged
merged 1 commit into from Feb 4, 2024

Conversation

oreo639
Copy link
Contributor

@oreo639 oreo639 commented Jan 27, 2024

This re-adds the GNU basename macro when compiling on musl.

Alternatively #include <libgen.h> can be added to musl_missing.h, this would continue the behavior as it currently is before the following musl change, although code will need to expect POSIX basename instead of GNU basename.

elgoind was able to build without either change on musl due to it containing a declaration in string.h (despite it only providing POSIX basename), this has recently been removed:
https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7

Let me know which way makes the most sense.

musl only provides POSIX basename and not GNU basename.
musl also recently removed their compatibility declaration from string.h:
https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7

reverts 8ca381d
@Yamakuzure
Copy link
Collaborator

Looks good to me.
It would be better to check for basename() in configuration stage and then make the define depending on the outcome. But I will add that check after merge, no problem.
Thank you very much!

@Yamakuzure Yamakuzure merged commit b4f93c4 into elogind:main Feb 4, 2024
1 check passed
@oreo639 oreo639 deleted the basename branch February 4, 2024 10:09
@Yamakuzure
Copy link
Collaborator

Please have a look at d8dc8f6 - are you okay with that?

@oreo639
Copy link
Contributor Author

oreo639 commented Feb 4, 2024

Just for sake of clarification, the difference between GNU basename() and POSIX basename() is that GNU never modifies the input argument, where POSIX does modify the input argument (e.g. if the argument has a trailing slash, POSIX says it should be removed from the input string before it is returned, where GNU says an empty string should be returned instead). musl never supported GNU basename.

The compatibility declaration was added to avoid issues where applications compile successfully, but then crash afterwards due to pointer truncation to int (which gcc supported for compatibility). musl removed the extra basename() decl because it gets interpreted differently in C23 and will be irrelevant with gcc 14 (which makes implicit function declarations and implicit int an error in C99+ without -fpermissive, similarly motivated by C23. glibc also ran in to a similar issue when removing libcrypt which forced them to keep a declaration of crypt() in unistd.h to avoid pointer truncation to int).

I tried using #define basename(path) and it appears to work fine on an unpatched musl.

Keep in mind #define basename(path) does break including libgen.h though, which elogind currently doesn't do.

The following code appears to expect GNU basename():
https://github.com/elogind/elogind/blob/main/src/basic/musl_missing.c#L59
(it calls basename() directly on a const input argument, which follows GNU basename() convention, but not POSIX basename() convention, which granted, argv[0] will not normally have a slash at the end unless directly called using exec*(). This could probably use last_path_component() instead.)

Aside from that, most other instances I see rn though are either tests or relatively harmless:
https://github.com/search?q=repo%3Aelogind%2Felogind%20basename&type=code

As for whether or not the configure check makes sense, I'm not sure, probably fine. Hopefully this helps though.

algitbot pushed a commit to alpinelinux/aports that referenced this pull request Feb 19, 2024
bell-sw pushed a commit to bell-sw/alpaquita-aports that referenced this pull request Feb 22, 2024
[ commit 5e310743fa30bbac68bb91fb733c9b50e5362373 ]

elogind/elogind#271

needed since 5b03d0f6107b5fec65df59dde0de8abf8d956c4d
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

2 participants