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

cmake: REQUIRED packages HYPREConfig.cmake #473

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

junghans
Copy link
Contributor

@junghans junghans commented Sep 3, 2021

When a package uses a mpi-enabled hypre, mpi is actually required, same for lapack, blas and OpenMP.

@osborn9

@cedricchevalier19
Copy link

Just an user comment on this: I think it's good to have the REQUIRED concept in the client but for packaging, with Spack for example, it should be better to have the REQUIRED keyword when doing the actual find_package in Hypre's own CMakeLists.txt. find_dependency should properly forward the REQUIRED and QUIET kewords (https://cmake.org/cmake/help/latest/module/CMakeFindDependencyMacro.html).
The advantage of this is if user asks for Hypre with OpenMP and OpenMP is not found, cmake will fail. For automatic installations it is a lot easier to fix fail at configure time than try to figure out why some features are missing later.

@osborn9 osborn9 self-assigned this Sep 8, 2021
@osborn9
Copy link
Contributor

osborn9 commented Sep 8, 2021

Ok, so based on @cedricchevalier19 comment, since we are using the REQUIRED keyword within the find_package calls then these keywords should be correctly forwarded.

@junghans Are there instances when the find_dependency isn't behaving as expected?

@junghans
Copy link
Contributor Author

junghans commented Sep 8, 2021

It is mainly good practice, but you could construct a case where you e.g. compile a consumer code with a different compiler that doesn't support OpenMP and then hypre is looking for OpenMP and doesn't find. Throwing an error in these cases seem better.

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

3 participants