Skip to content

Fix: remove the CMAKE_OSX_DEPLOYMENT_TARGET auto detect by CMake #628

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: develop2
Choose a base branch
from
Open

Fix: remove the CMAKE_OSX_DEPLOYMENT_TARGET auto detect by CMake #628

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9d3e483
Fix: remove the CMAKE_OSX_DEPLOYMENT_TARGET auto detect by CMake
Mar 13, 2024
a433805
Feat: remove the CMAKE_OSX_DEPLOYMENT_TARGET from CACHE after read
Mar 14, 2024
c0a6fa7
fix: add cache variable to control the os.version call
Mar 18, 2024
1479d94
fix: test os version
Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion conan_provider.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

@memsharded memsharded Mar 13, 2024

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?

Copy link
Contributor

@jcar87 jcar87 Mar 14, 2024

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!

message(STATUS "CMake-Conan: cmake_osx_deployment_target=${CMAKE_OSX_DEPLOYMENT_TARGET}")
set(${OS_VERSION} ${CMAKE_OSX_DEPLOYMENT_TARGET} PARENT_SCOPE)
endif()
Expand Down Expand Up @@ -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)
Copy link
Contributor

@jcar87 jcar87 Mar 14, 2024

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 variable MACOSX_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).

set(CONAN_CUSTOM_CMAKE_OSX_DEPLOYMENT_TARGET CMAKE_OSX_DEPLOYMENT_TARGET)
endif ()

find_program(_cmake_program NAMES cmake NO_PACKAGE_ROOT_PATH NO_CMAKE_PATH NO_CMAKE_ENVIRONMENT_PATH NO_CMAKE_SYSTEM_PATH NO_CMAKE_FIND_ROOT_PATH)
if(NOT _cmake_program)
get_filename_component(PATH_TO_CMAKE_BIN "${CMAKE_COMMAND}" DIRECTORY)
Expand Down