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

Allow more than one optimization flag to be set when running CMake #2551

Merged
merged 10 commits into from
Jan 12, 2023
Merged

Allow more than one optimization flag to be set when running CMake #2551

merged 10 commits into from
Jan 12, 2023

Conversation

med-ayssar
Copy link
Contributor

Fixes #2548

@jessica-mitchell jessica-mitchell requested review from heplesser, gtrensch and terhorstd and removed request for heplesser December 1, 2022 08:21
@jessica-mitchell jessica-mitchell added this to To do in Installation via automation Dec 1, 2022
terhorstd
terhorstd requested changes Dec 1, 2022
View reviewed changes
Copy link
Contributor

@terhorstd terhorstd left a comment •
edited

Choose a reason for hiding this comment

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

Nice addition 👍 Some minor suggestions for usability

CMakeLists.txt Show resolved Hide resolved
cmake/ProcessOptions.cmake Outdated Show resolved Hide resolved
cmake/ProcessOptions.cmake Outdated Show resolved Hide resolved
Installation automation moved this from To do to Review Dec 1, 2022
@gtrensch gtrensch mentioned this pull request Dec 1, 2022
Closed
gtrensch
gtrensch requested changes Dec 2, 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.

Thank you for this work 👍. Will be approved when the requested changes are made.

@med-ayssar
Copy link
Contributor Author

Thank you for this work +1. Will be approved when the requested changes are made.

Actually, I found a more compact version. So maybe if you please take a look at the changes again.

terhorstd
terhorstd approved these changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

All regular cases and most more extravagant cases seem to work. The only unintuitive behavior I found was using multiple -Dwith-optimize=… flags, which is however regular CMake behavior. There is little chance to change this from NEST side.

For me this looks good now! 👍

cmake/ProcessOptions.cmake Show resolved Hide resolved
gtrensch
gtrensch approved these changes Dec 16, 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! 👍

jougs
jougs reviewed Jan 12, 2023
View reviewed changes
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

I have just some wording changes for the comment on the function call positions.

CMakeLists.txt Outdated Show resolved Hide resolved
@jougs jougs added T: Enhancement New functionality, model or documentation S: Normal Handle this with default priority I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know labels Jan 12, 2023
@jougs jougs changed the title Add more than one optimization flag Allow more than one optimization flag to be set when running CMake Jan 12, 2023
@jougs jougs merged commit 32daadc into nest:master Jan 12, 2023
Installation automation moved this from Review to Done Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jougs jougs jougs left review comments

@terhorstd terhorstd terhorstd approved these changes

@gtrensch gtrensch gtrensch approved these changes

Assignees
No one assigned
Labels
I: Internal API Changes were introduced in basic internal workings of the simulator that developers need to know S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects
Installation
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cmake ignores the list of optimization options and chooses only the last item in the list
4 participants
@med-ayssar @jougs @terhorstd @gtrensch