minetest / irrlicht

Minetest's fork of Irrlicht
Other
115 stars 87 forks source link

User-specified compilation flags are ignored #164

Open numberZero opened 1 year ago

numberZero commented 1 year ago

https://github.com/minetest/irrlicht/blob/5527b9f373ef4267a2b44f816be07ddab48a289d/source/Irrlicht/CMakeLists.txt#L17-L18 This was probably intended to provide default values for CMAKE_CXX_FLAGS_RELEASE and (for whatever reason) CMAKE_CXX_FLAGS_DEBUG. Instead, it sets regular variables which hide the user-specified (cache) variables with hard-coded values.

E.g. I have this in my CMakeCache: CMAKE_BUILD_TYPE:STRING=Release CMAKE_CXX_FLAGS_RELEASE:STRING=-O3 -DNDEBUG But actual compilation command doesn’t contain -DNDEBUG.

sfan5 commented 1 year ago

Minetest does this too: https://github.com/minetest/minetest/blob/master/src/CMakeLists.txt#L791-L801

aren't users supposed to put their own wishes into CMAKE_CXX_FLAGS?

JosiahWI commented 1 year ago

CMake has variables by these names since version 3.11 as documented.

Our variables by the same names are, to my knowledge, internal. This is very confusing, and I suggest we change the names of our variables to something like IRR_CXX_FLAGS_RELEASE and IRR_CXX_FLAGS_DEBUG so they won't be confused with the built-in variables in 3.11 and onward. Alternatively, we could increase our minimum CMake version to 3.11, and use list(APPEND ...) instead of set(...) so that the cache variables are not shadowed. Yet again, we could set them as CACHE variables, so that the user provided values override them. The latter two solutions would need additional documentation, of course.

Also note that there's a documented variable that can be used to initialize the value of CMAKE_<LANG>_FLAGS_<CONFIG>. This offers a fourth solution.

The last link explicitly states that CMAKE_<LANG>_FLAGS_<CONFIG> is a cache variable, which means it should be possible for the user to override it from the command line. Our behavior is contradictory to the official CMake documentation for CMake version 3.11 and onwards.

numberZero commented 1 year ago

CMake has variables by these names since version 3.11 as documented.

CMake 2.6 has these already, just documented separately: https://cmake.org/cmake/help/cmake2.6docs.html#variable:CMAKE_LANG_FLAGS_DEBUG https://cmake.org/cmake/help/cmake2.6docs.html#variable:CMAKE_LANG_FLAGS_RELEASE

sfan5 commented 1 year ago

Sounds like we need to fix it in MT and Irrlicht then, PRs welcome.