-
Notifications
You must be signed in to change notification settings - Fork 69
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
Fix target_link_libraries calls w/ alias target #147
base: main
Are you sure you want to change the base?
Conversation
It seems to build for me. What issues do you see?
|
I am using
|
I can't tell from your input/output for certain, but make sure CMake is not downloading AMReX and that you're using an existing AMReX installation with
|
I see. Let me try again later today. |
Rebased to remove the updates to the CI workflows to install {cu,roc}SPARSE since @ajnonaka leapfrogged me. :) |
get_target_property(AMReX_ALIASED AMReX::amrex ALIASED_TARGET) | ||
target_link_libraries(${AMReX_ALIASED} INTERFACE ${MPI_LIBRARIES}) |
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.
Do the tutorials have a specific dimensionality?
AMReX::amrex
is for backwards compatibility and refers to the last-built spacedim (before, we could only build one -- keeping this target name around made transitions non-breaking/easier). But we have explicit AMReX::amrex_1d
/AMReX::amrex_2d
/AMReX::amrex_3d
targets that we should now use.
https://github.com/AMReX-Codes/amrex/blob/041f225dc5c97a5e3baf2559046ceabffd75f002/Src/CMakeLists.txt#L8-L15
@@ -63,7 +63,8 @@ else() | |||
|
|||
# Add mpi | |||
find_package(MPI REQUIRED) |
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.
Funny that we need MPI here at all, because if we need MPI, AMReX has to be built with MPI, which means MPI is already linked as a public transitive dependency in the AMReX::amrex_*d
targets.
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.
AMReX has to be built with MPI
Should we check for that? Or because we don't necessarily need MPI, do we just go with whatever the AMReX pkg we find already has (or has not)?
Transitive already in AMReX
Hey @ax3l, @atmyers, @WeiqunZhang,
I think the guided tutorials are no longer building?
Feel free to edit with a better/shorter fix. :)
Cheers,
-Nuno