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

Properly handle OpenMP linker flags #3051

Merged
merged 2 commits into from
Feb 5, 2024
Merged

Conversation

heplesser
Copy link
Contributor

Until now, we placed OpenMP_CXX_FLAGS in the MODULE_LINK_LIBS for nest. This is not quite correct and meant that libomp was missing from the list of libraries returned by nest-config --libs, which in turn caused trouble for building external models with OpenMP, at least on macOS. This PR fixes this.

@heplesser heplesser 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 Dec 15, 2023
@heplesser heplesser added this to To do in Build system and CI via automation Dec 15, 2023
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 contribution. It looks good to me. I would just like to make one suggestion for improvement. In order to report on the OpenMP libraries used, we could add the following to ConfigureSummary.cmake, after line 102:
message( " Libraries : ${OpenMP_CXX_LIBRARIES}" )

@heplesser
Copy link
Contributor Author

Thank you for this contribution. It looks good to me. I would just like to make one suggestion for improvement. In order to report on the OpenMP libraries used, we could add the following to ConfigureSummary.cmake, after line 102: message( " Libraries : ${OpenMP_CXX_LIBRARIES}" )

@gtrensch Thanks for the suggestion, fixed!

@heplesser
Copy link
Contributor Author

@gtrensch @clinssen Could you (re-)review asap, this is blocking progress elsewhere.

@heplesser heplesser removed the request for review from clinssen February 5, 2024 14:16
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 merged commit b8b2347 into nest:master Feb 5, 2024
24 checks passed
Build system and CI automation moved this from To do to Done Feb 5, 2024
@heplesser heplesser deleted the omplibs branch April 24, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants