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

Set BUILD_SHARED_LIBS and CMAKE_POSITION_INDEPENDENT_CODE as Cache variables in toolchain #12401

Merged

Conversation

jcar87
Copy link
Contributor

@jcar87 jcar87 commented Oct 26, 2022

…riables in CMakeToolchain

Changelog: Bugfix: When recipes have shared and fPIC as options, define BUILD_SHARED_LIBS and CMAKE_POSITION_INDEPENDENT_CODE as CACHE variables in the generated cmake_toolchain.cmake instead of regular variables, so that they are not masked by further calls to options().

Docs: Omit

Close #11840

Notes

The bug in #11840 can happen when CMake is operating on a policy version older than 3.13 and the project's CMake scripts make option() calls for either BUILD_SHARED_LIBS and CMAKE_POSITION_INDEPENDENT_CODE. These calls can shadow the values defined by the toolchain (if the toolchain has been loaded already by project()). Using option() for these variables seems valid as per CMake's own documentation.

These variables are only defined in the generated conan_toolchain.cmake if the recipe has fPIC and shared as options. These may be a tad outside of the scope of CMake toolchains.

By setting them as cache variables, we ensure that option() does not override their values if option() is called after the toolchain is loaded, but we also ensure that if the users manually and explicitly overrides the value via -D flag, that value will be respected too.

One caveat is that for projects that set the values after cmake_minimum_required() but before project() - the behaviour remains undefined, the value will depend on how the variable is set.

@jcar87 jcar87 requested review from czoido and memsharded October 26, 2022 14:59
@jcar87 jcar87 force-pushed the bugfix/build-shared-libs-option-and-policy branch from 1178075 to 8f6e9ef Compare November 1, 2022 10:43
@jcar87 jcar87 marked this pull request as ready for review November 2, 2022 13:54
Co-authored-by: Francisco Ramírez <franchuti688@gmail.com>
@jcar87 jcar87 added this to the 1.54 milestone Nov 2, 2022
@jcar87 jcar87 merged commit 9e6f7d5 into conan-io:develop Nov 2, 2022
@jcar87 jcar87 deleted the bugfix/build-shared-libs-option-and-policy branch November 2, 2022 16:12
garethsb added a commit to garethsb/conan-center-index that referenced this pull request Nov 14, 2022
bump required_conan_version to ">=1.54.0" to get conan-io/conan#12401
@garethsb
Copy link

Note that this also resolves the similar issue if the upstream CMakeLists.txt uses:

set(BUILD_SHARED_LIBS ON CACHE BOOL "Build shared libraries")

A workaround in Conan 1.53.0 is:

        tc.cache_variables["CMAKE_POLICY_DEFAULT_CMP0126"] = "NEW"

Whereas CMP077 is for option() variables, CMP0126 is for set(CACHE) ones - see https://cmake.org/cmake/help/latest/policy/CMP0126.html.

@memsharded
Copy link
Member

Thanks for the tips @garethsb !

conan-center-bot pushed a commit to conan-io/conan-center-index that referenced this pull request Nov 16, 2022
* Bump boost/1.80.0, openssl/1.1.1s, zlib/1.2.13

* conan v2: ConanFile, tools

* Use export_conandata_patches and apply_conandata_patches

* conan v2: CMakeDeps generator

* Delete CMakeLists.txt as no longer required

* Hopefully unbreak patches and add patch_type and patch_description

* Hm, maybe omitting base_path was right after all

* Missed patch_type and patch_description from one patch

* Apply suggestions from code review

Co-authored-by: Uilian Ries <uilianries@gmail.com>

* CMakeDeps: update patches to use CMake targets rather than CONAN_INCLUDE_DIRS_xxx and CONAN_LIBS_xxx

* Sanity check exported symbol for a public static constant

* conan v2 test_package
bump required_conan_version to ">=1.54.0" to get conan-io/conan#12401

* workaround for conan 1.53.0

Co-authored-by: Uilian Ries <uilianries@gmail.com>
prince-chrismc pushed a commit to prince-chrismc/conan-center-index that referenced this pull request Nov 16, 2022
* Bump boost/1.80.0, openssl/1.1.1s, zlib/1.2.13

* conan v2: ConanFile, tools

* Use export_conandata_patches and apply_conandata_patches

* conan v2: CMakeDeps generator

* Delete CMakeLists.txt as no longer required

* Hopefully unbreak patches and add patch_type and patch_description

* Hm, maybe omitting base_path was right after all

* Missed patch_type and patch_description from one patch

* Apply suggestions from code review

Co-authored-by: Uilian Ries <uilianries@gmail.com>

* CMakeDeps: update patches to use CMake targets rather than CONAN_INCLUDE_DIRS_xxx and CONAN_LIBS_xxx

* Sanity check exported symbol for a public static constant

* conan v2 test_package
bump required_conan_version to ">=1.54.0" to get conan-io/conan#12401

* workaround for conan 1.53.0

Co-authored-by: Uilian Ries <uilianries@gmail.com>
@garethsb
Copy link

Unfortunately, it looks like the 1.53.0 workaround wasn't effective... all packages were built, all test packages passed... but actually a shared package had been built and used in some cases. #14227 settled on a different workaround:

-        tc.cache_variables["CMAKE_POLICY_DEFAULT_CMP0126"] = "NEW"
+        tc.variables["BUILD_SHARED_LIBS"] = self.options.shared

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants