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

Use more modern CMake #1022

Merged
merged 5 commits into from
Jun 11, 2024
Merged

Conversation

ACSimon33
Copy link
Contributor

@ACSimon33 ACSimon33 commented Jun 5, 2024

Description

At the moment, LAPACK sets the global compile flags via the CMAKE_Fortran_FLAGS cache variable. The problem with that is if someone wants to compile LAPACK as a sub-project inside a bigger CMake project, the CMAKE_Fortran_FLAGS that LAPACK sets will automatically be applied to all targets of the parent project, which is definitely not ideal.

There are two options to fix this:

  • Use CMAKE_Fortran_FLAGS as a local variable, i.e. we set it without the CACHE keyword. That way the flags will only be applied in the LAPACK directory, not changing the flags in the parent target.
  • Use add_compile_options(...) and add_link_options(...). The latter needs CMake >= 3.13

Since we are already using target_compile_options(...) for some libraries, I think we should use the second option and not change any variables directly.

#994 already pointed out that we currently need at least CMake 3.12 anyway because we are linking object libraries. So, the jump to CMake 3.13 wouldn't be completely unreasonable. The issue also mentioned the use of Fortran_PREPROCESS, which is actually a CMake 3.18 feature. We can still use CMake < 3.18, because non-existing target properties are just ignored. The only place where it is necessary is if BUILD_INDEX64_EXT_API is enabled because we need to preprocess all Fortran files in that case.


This MR changes all occurrences of CMAKE_Fortran_FLAGS and CMAKE_*_LINKER_FLAGS to use the functions instead. I compared the compiler and linker flags of the targets before and after the changes. The compiler flags are identical (possibly in a different order). The linker flags are fewer than before because CMAKE_Fortran_FLAGS was added to the compiler and linker flags, which isn't the case anymore. For most flags, this is ok because they are not needed in the linking phase. Those that are needed during compilation and linking are added explicitly via add_link_options(...) (I hope I didn't miss any; please check).

I bumped the CMake minimum version to 3.13 and 3.18 if BUILD_INDEX64_EXT_API is enabled. If the user uses a CMake < 3.18, BUILD_INDEX64_EXT_API is now disabled by default so that the default config works without changing any variables.

Additionally, I fixed the formatting of the CMake file.

Fixes #994

Checklist

  • The documentation has been updated.
  • If the PR solves a specific issue, it is set to be closed on merge.

langou
langou previously approved these changes Jun 5, 2024
@langou
Copy link
Contributor

langou commented Jun 5, 2024

Thanks @ACSimon33. I approved the PR. I am letting it sit here for a few days if some folks want to express some concerns. If no one expresses concerns, I'll merge in a few days. Thanks a lot. Julien.

@langou langou merged commit 163c34b into Reference-LAPACK:master Jun 11, 2024
10 checks passed
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.

LAPACK master does not compile with CMake 3.9
2 participants