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.

Sign up for GitHub

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

Jump to bottom

Fix CMake check of Random123 generators #2349

Merged
merged 1 commit into from
Mar 28, 2022
Merged

Fix CMake check of Random123 generators #2349

merged 1 commit into from
Mar 28, 2022

Conversation

hakonsbm
Copy link
Contributor

The CMake if-test does not short-circuit, so both COMPILE_RESULT and RUN_RESULT were always checked. But if the compilation fails, RUN_RESULT is undefined and CMake crashes.

The if-test is changed to test COMPILE_RESULT and RUN_RESULT separately. I've verified that it works now in cases with failing compilation, with failing tests, and with working compilation and passing tests.

@hakonsbm hakonsbm added T: Bug Wrong statements in the code or documentation S: High Should be handled next I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Mar 18, 2022
@hakonsbm hakonsbm added this to To do in Installation via automation Mar 18, 2022
Installation automation moved this from To do to Review Mar 28, 2022
@heplesser heplesser requested a review from jougs March 28, 2022 08:35
gtrensch
gtrensch approved these changes Mar 28, 2022
View reviewed changes
Copy link
Contributor

@gtrensch gtrensch left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@heplesser heplesser removed the request for review from jougs March 28, 2022 13:17
@heplesser heplesser merged commit 01f6e5a into nest:master Mar 28, 2022
Installation automation moved this from Review to Done Mar 28, 2022
@hakonsbm hakonsbm deleted the fix_cmake_random123 branch August 1, 2022 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heplesser heplesser heplesser approved these changes

@gtrensch gtrensch gtrensch approved these changes

Assignees
No one assigned
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: High Should be handled next T: Bug Wrong statements in the code or documentation
Projects
Installation
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hakonsbm @heplesser @gtrensch