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

Add missing MPI target in src/CMakeLists.txt #556

Closed

Conversation

climbfuji
Copy link
Collaborator

Add missing MPI target in src/CMakeLists.txt

Not sure why this is not causing any errors currently because it should be there! The line that gets replaced is old and should have been removed in the past (neither LIBS nor CMAKE_DL_LIBS is defined).

User interface changes?: No

Fixes: [Github issue #s] n/a

Testing:
test removed: n/a
unit tests: n/a
system tests: n/a
manual testing: on my macOS with ufs-weather-model

Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

Is the line needed at all? If you are using an MPI compiler (or compiler wrapper), I thought the MPI libraries were always linked in by default.
Still, I can't see how this could hurt.

@climbfuji
Copy link
Collaborator Author

Is the line needed at all? If you are using an MPI compiler (or compiler wrapper), I thought the MPI libraries were always linked in by default. Still, I can't see how this could hurt.

It is needed, because we are NOT using mpicc etc as default compilers. As I understand, the cleaner way is to use the compiler gcc etc as default and only use mpicc etc when needed. Every cmake target should define exactly what it needs, including mpi. This way you avoid overlinking.

Copy link
Collaborator

@dustinswales dustinswales left a comment

Choose a reason for hiding this comment

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

Good catch.

@climbfuji
Copy link
Collaborator Author

climbfuji commented Apr 15, 2024

@grantfirl @dustinswales Can we add this to one of the next ufs-w-m PRs?

@dustinswales
Copy link
Collaborator

@climbfuji I don't see any reason not to.
I will leave it to @grantfirl @Qingfu-Liu @mkavulich, who sit in on the UFS application code mgmt. meeting

@grantfirl
Copy link
Collaborator

This will get merged with NOAA-EMC/fv3atm#816.

@mkavulich
Copy link
Collaborator

Combined with #555 for testing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants