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

GitHub: Silence clang-12 cross builds #668

Closed
wants to merge 2 commits into from

Conversation

jlduran
Copy link
Contributor

@jlduran jlduran commented Feb 25, 2023

The main idea is to stir a discussion, rather than present this pull request as the actual fix.

Can we just remove clang-12 from the test matrix?

cc/ @jrtc27, @emaste, @bsdjhb (educated guess of reviewers)

@bsdimp
Copy link
Member

bsdimp commented Mar 2, 2023

I think we can just drop clang12 from the mix... unless the change to linux/unistd.h really fixes it, in which case we should do that...

bsdimp pushed a commit to bsdimp/freebsd that referenced this pull request Mar 2, 2023
This reverts commit d6327ae.

Reviewed by: imp
Pull Request: freebsd#668
@lwhsu
Copy link
Member

lwhsu commented Mar 2, 2023

I think we can just drop clang12 from the mix... unless the change to linux/unistd.h really fixes it, in which case we should do that...

#678 seems working. BTW, if we're going to drop clang 12 support, we need to update https://docs.freebsd.org/en/articles/committers-guide/#_current_compiler_versions

@bsdimp
Copy link
Member

bsdimp commented Mar 2, 2023

I think we can just drop clang12 from the mix... unless the change to linux/unistd.h really fixes it, in which case we should do that...

#678 seems working. BTW, if we're going to drop clang 12 support, we need to update https://docs.freebsd.org/en/articles/committers-guide/#_current_compiler_versions

Yea, I think we should commit this fix, but keep clang12 since we need it still for stable/12 til its EOL. Might as well test it with a recent-ish ubuntu too.

@jlduran
Copy link
Contributor Author

jlduran commented Mar 2, 2023

You are right! it should be kept until EOY.

I just tested ubuntu-latest + clang-15 and it works as expected. I'll submit a separate pull request for evaluation, but before that, how should the final cross-build matrix be?:

OS version clang-12 clang-13 clang-14 clang-15
macos-latest
ubuntu-latest

@bsdimp
Copy link
Member

bsdimp commented Mar 7, 2023

Got the OK from @jrtc27 on IRC, with an appropriate commit message.

@bsdimp bsdimp added ready and removed needs-review labels Mar 7, 2023
Copy link
Member

@bsdimp bsdimp left a comment

Choose a reason for hiding this comment

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

I'll steal much of the comment to do the push...

@bsdimp bsdimp closed this Mar 7, 2023
freebsd-git pushed a commit that referenced this pull request Mar 7, 2023
This reverts commit d6327ae.

Before version 2.25, glibc's unistd.h would define the POSIX subset of
getopt.h by defining __need_getopt, including getopt.h (which would
disable the header guard) and then undefining it so later including
getopt.h explicitly would define the extensions. However, we wrap
getopt, and so the wrapper's #pragma once breaks that. Thus getopt.h was
included before the real unistd.h to ensure we get all the extensions.

However, with clang 12 that causes problems where we get a function
mismatch (since getopt can throw exceptions). If we include it after
unistd.h, it will get the full definitions since glibc no longer does
the subsetting thing. This will result in matching definitions and fix
clang 12.

Reviewed by: imp, jrtc27 (OK'd on irc)
Pull Request: #668
@jlduran jlduran deleted the silence-github-clang-12 branch March 8, 2023 03:28
bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Mar 22, 2023
This reverts commit d6327ae.

Before version 2.25, glibc's unistd.h would define the POSIX subset of
getopt.h by defining __need_getopt, including getopt.h (which would
disable the header guard) and then undefining it so later including
getopt.h explicitly would define the extensions. However, we wrap
getopt, and so the wrapper's #pragma once breaks that. Thus getopt.h was
included before the real unistd.h to ensure we get all the extensions.

However, with clang 12 that causes problems where we get a function
mismatch (since getopt can throw exceptions). If we include it after
unistd.h, it will get the full definitions since glibc no longer does
the subsetting thing. This will result in matching definitions and fix
clang 12.

Reviewed by: imp, jrtc27 (OK'd on irc)
Pull Request: freebsd/freebsd-src#668

(cherry picked from commit 320e7e0)
bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Mar 30, 2023
This reverts commit d6327ae.

Before version 2.25, glibc's unistd.h would define the POSIX subset of
getopt.h by defining __need_getopt, including getopt.h (which would
disable the header guard) and then undefining it so later including
getopt.h explicitly would define the extensions. However, we wrap
getopt, and so the wrapper's #pragma once breaks that. Thus getopt.h was
included before the real unistd.h to ensure we get all the extensions.

However, with clang 12 that causes problems where we get a function
mismatch (since getopt can throw exceptions). If we include it after
unistd.h, it will get the full definitions since glibc no longer does
the subsetting thing. This will result in matching definitions and fix
clang 12.

Reviewed by: imp, jrtc27 (OK'd on irc)
Pull Request: freebsd/freebsd-src#668
@emaste emaste added the merged label Jun 12, 2023
@bsdimp bsdimp removed the ready label Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants