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: Modernize DEAL_II_PACKAGE_HANDLE macro and clean up Find*.cmake files #13325

Merged
merged 10 commits into from Feb 7, 2022

Conversation

tamiko
Copy link
Member

@tamiko tamiko commented Feb 2, 2022

This pull request changes the DEAL_II_PACKAGE_HANDLE macro:

  • simplify logic by removing unnecessary booleans
  • use temporary variables to accumulate lists/string and override canonical names at end

This allows to use "canonical" names such as "FEATURE_LIBRARIES" already in
the call to DEAL_II_PACKAGE_HANDLE by clearing these variables at the end
and not at the beginning of the DEAL_II_PACKAGE_HANDLE macro.

This also raises the required minimal CMake version to 3.3.0 which was released 7 years ago.

@tamiko
Copy link
Member Author

tamiko commented Feb 2, 2022

I have manually verified that this still produces the same detailed.log output than the current master branch with one notable change: Empty variables are now removed from detailed.log.

Copy link
Member

@tjhei tjhei left a comment

Choose a reason for hiding this comment

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

CMake is beautiful. I am not sure I understood everything, but it looks reasonable.

@@ -33,14 +33,14 @@
MESSAGE(STATUS "This is CMake ${CMAKE_VERSION}")
MESSAGE(STATUS "")

CMAKE_MINIMUM_REQUIRED(VERSION 3.1.0)
CMAKE_MINIMUM_REQUIRED(VERSION 3.3.0)
Copy link
Member

Choose a reason for hiding this comment

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

we currently require 3.1 according to https://www.dealii.org/developer/readme.html
Do you need 3.3? If yes then we need to adjust.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I chose to simply bump to 3.3.0. It will make some cleanup of the CMake code I plan to do easier - and version 3.3.0 is reasonably ancient (Ubuntu 16.04 LTS already shipped 3.5.0). I have adjusted the readme and the rest of the documentation to state 3.3.0.

AND "${_suffix}" STREQUAL "LIBRARIES")
LIST(APPEND _temp_${_suffix} ${_arg})

ELSEIF("${_arg}" STREQUAL "CLEAR") # FIXME
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot to finish refactoring. The variable _fill_clear is now gone and the FIXMEs have been removed.

cmake/macros/macro_deal_ii_package_handle.cmake Outdated Show resolved Hide resolved
cmake/macros/macro_deal_ii_package_handle.cmake Outdated Show resolved Hide resolved
@tamiko
Copy link
Member Author

tamiko commented Feb 5, 2022

@tjhei Thanks for the thorough read! I hope I have addressed all review comments. I checked that my latest changes still produce the same CMakeCache.txt and detailed.log file on my system. But let's see what the testsuite has to say.

Copy link
Member

@tjhei tjhei left a comment

Choose a reason for hiding this comment

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

Looks good. Would you mind writing a changelog entry for the cmake version change?

@tamiko
Copy link
Member Author

tamiko commented Feb 5, 2022

@tjhei Done!

@masterleinad masterleinad merged commit 12b083b into dealii:master Feb 7, 2022
@masterleinad masterleinad deleted the cmake_fix_1 branch February 7, 2022 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants