-
Notifications
You must be signed in to change notification settings - Fork 251
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
Fix: remove the CMAKE_OSX_DEPLOYMENT_TARGET auto detect by CMake #628
base: develop2
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just an implementation detail of the variable "type"
conan_provider.cmake
Outdated
@@ -50,7 +50,7 @@ function(detect_os OS OS_API_LEVEL OS_SDK OS_SUBSYSTEM OS_VERSION) | |||
message(STATUS "CMake-Conan: cmake_osx_sysroot=${CMAKE_OSX_SYSROOT}") | |||
set(${OS_SDK} ${_OS_SDK} PARENT_SCOPE) | |||
endif() | |||
if(DEFINED CMAKE_OSX_DEPLOYMENT_TARGET) | |||
if(DEFINED CONAN_CUSTOM_CMAKE_OSX_DEPLOYMENT_TARGET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a bit weird that the variable is given the value of the original content, but then it is not used. If it is intended to be used as a boolean toggle, then maybe when it is assigned below, assign it directly to True/On?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! I would either:
- Make it a bool (along the lines of
CONAN_CMAKE_OSX_DEPLOYMENT_AUTODETECTED
) - or save the value, in case somehow the user does a
set(CMAKE_OSX_DEPLOYMENT_TARGET ...)
after project (i know, too many cases) - then we can compare. Although we don't have to cover this case, if the documentation says it should be cache variable, then it should!
conan_provider.cmake
Outdated
@@ -620,6 +620,10 @@ set(CONAN_HOST_PROFILE "default;auto-cmake" CACHE STRING "Conan host profile") | |||
set(CONAN_BUILD_PROFILE "default" CACHE STRING "Conan build profile") | |||
set(CONAN_INSTALL_ARGS "--build=missing" CACHE STRING "Command line arguments for conan install") | |||
|
|||
if(DEFINED CMAKE_OSX_DEPLOYMENT_TARGET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the docs of project()
Reading this I think we're good, and that the macOS version detection would happen after this file is processed (from CMAKE_PROJECT_TOP_LEVEL_INCLUDES
) - but if that's not the case, there's really nothing we can do
From https://cmake.org/cmake/help/latest/variable/CMAKE_OSX_DEPLOYMENT_TARGET.html - we need to consider 3 scenarios:
CMAKE_OSX_DEPLOYMENT_TARGET
is already defined as a cache variable and this is the first cmake run (typically because it was passed with-D
-> I would explicitly check for a cache variable (see https://cmake.org/cmake/help/latest/variable/CACHE.html)CMAKE_OSX_DEPLOYMENT_TARGET
is not defined, but the environment variableMACOSX_DEPLOYMENT_TARGET
is defined -> check for this as well (see https://cmake.org/cmake/help/latest/variable/ENV.html) - this has the same effect of "the user defined this on purpose"CMAKE_OSX_DEPLOYMENT_TARGET
is defined, but is not a cache variable - issue a warning, since according to the docs this could be a bit of an undefined behaviour (cmake may set the cache variable instead). Here we may want to interpret that this is intentional and thus respect this value.
Re: CMAKE_OSX_DEPLOYMENT_TARGET
as a cache variable, note that if the user did not specify this in the first run, this will be a cache variable in the second run - so we need to detect somehow when we are in the first run or not (we can use a cache variable for that).
conan_provider.cmake
Outdated
# Remove the CMAKE_OSX_DEPLOYMENT_TARGET from the CACHE to avoid errors | ||
if($CACHE{CMAKE_OSX_DEPLOYMENT_TARGET}) | ||
unset(CMAKE_OSX_DEPLOYMENT_TARGET CACHE) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to keep the CACHE clean so that we take the value of the variable only if it is passed as a parameter by the user. I don't know if this is exactly the expected use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads a bit concerning in my opinion, I don't think that the conan provider should be altering in any way the user defined CMAKE_xxxx variables.
@@ -615,6 +611,19 @@ macro(conan_provide_dependency_check) | |||
endmacro() | |||
|
|||
|
|||
macro(set_osx_deployment_target_flag) | |||
if(NOT DEFINED $CACHE{CONAN_USE_CMAKE_OSX_DEPLOYMENT_TARGET}) | |||
if (DEFINED CMAKE_OSX_DEPLOYMENT_TARGET) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may want to do if DEFINED $CACHE{...} OR DEFINED $ENV{...}
as both are valid ways for the user to express intent.
the non-cache CMAKE_OSX_DEPLOYMENT_TARGET
can be defined too - we may want to ignore it, or raise a warning (since CMake documentation says that it may be overwritten completely during the call to project) - that could be an elseif()
If we don't initialize the variable
CMAKE_OSX_DEPLOYMENT_TARGET
, it tries to retrieve it from an environment variable, and if not, it calculates it based on the host platform. To prevent this, we check that it is initialized before the call toproject()
.https://cmake.org/cmake/help/latest/variable/CMAKE_OSX_DEPLOYMENT_TARGET.html
Close: #627