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

Add support support for musl libc on linux #1632

Closed
wants to merge 4 commits into from
Closed

Conversation

BenJarg
Copy link
Contributor

@BenJarg BenJarg commented Mar 2, 2022

These changes are to make it possible to compile xrootd on musl libc, which is a standards conforming C standard library for linux. I put this together while packaging xrootd for void linux, which supports both glibc and musl ( void-linux/void-packages#34647 ).

@matthewfeickert
Copy link
Contributor

@BenJarg do you have suggestions on additions that need to be made to the build chain? It would be helpful if you could both:

@Chocimier
Copy link
Contributor

Hello,

here is a workflow building on musl Void: https://github.com/Chocimier/xrootd/commits/musl

@matthewfeickert
Copy link
Contributor

Hello,

here is a workflow building on musl Void: https://github.com/Chocimier/xrootd/commits/musl

@BenJarg Do you want to take @Chocimier's CI contributions and commit those to this PR with a co-author note in the commit? e.g.

Co-authored-by: =?UTF-8?q?Piotr=20W=C3=B3jcik?= <chocimier@tlen.pl>

(@Chocimier that's what shows up as your name associated with commits at least according to https://github.com/Chocimier/xrootd/commit/be6e4dae6b617672ff11062d8e5a6d8fa9b97ddb.patch — if that's supposed to just be Piotr can you let us know?)

@BenJarg BenJarg force-pushed the musl branch 2 times, most recently from 378e195 to 21809d4 Compare July 24, 2022 05:02
@BenJarg
Copy link
Contributor Author

BenJarg commented Jul 24, 2022

Rebased and added @Chocimier 's contributions. I see them listed as the author and me the committer; let me know if there's something else to do to make that more clear.

@Chocimier
Copy link
Contributor

Fine for me.

@abh3
Copy link
Member

abh3 commented Dec 16, 2022

OK, it seems no one really wants to take ownership of this. So, here is the problem. The patch here depends on inference (i.e. we have Linux and not something else). That is not a good approach. This patch will only be accepted if actual availability has been prescreened and a specific cmake variable has been set for applicability. That is, there needs to be a test to determine that must is indeed available and would set a specific cmake variable (i.e. have_muscl) this would then determine what is actually compiled. I am sorry to say that the current way of doing this is way to problematic.

@BenJarg
Copy link
Contributor Author

BenJarg commented Dec 17, 2022

@abh3 Yeah, this patch was originally an ad hoc solution to get xrootd to compile on musl for Void Linux, where glibc and musl are the two possible libcs. I went ahead and added some checks in cmake to see if all the relevent musl things are available given it's not glibc, then defines HAVE_MUSL_LIBC if they are. Let me know if you'd like any changes to the way this is done.

@simonmichal
Copy link
Contributor

@BenJarg : are those tests:

     check_function_exists( __fseterr HAVE_fseterr )
     check_include_file( bits/alltypes.h HAVE_BITS_ALLTYPES )
     check_include_file( stdio_ext.h HAVE_STDIO_EXT )

unique for musl?

In general it looks good, all the tests passed including the one you added for voidlinux.

Maybe just to be on the safe side let's ask @amadio for his opinion ;-)

@BenJarg
Copy link
Contributor Author

BenJarg commented Dec 19, 2022

@simonmichal As far as I can tell, __fseterr and bits/alltypes.h are unique to musl, or at least I don't see them in glibc or the other Linux libcs I checked. Those three things are the features used in the musl-specific code blocks, so I think testing the three of them should be enough. Although, I don't have a lot of experience with alternative libcs, so input from others would be appreciated.

@amadio
Copy link
Member

amadio commented Feb 14, 2023

I am taking a look at merging this for 5.6. I think some of the changes can be simplified, though, like including sys/types.h in a few places rather than bits/alltypes.h with specific defines.

amadio added a commit to amadio/xrootd that referenced this pull request Feb 14, 2023
Based on work from pull request xrootd#1632.

Co-authored-by: Guilherme Amadio <amadio@cern.ch>
amadio added a commit to amadio/xrootd that referenced this pull request Feb 15, 2023
Based on work from pull request xrootd#1632.

Co-authored-by: Guilherme Amadio <amadio@cern.ch>
amadio added a commit to amadio/xrootd that referenced this pull request Feb 17, 2023
Based on work from pull request xrootd#1632.

Co-authored-by: Guilherme Amadio <amadio@cern.ch>
@amadio
Copy link
Member

amadio commented Feb 17, 2023

This has been superceded by #1908, which I just merged into the master branch. Thanks for the initial work on this!

@amadio amadio closed this Feb 17, 2023
amadio added a commit to amadio/xrootd that referenced this pull request May 23, 2023
Based on work from pull request xrootd#1632.

Co-authored-by: Guilherme Amadio <amadio@cern.ch>
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

6 participants