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 for Musl libc #798

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add support for Musl libc #798

wants to merge 3 commits into from

Conversation

MaxDesiatov
Copy link
Contributor

Since Musl is sufficiently different from Glibc (see https://wiki.musl-libc.org/functional-differences-from-glibc.html), it requires additional checks for available headers.

Since Musl is sufficiently different from Glibc (see https://wiki.musl-libc.org/functional-differences-from-glibc.html), it requires additional checks for available headers.
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

compnerd
compnerd previously approved these changes Jun 22, 2023
os/generic_unix_base.h Outdated Show resolved Hide resolved
@compnerd compnerd dismissed their stale review June 22, 2023 15:54

Follow up change request

@MaxDesiatov MaxDesiatov requested a review from compnerd June 22, 2023 15:56
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

2 similar comments
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@@ -25,7 +25,7 @@
#endif
#include <sys/param.h>

#if __has_include(<sys/cdefs.h>)
#if __has_include(<sys/cdefs.h>) && (defined(__FreeBSD__) || defined(__OpenBSD__) || defined(__NetBSD__) || defined(__GLIBC__))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why we're making this particular change. Musl (at least the copy I'm looking at) doesn't have <sys/cdefs.h>, so wouldn't have tried to include it anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Alpine /usr/include/sys/cdefs.h is present and it contains #warning usage of non-standard #include <sys/cdefs.h> is deprecated, which fails due to our usage of -Werror.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I wonder if we can work out why we're including <sys/cdefs.h> and just not do that anywhere.

@@ -33,6 +33,8 @@
#if __has_include(<sys/event.h>)
#define HAS_SYS_EVENT_H 1
#include <sys/event.h>
#elif __has_include(<poll.h>)
#include <poll.h>
#else
#include <sys/poll.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a platform with <sys/poll.h> that doesn't also have <poll.h>? Could we just have <poll.h> here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, I'm a bit afraid to touch this, as this repo may have support for OpenBSD, FreeBSD etc, but we don't have CI set up for those to verify those don't break.

Copy link
Contributor

Choose a reason for hiding this comment

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

Those aren't officially supported platforms; I wouldn't worry too much about breaking them anyway, but in this particular case both OpenBSD and FreeBSD document poll as being in <poll.h>.

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

2 similar comments
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants