Skip to content

Conversation

@matthewfeickert
Copy link
Contributor

@matthewfeickert matthewfeickert commented Nov 15, 2024

Description

  • Allow for CMAKE_CXX_FLAGS to use its default behavior of taking the value of CXXFLAGS if it exists in the environment.
  • Remove subtractive behavior of removing compiler flags and instead use additive behavior. This both makes the user more responsible for their flag choices and ensures that known required flags can be added without interfering as repeated flags are safe.
  • Set range of compatible CMake versions to allow more modern CMake to be used if available.
  • Correct typos:
    • CMAKE_INSTALL_INCLUDE_DIR -> CMAKE_INSTALL_INCLUDEDIR
  • Add pixi ignores to .gitignore.

* Allow for CMAKE_CXX_FLAGS to use its default behavior of taking
  the value of CXXFLAGS if it exists in the environment.
   - c.f. https://cmake.org/cmake/help/v3.31/envvar/CXXFLAGS.html
   - Set CMAKE_CXX_FLAGS and then determine all other compiler or linker
     flags.
* Remove subtractive behavior of removing compiler flags and instead
  use additive behavior. This both makes the user more responsible
  for their flag choices and ensures that known required flags
  can be added without interfering as repeated flags are safe.
* Set range of compatible CMake versions to allow more modern CMake
  to be used if available.
   - Bump lower bound on CMake to v3.12 when this feature was added.
   - c.f. https://cmake.org/cmake/help/latest/command/cmake_minimum_required.html
* Correct typos:
   - 'CMAKE_INSTALL_INCLUDE_DIR' -> 'CMAKE_INSTALL_INCLUDEDIR'
* Add pixi ignores to .gitignore.
@matthewfeickert matthewfeickert force-pushed the fix/allow-setting-cmake-cxx-flags-from-cxx-flags branch from 9db26ad to deac7cf Compare November 15, 2024 05:58
Copy link
Contributor Author

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

High level comments for the reviewer.

@@ -1,4 +1,4 @@
cmake_minimum_required (VERSION 3.0.2)
cmake_minimum_required(VERSION 3.12...3.31)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use range of compatible CMake versions (https://cmake.org/cmake/help/latest/command/cmake_minimum_required.html) which was introduced in v3.12. This will take the newest available CMake version between v3.12 and v3.31 that can be found a build time.

Comment on lines +24 to +25
# Set default CXXFLAGS but allow for environment override
# c.f. https://cmake.org/cmake/help/v3.31/envvar/CXXFLAGS.html
Copy link
Contributor Author

Choose a reason for hiding this comment

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

First determine the values of CMAKE_CXX_FLAGS: Either taken from the environment CXXFLAGS like normal, or if they are absent, set defaults manually.

Comment on lines +46 to +47
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -fsanitize=address ${CMAKE_CXX_FLAGS}" CACHE STRING "debug CXXFLAGS" FORCE)
set(CMAKE_EXE_LINKER_FLAGS_DEBUG "${CMAKE_EXE_LINKER_FLAGS_DEBUG} -fsanitize=address" CACHE STRING "debug linker flags" FORCE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once CMAKE_CXX_FLAGS has been set, then all other compiler or linker flags can be set accordingly in an additive fashion to their existing environmental defaults based on the CMAKE_CXX_COMPILER_ID.

@matthewfeickert matthewfeickert marked this pull request as ready for review November 15, 2024 06:03
@matthewfeickert
Copy link
Contributor Author

This is ready for review now.

Copy link
Owner

@scarrazza scarrazza left a comment

Choose a reason for hiding this comment

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

@matthewfeickert thanks a lot for this. I have tested on linux and m3 and it works fine.

@scarrazza scarrazza merged commit 1bc31df into scarrazza:master Nov 15, 2024
1 check passed
@matthewfeickert matthewfeickert deleted the fix/allow-setting-cmake-cxx-flags-from-cxx-flags branch November 15, 2024 06:50
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.

CMake CHECK_CXX_COMPILER_FLAG logic invalid

2 participants