-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Remove redundant feature checks on re-run of CMake config step #2084
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
Conversation
|
@LebedevRI: Can you please re-trigger the CI? Is github having network issues?
Edit: Fixed MacOS failures by removing |
- On a config re-run, CMake will not check features again as it will read previously defined cache variables. - Logic was difficult to follow, so refactored `cxx_feature_check` for simplicity
985f7b6 to
b2d787a
Compare
|
It seems that there are intermittent failures already crept into
A quick look at the build-and-test workflow indicates that the pipeline is already failing tests that are showing up here as well on Based on this evidence, I believe that the failures are unrelated. @LebedevRI: Can you please merge this and are the sporadic failures in |
LebedevRI
left a comment
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.
Is this AI-made?
| if(DEFINED RUN_${FEATURE} AND RUN_${FEATURE} EQUAL 0) | ||
| message(STATUS "Performing Test ${FEATURE} -- success") | ||
| set(HAVE_${VAR} 1 PARENT_SCOPE) | ||
| add_definitions(-DHAVE_${VAR}) |
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.
Why?
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.
Why what?
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.
Since the line is only on the LHS of the diff, why this line and the other snippet being removed.
This literally renders this whole check pointless, since we no longer communicate it's results.
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.
I am not sure what you mean as the CACHE token here is communicating results at the parent scope:
set(${VAR} TRUE CACHE BOOL "" FORCE)
No other variables need to escape the function scope.
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.
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.
Yes, HAVE_STD_REGEX, HAVE_GNU_POSIX_REGEX or HAVE_POSIX_REGEX are being properly propagated in my patch, depending on which REGEX engine is detected. If it wasn't, that file wouldn't compile with my changes and CI would fail.
As an example, in my local machine, it detects HAVE_STD_REGEX and does the right thing.
I am not sure I see the issue.
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.
Yes,
HAVE_STD_REGEX,HAVE_GNU_POSIX_REGEXorHAVE_POSIX_REGEXare being properly propagated in my patch, depending on which REGEX engine is detected.
cmake cache variables - sure.
Preprocessor defines - no, they are not.
Could you please point me at where they are being set.
The answer i was looking for was: "are you illiterate or something, just on the line above, there's add_compile_definitions()".
If it wasn't, that file wouldn't compile with my changes and CI would fail.
This is not quite true, as you can see from that snippet, it has fallback defaults.
No. |
LebedevRI
left a comment
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.
I think this is probably fine.
@theComputeKid thank you!
cxx_feature_checkfor simplicityOn CMake config first run:
Before (slow): checks all REGEX engines
After (fast): short circuits on first REGEX engine found.
On CMake config re-run:
Before (slow due to checks):
After (fast as checks skipped): No output (as checks not performed)
Fixes #2078