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

[bug] conan 1.42.0: CMakeDeps does not inherit anymore from cmake_find_package_multi names #9943

Closed
SpaceIm opened this issue Nov 4, 2021 · 24 comments
Assignees
Milestone

Comments

@SpaceIm
Copy link
Contributor

SpaceIm commented Nov 4, 2021

Environment Details (include every applicable attribute)

  • Operating System+version: macOS Big Sur
  • Compiler+version: Apple Clang 13
  • Conan version: 1.42.0
  • Python version: 3.9.7

Steps to reproduce (Include if Applicable)

  • create a recipe where self.cpp_info.names["cmake_find_package_multi"] in package_info() has a different name than recipe name (a simple one is utfcpp recipe).
  • consume this recipe in a project and use CMakeDeps generator (you can replace cmake_find_package_multi by CMakeDeps in test_package of utfcpp recipe and run conan create).

=> generated config file and target will have default value (recipe name), so find_package() and target_link_libraries() might fail.

It has been seen in conan-io/conan-center-index#7887

Workaround: migrate to set_property() (which has an unwanted side effect in CONAN_PKG targets of cmake generator: conan-io/conan-center-index#7925, so the combination of those 2 issues is really annoying).

@memsharded
Copy link
Member

memsharded commented Nov 4, 2021

Hi @SpaceIm

yes, this is known, not a bug. CMakeDeps will not follow any of the legacy methods, not the .names field, not the cmake_find_package_multi one. It will only use the new set_property() mechanism, this is necessary to move forward.

Adding the properties in recipes is the way to go.
If the set_property is breaking cmake generator, then we should have a look at this and try to fix it.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Nov 4, 2021

I understand, but to move forward in CCI recipes, I would have expected:

  • a new CCI hook preventing to use .names
  • CMakeDeps compatibility with .names["cmake_find_package_multi"] until conan v2 to ensure smooth and transparent transition of consumers to CMakeDeps generator (it will take weeks or months to update CCI recipes).

@memsharded
Copy link
Member

CMakeDeps compatibility with .names["cmake_find_package_multi"] until conan v2 to ensure smooth and transparent transition of consumers to CMakeDeps generator (it will take weeks or months to update CCI recipes).

Yes, we tried so far, but the issue is that CMakeDeps have moved internally to use all the new dependencies and new CppInfo model, that no longer has names field, it is simply not there (and can't be easily introduced without extending the legacy into 2.0, which is what is not desired).

The CCI hook was planned, but we haven't had the time to do it.

So I think we should focus on keeping the cmake generator working as expected, lets see what is necessary, trying to summarize: From conan-io/conan-center-index#7925, it seems that the expected behavior of the cmake generator is to respect the name of the package? Always? target_link_libraries(test PUBLIC CONAN_PKG::ms-gsl). If that is the case, I guess this could be an easy fix?

@Minimonium
Copy link
Contributor

The issue is that cmake generator uses cmake_target_name as a name for a target, which, in the new properties and find_package generator, correlate to the namespace. Breaking changes when using find_package generator are somewhat expected because the intent is to emulate the "native" CMake installation. Breaking changes when using cmake generator are not expected.

Another issue tho if that if you change the generator now - it'd also break users who already fixed their build scripts. And cmake generator is deprecated. But if we keep migrating to properties in CCI - more users who use cmake generator would break. :)

@KerstinKeller
Copy link

What can we do, if we are using recipes from CCI in conjuction with CMakeDeps?
I have noticed the problem with the qt package (previously generated Qt5Config.make, now generates qt-config.cmake.
It's also the same with CURL / libCurl and a few more.
The other thing is that now the generated files have - instead of Camelcase.

What exactly is required to patch the current recipes in CCI?

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Nov 5, 2021

What exactly is required to patch the current recipes in CCI?

move to set_property() instead of .names in CCI recipes. But there are many recipes to upgrade, so it will prevent adoption of CMakeDeps in conan users since most of generated config files are now broken with this generator (if conan >= 1.42).

@eigenwhat
Copy link

What can we do, if we are using recipes from CCI in conjuction with CMakeDeps? I have noticed the problem with the qt package (previously generated Qt5Config.make, now generates qt-config.cmake. It's also the same with CURL / libCurl and a few more. The other thing is that now the generated files have - instead of Camelcase.

Producing the - version of the config files (e.g. catch2-config.cmake vs Catch2Config.cmake) does have a slight benefit in that find_package() effectively works case-insensitively. find_package(XyZ CONFIG) will first look for a case-matching XyZConfig.cmake, and failing that, looks for the fully lowercase xyz-config.cmake. The upshot is that with the former you must write find_package(XyZ CONFIG) but with the latter find_package(XYZ CONFIG), find_package(Xyz CONFIG), find_package(xyz CONFIG), etc. all work.


I did run into the parent issue with Catch2 where it errors out when using CMakeDeps because its output creates the catch2::catch2 target instead of the expected Catch2::Catch2. I suppose the only recourse is to wait for the recipes to update accordingly?

@SSE4
Copy link
Contributor

SSE4 commented Nov 11, 2021

Producing the - version of the config files (e.g. catch2-config.cmake vs Catch2Config.cmake) does have a slight benefit in that find_package() effectively works case-insensitively.

not exactly. it uses a simple search within file system under the hood, and behavior depends whether or not your file system is case-sensitive itself.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Nov 11, 2021

Producing the - version of the config files (e.g. catch2-config.cmake vs Catch2Config.cmake) does have a slight benefit in that find_package() effectively works case-insensitively.

not exactly. it uses a simple search within file system under the hood, and behavior depends whether or not your file system is case-sensitive itself.

I've not tested again recently, but I'm pretty sure it's no true: #8048 (comment)

@SSE4
Copy link
Contributor

SSE4 commented Nov 11, 2021

I've not tested again recently, but I'm pretty sure it's no true: #8048 (comment)

I've just did a quick test on Windows vs Linux, and behavior is different:
CMakeLists.txt

cmake_minimum_required(VERSION 2.8)
project(xxx)
find_package(XYZ CONFIG REQUIRED)
find_package(Xyz CONFIG REQUIRED)
find_package(xyz CONFIG REQUIRED)

then run:

$ touch XYZConfig.cmake
$ cmake . -DCMAKE_PREFIX_PATH=.

on Windows it configures successfully, while on Linux it fails:

CMake Error at //CMakeLists.txt:4 (find_package):
  Could not find a package configuration file provided by "Xyz" with any of
  the following names:

    XyzConfig.cmake
    xyz-config.cmake

  Add the installation prefix of "Xyz" to CMAKE_PREFIX_PATH or set "Xyz_DIR"
  to a directory containing one of the above files.  If "Xyz" provides a
  separate development package or SDK, be sure it has been installed.

so it definetely uses a file system search.
same check if you do instead:

$ touch XYZ-config.cmake

also fails on Linux and passes on Windows. so it definetely depends on the file system in use.

@KerstinKeller
Copy link

I guess what @SpaceIm means is that if you lowercase the config file

touch xyz-config.cmake

then all of

cmake_minimum_required(VERSION 2.8)
project(xxx)
find_package(XYZ CONFIG REQUIRED)
find_package(Xyz CONFIG REQUIRED)
find_package(xyz CONFIG REQUIRED)

should now work on all platforms.

@SSE4
Copy link
Contributor

SSE4 commented Nov 11, 2021

I guess what @SpaceIm means is that if you lowercase the config file

touch xyz-config.cmake

then all of

cmake_minimum_required(VERSION 2.8)
project(xxx)
find_package(XYZ CONFIG REQUIRED)
find_package(Xyz CONFIG REQUIRED)
find_package(xyz CONFIG REQUIRED)

should now work on all platforms.

yes, that's the only case it works portable on all platforms.
my comment was about this statement:

does have a slight benefit in that find_package() effectively works case-insensitively.

however it doesn't work case-insensitively on all platforms, it requires a config file to be lower-case in order to be portable.

@eigenwhat
Copy link

eigenwhat commented Nov 16, 2021

Good bloody grief, I meant that the package name parameter passed into find_package() works case-insensitively, not that suddenly everything related to find_package() won't care about capitalization and that you can spell the filename in mixed case and have it work just because you include a dash. I thought it went without saying that the - version means the all-lowercase form, seeing as that's how it's specified by CMake. The allowable spellings of the config file is well-defined by CMake as being either "a file called <PackageName>Config.cmake or <lower-case-package-name>-config.cmake". I figured that this plus the fact that the whole rest of the comment elaborates on that with examples would make it abundantly clear I was talking about input parameters, not the file name, so I'm more than a little annoyed we all spent all this time going back and forth about one sentence taken aggressively out of context. (What is this, Reddit? 😉) Maybe next time we should ask for clarification instead? 🙂

Anyhow, @SpaceIm was right on. I was trying to echo the ideas from #8048 here because it's related. The same benefit in using the <lower-case-package-name>-config.cmake applies here as well. The comment I was replying to seemed to imply that using the <lower-case-package-name>-config.cmake instead of <PackageName>Config.cmake was a regression as well. I was trying to say that using this - version has the benefit that the package name input to find_package() effectively works case-insensitively, which is rather handy in various scenarios. It sounds to me like the only thing that needs to be done is to update recipes to use set_property(), but since this issue is still open, please please please don't "fix" CMakeDeps to use <PackageName>Config.cmake unless you explicitly want the more limited behavior. That's all.

@memsharded
Copy link
Member

This is the current logic in CMakeDeps:

@property
    def filename(self):
        if self.find_module_mode:
            return "Find{}.cmake".format(self.file_name)
        else:
            if self.file_name == self.file_name.lower():
                return "{}-config.cmake".format(self.file_name)
            else:
                return "{}Config.cmake".format(self.file_name)

We don't plan to change this, and we think the solution is to update recipes to add set_property(), aligned with your feedback @eigenwhat. Still, if a recipe adds a property cmake_file_name=Something that contains upper case, the filename will be SomethingConfig.cmake. I hope this is still ok, right?

@eigenwhat
Copy link

Sounds reasonable to me.

@memsharded
Copy link
Member

I think this can be closed, it is expected that CMakeDeps is not using those names anymore, and there are other initiatives (updating recipes in ConanCenter with newer properties) to fix this. But please tell if I am missing something. Thanks.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Nov 23, 2021

@memsharded

So how to offer stable interface for consumers of CCI during this transition? I believe that breaking CMake imported targets and config files is not acceptable. Transition from cmake_find_package to CMakeDeps, and from conan v1 to conan v2 should be as transparent as possible for consumers of CCI.

@memsharded
Copy link
Member

Hi @SpaceIm
I am not sure what you mean:

  • The cmake and cmake_multi generators use Conan targets CONAN_PKG:: Those are no issue, as they always use the package name, this has been fixed after some regression
  • The cmake_find_package and cmake_find_package_multi generators will follow correctly the defined properties (and also fallback to .names to not break in the transition
  • CMakeDeps is the experimental one, that will require to have the new properties defined, if they are not, the targets in CMakeDeps will be the default one, but that shouldn't be an issue, because CMakeDeps is experimental, isn't it?

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Nov 24, 2021

Ok. Therefore I believe that:

  • CMakeDeps shouldn't be promoted by conan team (in tutorials, and Slack/reddit etc) until CCI recipes properly support it. CMakeToolchain + cmake_find_package* is the way to go currently.
  • Transition to set_property() in CCI (and therefore proper support of CMakeDeps) should begin when set_property() is stable enough. CMakeDeps shouldn't be recommended for consumers until the end of this transition.

@memsharded
Copy link
Member

CMakeDeps shouldn't be promoted by conan team (in tutorials, and Slack/reddit etc) until CCI recipes properly support it. CMakeToolchain + cmake_find_package* is the way to go currently.

There is way more Conan usage out there not using ConanCenter than using it. The estimation is around 70-80% of users do not depend on ConanCenter, but maintain their own recipes (forks or from scratch) for third party packages. I know a few teams that are already happily migrating to CMakeDeps (and not being affected by these issues)
Given that we are already in Conan 2.0-alpha, that only implements CMakeDeps, it is very necessary to promote it to start getting quality feedback and stabilizing it. It is a chicken and egg problem, it is likely that we will have a reasonably stable CMakeDeps to start being adopted in ConanCenter, thanks to all the people testing it, adopting and giving feedback so far.

I am totally fine, and I agree that maybe we should wait in ConanCenter to adopt CMakeDeps until the properties are migrated and stabilized.

@db4
Copy link
Contributor

db4 commented Nov 25, 2021

There is way more Conan usage out there not using ConanCenter than using it. The estimation is around 70-80% of users do not depend on ConanCenter, but maintain their own recipes (forks or from scratch) for third party packages

Let's take Boost as an example. I don't believe there are 70% serious C++ software developers not ever using it or recreated its recipe from scratch. If they're so unlucky to adopt CMakeDeps in their projects (like me) - they're toast now. Yes, I forked Boost recipe and fixed it internally but it's big pain, Boost is not the only one. Your argument that CMakeDeps is experimental does not quite convince me: it's you who recommended it for new projects as other CMake generators will disappear in the near future: #9108 (comment) Yes, it's experimental but there are many features that are still experimental like python requires. components or lock files but without them one just can't use Conan nowadays. And a new Conan release that just makes such a feature completely unusable in many existing recipes without deprecation notice is IMHO unacceptable.

@memsharded
Copy link
Member

I understand this is unfortunate, but in our defense, most of this came from a new requisite, that didn't exist before when CMakeDeps was starting to be promoted, and it is the need to define new ways of defining specific target names (with custom namespaces, etc). We couldn't foresee that, and the path forward in the migration is now more painful than expected. I really apologize for it. But also this is the case where the experimental qualifier kicks in. The fact that we have been doing a huge effort in not breaking other features labeled as experimental doesn't mean that at some point we would need to break one of them. Otherwise we would always be stuck in a very bad situation: nobody ever uses and tests new features, so they are never improved, so they keep experimental forever.

Said that, I totally understand the pain, so lets try to alleviate it as fast as possible:

  • Lets stop promoting CMakeDeps until the properties are stabilized
  • Lets stabilize the final properties in 1.43
  • Lets try to put extra resources as soon as 1.43 is available in conan-center-index to update recipes to use those final properties.

Thanks very much for the feedback.

@KerstinKeller
Copy link

@memsharded I think what you proposed sounds like a decent plan.
For the build of our open source project we will just stay on Conan 1.41 for now. If properties are stabilized in 1.43, I am willing to make PR's for the packages that are relevant for us.

So I am understanding that .set_property and .names can coextist during the transition to Conan 2.0, and then at some point in the future all the .names can be removed from the recipes?

CMakeDeps is great, as I can also, on recipe level, turn off generation of individual Config files 😄

@memsharded
Copy link
Member

#10077 (comment) contains some updates about this. Quick summary:

  • We will keep the .names longer (2.X), to ensure not breaking cmake_find_package[multi] generators. These generators will not listen to the new properties (we will revert back .names in ConanCenter, before 1.43 is out)
  • New properties will define only for CMakeDeps, this is the way we can deliver new features without breaking

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

No branches or pull requests

9 participants