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

Mimic GNU basename() API for non-glibc library e.g. musl #15561

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

kraj
Copy link
Contributor

@kraj kraj commented Mar 15, 2024

musl only provides POSIX version of basename and it has also removed providing it via string.h header [1] which now results in compile errors with newer compilers e.g. clang-18

[1] https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7

@ton31337
Copy link
Member

Even more, we have compile errors:

15-Mar-2024 21:40:56 	In file included from zebra/zapi_msg.c:12:
15-Mar-2024 21:40:56 	./lib/zebra.h:230:36: error: expected declaration specifiers or '...' before '/'
15-Mar-2024 21:40:56 	  230 | #define basename(src) (strrchr(src,'/') ? strrchr(src,'/')+1 : src)
15-Mar-2024 21:40:56 	      |                                    ^~~
15-Mar-2024 21:40:56 	./lib/zebra.h:230:41: error: expected ')' before '?' token
15-Mar-2024 21:40:56 	  230 | #define basename(src) (strrchr(src,'/') ? strrchr(src,'/')+1 : src)
15-Mar-2024 21:40:56 	      |                                         ^
15-Mar-2024 21:40:56 	  CC       zebra/zebra_affinitymap.o
15-Mar-2024 21:40:56 	In file included from /usr/include/stdint.h:32,
15-Mar-2024 21:40:56 	                 from /usr/local/lib/gcc12/gcc/x86_64-portbld-freebsd14.0/12.2.0/include/stdint.h:9,
15-Mar-2024 21:40:56 	                 from ./lib/compiler.h:366,
15-Mar-2024 21:40:56 	                 from ./lib/zebra.h:13:
15-Mar-2024 21:40:56 	zebra/zapi_msg.c: In function 'zserv_encode_vrf':
15-Mar-2024 21:40:56 	zebra/zapi_msg.c:105:44: error: 'basename' undeclared (first use in this function)
15-Mar-2024 21:40:56 	  105 |                 strlcpy(data.l.netns_name, basename((char *)netns_name),
15-Mar-2024 21:40:56 	      |                                            ^~~~~~~~
15-Mar-2024 21:40:56 	zebra/zapi_msg.c:105:44: note: each undeclared identifier is reported only once for each function it appears in

@@ -41,6 +41,10 @@
#define ZEBRA_NS_POLLING_INTERVAL_MSEC 1000
#define ZEBRA_NS_POLLING_MAX_RETRIES 200

#if !defined(__GLIBC__)
#define basename(src) (strrchr(src,'/') ? strrchr(src,'/')+1 : src)
Copy link
Member

Choose a reason for hiding this comment

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

Just loudly thinking, if we need to add an additional check here:

#ifndef basename
    #define basename(src) (strrchr(src,'/') ? strrchr(src,'/')+1 : src)
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what would that get us ? it is in a .c source file unless its included multiple times, it wont be useful.

@mwinter-osr
Copy link
Member

CI:rerun Rerun after fixing git access on CI infra

@Jafaral
Copy link
Member

Jafaral commented Aug 22, 2024

@kraj can you please rebase on top of master? will try to get his in.

@kraj
Copy link
Contributor Author

kraj commented Aug 22, 2024

@kraj can you please rebase on top of master? will try to get his in.

done

@Jafaral
Copy link
Member

Jafaral commented Aug 22, 2024

minor nit by the linter

  #if !defined(__GLIBC__)
-#define basename(src) (strrchr(src,'/') ? strrchr(src,'/')+1 : src)
+#define basename(src) (strrchr(src, '/') ? strrchr(src, '/') + 1 : src)
 #endif
 

musl only provides POSIX version of basename and it has also removed
providing it via string.h header [1] which now results in compile errors
with newer compilers e.g. clang-18

[1] https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7

Signed-off-by: Khem Raj <[email protected]>
@Jafaral
Copy link
Member

Jafaral commented Aug 23, 2024

Thank you @kraj. Ping me if you need any help to get FRR on OE side.

@Jafaral Jafaral merged commit 83a60e7 into FRRouting:master Aug 23, 2024
11 checks passed
@Jafaral
Copy link
Member

Jafaral commented Aug 23, 2024

Oh, BTW, would love to see 10.1 on scarthgap. I can backport this to 10.1 if that helps.

@kraj
Copy link
Contributor Author

kraj commented Aug 23, 2024

Oh, BTW, would love to see 10.1 on scarthgap. I can backport this to 10.1 if that helps.

We are on 10.0.1 atm, isnt 10.1 a major release compared to 10.0 ? if not then its easier to get it in if major release its against the policy sadly but we can make a case if there are critical fixes and features in 10.1

@Jafaral
Copy link
Member

Jafaral commented Aug 23, 2024

10.1 is a minor release. 10.0 was a major release mainly due to the push for support NB interface. 10.1 improves on that.

@kraj
Copy link
Contributor Author

kraj commented Aug 23, 2024

10.1 is a minor release. 10.0 was a major release mainly due to the push for support NB interface. 10.1 improves on that.

thanks please push this change to 10.1 as well and we can try to get scarthgap updated to use it

@Jafaral
Copy link
Member

Jafaral commented Aug 23, 2024

@Mergifyio backport stable/10.1

Copy link

mergify bot commented Aug 23, 2024

backport stable/10.1

✅ Backports have been created

donaldsharp added a commit that referenced this pull request Aug 23, 2024
Mimic GNU basename() API for non-glibc library e.g. musl (backport #15561)
@Jafaral
Copy link
Member

Jafaral commented Aug 23, 2024

@kraj backport merged to stable/10.1

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.

4 participants