-
Notifications
You must be signed in to change notification settings - Fork 857
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
Require C11 #12660
base: main
Are you sure you want to change the base?
Require C11 #12660
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for 5.x
Mhh, CI fails with this error:
I tried adding POSIX feature test macros but that doesn't seem to help. There seems to be a fix in |
Add I guess you will need to so the same for all lex/yacc outputs |
8714b5e
to
c6ba95b
Compare
config/opal_setup_cc.m4
Outdated
AC_MSG_NOTICE([using $flag to ensure C11 support]) | ||
break | ||
fi | ||
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to put a check here to ensure that some flag was found and that C11 is, indeed, supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do error out if C11 is not supported (the block above). This is just adding the C11 flag as a restriction (GCC will eventually default to C23 so we could accidentally sneak in C23 code that). If we don't find a flag for some reason we can just move on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah -- so there's no case where an additional flag is needed, but we can't find that flag? If that's true, then this is good enough.
I ask because $opal_cv_c11_flag_required
is set to true if we try to compile a C11 program (i.e., checking __STDC_VERSION__
) with no additional flags and it fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OPAL_PROG_CC_C11
macro sets the -std=c11
parameter if it's needed for C11 support and then tries to pick the right flag. If we don't find a suitable flag we error out. If C11 support is available without a flag then we get here and try to find one to restrict ourselves. So this block is somewhat optional and we I'm ok with removing it if we don't want to restrict ourselves to C11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, gotcha. Yes, if this is redundant code, let's ditch it.
Per your other point: I can't think of a reason to restrict ourselves to C11 -- can you? If neither of us can think of one, I think we're clear to remove this block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh, I was thinking we should protect ourselves from unintentionally introducing C23 code once compilers default to that but maybe that risk is low. I removed that part of configure.
9a54aae
to
4ec6936
Compare
Not sure why CI fails. All tests seem green but at the end it's marked as failed. Can someone please point me to the error? |
Got it. Tests show green even if they failed in Jenkins... The culprit is our tests on RHEL 7.9, which still ships GCC 4.8. GCC added support for I see three options:
[1] https://gcc.gnu.org/wiki/C11Status |
The extended supported is a paid subscription IIUC so I don't think we can strictly secure the test environment. FYI EFA is dropping support for CentOS 7 and RHEL 7. |
That's an important data point, thanks @wenduwan. I put this topic on the list for the next devel call. |
58dc607
to
f925b01
Compare
It looks like the Mac OS X environment is borked. It fails to find
|
Take a gander at @wenduwan comment in the slack "general" thread - looks like GitHub is making some significant changes to the Mac "actions" environment. |
@rhc54 Thanks for the pointer. According to https://github.blog/changelog/2024-08-19-notice-of-upcoming-deprecations-and-breaking-changes-in-github-actions-runners/ the macos-12 are phased out but we're using macos-14-arm64 so I don't think we're affected here. |
Error out if the compiler does not support C11. Also removes support for `__thread` since we now rely on C11 `_Thread_local`. Signed-off-by: Joseph Schuchart <[email protected]>
All uses of the `PREPARE_REQUESTS_WITH_NO_FREE` macro map to size_t. Signed-off-by: Joseph Schuchart <[email protected]>
Flex prior to 2.6.6 uses fileno() but does not define the needed feature test macros. See westes/flex#263 for details. Signed-off-by: Joseph Schuchart <[email protected]>
Signed-off-by: Joseph Schuchart <[email protected]>
Move calls to nanosleep out of headers and add the feature test macro _POSIX_C_SOURCE to enable its use. This should not cause any significant overheads. Signed-off-by: Joseph Schuchart <[email protected]>
Needed for strsep(), posix_memalign(), and getopt(). Also fix the min/max macros. There is no need to cache the variables as these macros are used without producing side-effects. Signed-off-by: Joseph Schuchart <[email protected]>
f925b01
to
13e8ddb
Compare
Error out if the compiler does not support C11 and limit the standard to C11 if the compiler accepts a standard flag. This limit prevents us from using features of newer standard versions.
Also removes support for
__thread
since we now rely on C11_Thread_local
.Please check my m4 code, not an expert on this.
See #12658 (review) for notes from the RM discussion and the PR that initiated this.
There will likely be more changes over time replacing pre-C11 features with C11 features where possible but I wanted to keep this manageable.
@jsquyres where should I put a note about this for users?