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

[bug] conan_toolchain.cmake overwrites CMAKE_${lang}_FLAGS_INIT from user_toolchain #9575

Closed
puetzk opened this issue Sep 9, 2021 · 6 comments
Assignees
Milestone

Comments

@puetzk
Copy link

puetzk commented Sep 9, 2021

There are a couple design issues with how the cmake_flags_init currently seems to work.

  • CMAKE_LANG_FLAGS_INIT should not be a CACHE variable - it's only really consulted during cmake_initialize_per_config_variable, and setting it later won't change the resulting cached CMAKE__FLAGS. Changing it later and reconfiguring will not actually apply any changes, since this gets baked into CMakeFiles/3.20.0/CMakeCXXCompiler.cmake

  • Just flat-out overwriting it (with FORCE, even!) is pretty uncooperative, and will break if the user_toolchain that also had flags that neeed to be passed (e.g. we must pass winegcc -Bprefix and --target= options so it picks the right underlying gcc, and conan won't know anything about that, and shouldn't need to).

So I'd think at least

-set(CMAKE_CXX_FLAGS_INIT "${CONAN_CXX_FLAGS}" CACHE STRING "" FORCE)
-set(CMAKE_C_FLAGS_INIT "${CONAN_C_FLAGS}" CACHE STRING "" FORCE)
-set(CMAKE_SHARED_LINKER_FLAGS_INIT "${CONAN_SHARED_LINKER_FLAGS}" CACHE STRING "" FORCE)
-set(CMAKE_EXE_LINKER_FLAGS_INIT "${CONAN_EXE_LINKER_FLAGS}" CACHE STRING "" FORCE)
+set(CMAKE_CXX_FLAGS_INIT "${CMAKE_CXX_FLAGS_INIT} ${CONAN_CXX_FLAGS}")
+set(CMAKE_C_FLAGS_INIT "${CMAKE_C_FLAGS_INIT} ${CONAN_C_FLAGS}")
+set(CMAKE_SHARED_LINKER_FLAGS_INIT "${CMAKE_SHARED_LINKER_FLAGS_INIT} ${CONAN_SHARED_LINKER_FLAGS}")
+set(CMAKE_EXE_LINKER_FLAGS_INIT "${CMAKE_EXE_LINKER_FLAGS_INIT} ${CONAN_EXE_LINKER_FLAGS}")

or maybe

if(CONAN_CXX_FLAGS)
    string(APPEND CMAKE_CXX_FLAGS_INIT "${CONAN_CXX_FLAGS}")
endif()

Or maybe even rename these to CONAN_*_FLAGS_INIT in the toolchain file, to emphasize that they only do anything the first time around (when cmake is compiler probing)?

Environment Details (include every applicable attribute)

  • Operating System+version: Ubuntu 20.04
  • Compiler+version: 9.3.0-17ubuntu1~20.04
  • Conan version: 1.40
  • Python version: 3.8.10
@memsharded
Copy link
Member

I see. Lets try in #9576 to see if something breaks (we have had issues because of not CACHE FORCE some vars in the past).

@memsharded memsharded added this to the 1.40.1 milestone Sep 9, 2021
@puetzk
Copy link
Author

puetzk commented Sep 9, 2021

well, if you CACHE without FORCE, it won't recalculate on a re-run (keeping the previous cached value). but CACHE force is just about equivalent to not using CACHE at all (unless you're adding some further logic with if(DEFINED) so you sometimes use a cached value, sometimes recalculate and force a new one, based on additional criteria)

But this won't recalculate anyway, because CMAKE_*_FLAGS_INIT only affects things once (before CMAKE uses it to cache CMAKE_*_FLAGS).

@memsharded
Copy link
Member

This was fixed in #9576, will be released soon in 1.40.1, thanks!

@puetzk
Copy link
Author

puetzk commented Sep 25, 2021

Fixed indeed, but I did discover one other interesting wrinkle that might be worth a changelog mention (your call).

In addition to the problem I had with it (didn't play nice with a user_toolchain), the old way using of CACHE here breaks in CMake 3.21 (specifically if CMP0126 set(CACHE) does not remove a normal variable of the same name is NEW).

cmake_minimum_required(VERSION 3.20)
project(test LANGUAGES)

set(FOO 1)
set(FOO 2 CACHE STRING "" FORCE)
message(STATUS "FOO=${FOO}")

FOO=2

cmake_minimum_required(VERSION 3.21)

FOO=1

set(CACHE ... FORCE still overwrites the CACHE value with 2, but in the NEW mode it no longer removes the shadowing directory-scoped variable, which is preferred by the search order of variable references:

https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#variables

When evaluating Variable References, CMake first searches the function call stack, if any, for a binding and then falls back to the binding in the current directory scope, if any. If a "set" binding is found, its value is used. If an "unset" binding is found, or no binding is found, CMake then searches for a cache entry. If a cache entry is found, its value is used. Otherwise, the variable reference evaluates to an empty string. The $CACHE{VAR} syntax can be used to do direct cache entry lookups.

@puetzk
Copy link
Author

puetzk commented Sep 25, 2021

The fix in #9576 should already address this too, but it means it might have more widespread impact than I had realized when I reported it as a bug for those doing fancy stuff in user_toolchain.

@memsharded
Copy link
Member

Thanks for highlighting this issue. It sounds good that this fix helped with that CMake 3.21 issue too.

Not sure if a late changelog change would help much, the change is already documented, and as long as the behavior is the expected one now, I'd say we are good.

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

No branches or pull requests

2 participants