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

CMakeDeps checks build profile + CMakeDeps check real targets #9206

Merged
merged 6 commits into from
Jul 7, 2021

Conversation

lasote
Copy link
Contributor

@lasote lasote commented Jul 5, 2021

Changelog: Fix: Added warning in the new toolchains (the used in the generate() method) if no build profile is being used.
Changelog: Fix: Implemented check that will raise an error in the CMakeDeps generator when using the build_context_activated, build_context_suffix or build_context_build_modules attributes if no build profile is being used.
Changelog: Fix: TheCMakeDeps generator will check if the targets specified in the find_package(foo components x y z) exist instead of checking against an internal variable. Also, this check will be done at the end of the xxx-config.cmake so any included build_module can declare the needed targets.
Docs: conan-io/docs#2173
For documentation: (Modify the documentation with a note indicating that the usage of build_context_activated, build_context_suffix and build_context_build_modules will fail if no build profile is specified. Also a note in every new generator that the usage of the build profile is very recommended or unexpected errors/behavior may ocurr)

Closes #9112

@lasote lasote requested review from memsharded and danimtb July 5, 2021 15:47
@lasote lasote added this to the 1.39 milestone Jul 5, 2021
@danimtb
Copy link
Member

danimtb commented Jul 6, 2021

Just missing a test here that shows the expected behavior for components coming from a build module. I think it is important to have this covered by a test

@lasote
Copy link
Contributor Author

lasote commented Jul 6, 2021

@danimtb Added test, please review.

@cboos
Copy link

cboos commented Jul 7, 2021

Hello, I'm commenting on:

Changelog: Fix: TheCMakeDeps generator will check if the targets specified in the find_package(foo components x y z) exist instead of checking against an internal variable. Also, this check will be done at the end of the xxx-config.cmake so any included build_module can declare the needed targets.

This is indeed a step further to fix the "regression" from 1.36 for my Qt5 use case (cf. #9112 comment), but the explicit check for defined components won't work in this situation.

e.g. if we have in our CMakeLists.cmake something like:

find_package(Qt5 COMPONENTS Core LinguistTools REQUIRED)

then the test in Conan's Qt5Config.cmake passes for Core but not for LinguistTools:

...
  Paths specified by the find_package PATHS option.

    C:/Users/cboos/.conan/data/Qt5/5.15.2.0/bct/testing/package/811010fc454bb85ee1e902a530feb1ec906ff80f/lib/cmake



  find_package considered the following locations for the Config module:

    C:/Users/cboos/.conan/data/Qt5/5.15.2.0/bct/testing/package/811010fc454bb85ee1e902a530feb1ec906ff80f/lib/cmake/Qt5LinguistToolsConfig.cmake
    C:/Users/cboos/.conan/data/Qt5/5.15.2.0/bct/testing/package/811010fc454bb85ee1e902a530feb1ec906ff80f/lib/cmake/qt5linguisttools-config.cmake
    C:/Users/cboos/.conan/data/Qt5/5.15.2.0/bct/testing/package/811010fc454bb85ee1e902a530feb1ec906ff80f/lib/cmake/Qt5LinguistTools/Qt5LinguistToolsConfig.cmake

  The file was found at

    C:/Users/cboos/.conan/data/Qt5/5.15.2.0/bct/testing/package/811010fc454bb85ee1e902a530feb1ec906ff80f/lib/cmake/Qt5LinguistTools/Qt5LinguistToolsConfig.cmake

Call Stack (most recent call first):
  build/Qt5Config.cmake:21 (include)
  applications/bct_ico_sync/CMakeLists.txt:7 (find_package)


-- Conan: Component 'Core' found in package 'Qt5'
CMake Error at build/cmakedeps_macros.cmake:4 (message):
  Conan: Component 'LinguistTools' NOT found in package 'Qt5'
Call Stack (most recent call first):
  build/Qt5Config.cmake:31 (conan_message)
  applications/bct_ico_sync/CMakeLists.txt:7 (find_package)


CMake Debug Log at applications/bct_ico_sync/CMakeLists.txt:7 (find_package):
    D:/Workspace/src/git/BCT/main/build/Qt5Config.cmake

This is because the Qt5LinguistToolsConfig.cmake (as shipped in Qt 5.15.2) provides targets like Qt5::lrelease, Qt5::lupdate, but no Qt5::LinguistTools target. I'm not sure how legit that is from a CMake point of view, but it certainly works well in practice. However, Conan assumes that for each ${Qt5_FIND_COMPONENTS} there is a Qt5::${_FIND_COMPONENT} target defined, and that's not the case with this "virtual" component.

@cboos
Copy link

cboos commented Jul 7, 2021

However, Conan assumes that for each ${Qt5_FIND_COMPONENTS} there is a Qt5::${_FIND_COMPONENT} target defined, and that's not the case with this "virtual" component.

... following-up with a suggestion: would it be possible to make that explicit check optional? (the whole # Check that the specified components in the find_package(Foo COMPONENTS x y z) are there part).

An invalid component name in a find_package will lead to an error anyway:

find_package(Qt5 COMPONENTS Core LinguistToolZ REQUIRED)
-- Conan: Target declared 'Qt5::Qt5'
-- Conan: Including build module from 'C:/Users/cboos/.conan/data/Qt5/5.15.2.0/bct/testing/package/811010fc454bb85ee1e902a530feb1ec906ff80f/lib/cmake/Qt5/Qt5Config.cmake'
-- Conan: Target declared 'Qt5::Qt5'
-- Conan: Including build module from 'C:/Users/cboos/.conan/data/Qt5/5.15.2.0/bct/testing/package/811010fc454bb85ee1e902a530feb1ec906ff80f/lib/cmake/Qt5/Qt5Config.cmake'
CMake Error at C:/Users/cboos/.conan/data/Qt5/5.15.2.0/bct/testing/package/811010fc454bb85ee1e902a530feb1ec906ff80f/lib/cmake/Qt5/Qt5Config.cmake:28 (find_package):
  Could not find a package configuration file provided by "Qt5LinguistToolZ"
  with any of the following names:

    Qt5LinguistToolZConfig.cmake
    qt5linguisttoolz-config.cmake

Something like: self.cpp_info.set_property('cmake_check_components', false) (or always leave it out and rely on cmake to produce an error when needed - well, in the regular situation, when the only package .cmake files are the ones generated by Conan, I understand this must be there).

@lasote
Copy link
Contributor Author

lasote commented Jul 7, 2021

Hi @cboos. Sorry, I don't get it. If you use find_package(Qt5 COMPONENTS Core LinguistToolZ REQUIRED) it will make CMake fail because it is what REQUIRED is intended, right? We cannot avoid that from Conan.
It would be different if you use don't specify the REQUIRED: find_package(Qt5 COMPONENTS Core LinguistToolZ) in that case will be Conan the one failing, right?.
We could consider avoiding the check but the correct way to do it would be in the generate() method of the consumer, it could be something like:

def generate(self):
    deps = CMakeDeps(self)
    deps.skip_components_check = True
    deps.generate()

(and remove the generators = "CMakeDeps")

@lasote
Copy link
Contributor Author

lasote commented Jul 7, 2021

Edit: I have checked that REQUIRED is not affecting when a component is missing, that's why we introduce that check.

@cboos
Copy link

cboos commented Jul 7, 2021

the correct way to do it would be in the generate() method of the consumer

I did hope it would be possible to do that at the level of my Qt5 Conan package itself, so that I can keep using simple consumers based on a conanfile.txt and [generators] CMakeDeps. Well, I suppose it might be possible to support generator options there at some point (CMakeDeps [skip_components_check=True]).

But still, that would then apply to all the dependencies, not just to that Qt5 one, and it's possibly the only one which wouldn't need the components check. So I see that more as a "property" of that package itself rather than a constraint to spread on its consumers, possibly affecting other packages.

@lasote
Copy link
Contributor Author

lasote commented Jul 7, 2021

After checking that CMake is not failing by default when a component specified in the find_package doesn't exist we changed our default behavior and we are not checking anymore the components existence unless it is explicitly activated with deps.check_components_exist=True

@cboos
Copy link

cboos commented Jul 7, 2021

I just checked 468b68e and with that, I can use again my Qt5 package again! So thanks a lot! 👍

The point I raised above that it could be more appropriate to decide to check for components or not (so check_components_exist now) at the level of a given package rather than for all dependencies of a consumer might still be a valid concern. But as the default behavior fits my use case, I won't insist on that ;-)

@memsharded memsharded merged commit 38bf9b7 into conan-io:develop Jul 7, 2021
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.

[bug] Conan 1.37.2 CMakeDeps generator does not bootstrap package's own CMake config file
4 participants