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

Make work with CMake MUMPS package #536

Open
wants to merge 3 commits into
base: devel
Choose a base branch
from

Conversation

scivision
Copy link

@scivision scivision commented Aug 26, 2024

the Git commits are independent. The first is to allow expected behavior with find_package by setting the maximum policy version to 3.12. This does not affect CMake < 3.12.

The final commit adds logic to work with MUMPS CMake package https://github.com/scivision/mumps that has become a popular way over the past several years to use MUMPS regardless of operating system or compiler or MUMPS options.

This also enables a possible future PR where MUMPS would automatically be downloaded and built if not found.

Comment on lines -510 to +515
IF(WITH_Mumps)
FIND_PACKAGE(Mumps REQUIRED)
ENDIF()
if(WITH_Mumps)
find_package(MUMPS CONFIG)
if(MUMPS_FOUND)
set(Mumps_FOUND true)
set(Mumps_LIBRARIES ${MUMPS_LIBRARIES})
set(Mumps_INCLUDE_DIR ${MUMPS_INCLUDE_DIRS})
if(MUMPS_METIS_FOUND)
set(Metis_FOUND true)
set(Metis_LIBRARIES ${METIS_LIBRARIES})
set(Metis_INCLUDE_DIR ${METIS_INCLUDE_DIRS})
endif()
else()
FIND_PACKAGE(Mumps REQUIRED)
endif()
endif()
Copy link
Contributor

@mmuetzel mmuetzel Aug 26, 2024

Choose a reason for hiding this comment

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

To make this more similar to what is done for a similar situation with UMFPACK or CHOLMOD, would it be possible to move this to FindMumps.cmake?
See, e.g.,

# Try to find with CMake config file of upstream UMFPACK.
FIND_PACKAGE(UMFPACK CONFIG)
IF(UMFPACK_LIBRARIES AND UMFPACK_INCLUDE_DIR)
RETURN()
ENDIF()
.

You'll probably need to move that upper-case to lower-case translation to that file, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

As background for why moving that part to FindMumps.cmake would be nice:
Currently, ElmerFEM doesn't install .cmake files. (It only optionally installs pkg-config files.) But if at some point in the future a feature that installs cmake modules alongside ElmerFEM should be integrated, that hunk of logic would need to be repeated wherever those installed .cmake modules would be used in downstream code.
It would be a lot cleaner if that hunk of logic would be integrated in the existing FindMUMPS.cmake file.

@mmuetzel
Copy link
Contributor

Thank you for your contribution.
Apart from a small review request, this looks good to me. 👍

Copy link
Contributor

@mmuetzel mmuetzel left a comment

Choose a reason for hiding this comment

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

Why is the commit "require CMake >= 3.12 so find_package works as expected" needed?
What doesn't work as expected with previous versions?

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.

2 participants