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

change CMakeDeps xxx_LIBRARIES to be a list of libraries not targets #12600

Conversation

memsharded
Copy link
Member

Changelog: Fix: change CMakeDeps xxx_LIBRARIES to be a list of libraries not targets
Docs: https://github.com/conan-io/docs/pull/XXXX

Attempt at:
#12012
#12180

Just a draft attempt to see if something breaks...

@jcar87
Copy link
Contributor

jcar87 commented Nov 24, 2022

I think it's valid for the xxx_LIBRARIES variable to be a target or a list of targets - at least this much is specified by CMake in its documentation. For compatibility this "should" work - CMake themselves do this for to provide backwards compatibility in the find GTest module: https://github.com/Kitware/CMake/blob/master/Modules/FindGTest.cmake#L201-L202

I wonder what the underlying problem was in #12012 -
The CMake documentation for check_source_compiles says that CMAKE_REQUIRED_LIBRARIES can be a imported targets too - so using targets instead of libraries in this case should not be in itself a problem here. Maybe it was related to the issue that was fixed here: #12049

On the other hand, check_source_compiles in that particular recipe was used to try and identify the version of OpenSSL from one of the macros defined in the headers - by causing the compiler to inspect that value and perform a check at compile time. A bit of an odd choice - CMake's Find OpenSSL module can already correctly retrieve this from parsing the headers (May not be the most elegant or convenient, but this is hidden away from consumers).

Perhaps not a big deal to change - though we would have to deal with the limitation only mapping one configuration (Release as usual), rather than the target that supports multiple.

I think the issue here: #12180 - is similar, it's checking to see if a symbol is present as a way of determining if a minimum version threshold is satisfied. I wonder if check_symbol_exists should also work for targets and if this is a CMake defect - on the other hand, the check is very redundant when recipes can handle this logic of ensuring a minimum version, but I do sympathise with wanting to avoid patching recipes

@memsharded
Copy link
Member Author

Good. I'll see if I can manage to do a repro case for the original issue.

@HappySeaFox
Copy link

This issue blocks my recipe. Will it be included in the next release?

@jcar87
Copy link
Contributor

jcar87 commented Jan 8, 2023

This issue blocks my recipe. Will it be included in the next release?

Hi @HappySeaFox - if your issue is related to failures in check_source_compiles (or any other cmake call that invokes try_compile()), could you please try passing CMAKE_TRY_COMPILE_CONFIGURATION to CMake to see if that fixes the issue?
e.g., in a recipe that is using CMakeToolchain:

def generate(self):
    tc = CMakeToolchain(self)
    ...
    tc.cache_variables[`CMAKE_TRY_COMPILE_CONFIGURATION`] = self.settings.build_type
    ...
    tc.generate()

@HappySeaFox
Copy link

HappySeaFox commented Jan 8, 2023

Hi! Yes, it's related to check_c_source_compiles: conan-io/conan-center-index#14871. I'll try once the CI is back online.

@HappySeaFox
Copy link

@jcar87 It didn't help, the tc.generate() step fails with TypeError: Object of type SettingsItem is not JSON serializable: https://c3i.jfrog.io/c3i/misc/logs/pr/14871/15-linux-gcc/sail/0.9.0-rc2//e126bddc6fd00ff8e7c3b656af37d15a09c3c3c8-build.txt

@HappySeaFox
Copy link

HappySeaFox commented Jan 9, 2023

Oh, I added str() and it worked! Thanks 👍

if is_msvc(self):
    tc.cache_variables["CMAKE_TRY_COMPILE_CONFIGURATION"] = str(self.settings.build_type)

@memsharded
Copy link
Member Author

Closing this in favor of #16964 with workaround tc.cache_variables[`CMAKE_TRY_COMPILE_CONFIGURATION`] = self.settings.build_type until then

@memsharded memsharded closed this Oct 18, 2024
@memsharded memsharded deleted the fix/cmakedeps_libraries_not_targets branch October 18, 2024 20:10
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

Successfully merging this pull request may close these issues.

3 participants