FetchContent and install path updates

scivision / mumps

MUMPS via CMake

http://mumps-solver.org

MIT License

115 stars 46 forks source link

FetchContent and install path updates #7

Closed puneetmatharu closed 3 years ago

puneetmatharu commented 3 years ago

I've added a couple of updates that might be helpful, feel free to use them if you wish, or not:

Added a new option use_default_install_location to disable the overload of CMAKE_INSTALL_PREFIX if the default path is used. This allows people to pull the package in via FetchContent without it overriding the install path of their outer project.
Updated the install paths so that they install to include/MUMPS and lib/MUMPS (rather than include/ and lib/) so that they don't directly pollute system paths.

There are a lot of formatting changes because of my auto-formatter that might confuse things - the only actual changes are to the argument of the DESTINATION keyword of each install() call.

scivision commented 3 years ago

Thanks--I should have realized that install prefix would be a problem for FetchContent consumers. I changed in ae7e258c32d4c60c896c58ba4f3cc7c926e49f88 to NOT do this problematic cache override.

scivision commented 3 years ago

With regard to forcing relative install paths like lib/MUMPS and include/MUMPS, I think this is a matter of different practices by different projects. Instead of relative install paths, I have been using distinct install prefixes like:

cmake -B build -DCMAKE_INSTALL_PREFIX=~/local/mumps

and then set environment variables like:

export MUMPS_ROOT=~/local/mumps

scivision commented 3 years ago

So I really appreciate you letting me know about the FetchContent install prefix override issue, I will also check other projects of ours for the same problem.

With regard to install relative paths, I would like to retain the current Mumps approach of using distinct install prefixes. For our work, we often test across library versions, compiler vendors and compiler versions, and using distinct install prefixes has been a better fit for us.

puneetmatharu commented 3 years ago

Thanks--I should have realized that install prefix would be a problem for FetchContent consumers.

I changed in ae7e258c32d4c60c896c58ba4f3cc7c926e49f88 to NOT do this problematic cache override.

Excellent, thanks for sorting this out so quickly.

puneetmatharu commented 3 years ago

With regard to forcing relative install paths like lib/MUMPS and include/MUMPS, I think this is a matter of different practices by different projects. Instead of relative install paths, I have been using distinct install prefixes like:
cmake -B build -DCMAKE_INSTALL_PREFIX=~/local/mumps
and then set environment variables like:
export MUMPS_ROOT=~/local/mumps

Ah, I see! It makes sense for the project I'm working on to have its own project subdirectory as it installs so many files. I have made another update in my forked repo to adjust the FindMUMPS.cmake script with the new install locations if you wanted to use the format I mentioned. :)

puneetmatharu commented 3 years ago

So I really appreciate you letting me know about the FetchContent install prefix override issue, I will also check other projects of ours for the same problem.

With regard to install relative paths, I would like to retain the current Mumps approach of using distinct install prefixes. For our work, we often test across library versions, compiler vendors and compiler versions, and using distinct install prefixes has been a better fit for us.

Not a problem, glad I could help! Thanks for doing such a great job of exporting the package, it has otherwise made it incredibly easy to import the solver into my library.