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

preparing migration of names to CMakeDeps #8568

Merged
merged 3 commits into from
Mar 4, 2021

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Feb 26, 2021

Changelog: Fix: Allow cmake_find_package_multi and CMakeDeps to be aliases for cpp_info.names and cpp_info.filenames to allow easy migration.
Docs: Omit

cc/ @mpusz

@SSE4
Copy link
Contributor

SSE4 commented Feb 28, 2021

does easy migration worth making this implicit magic (then setting one generator suddenly impacts on another one)?

@memsharded
Copy link
Member Author

does easy migration worth making this implicit magic (then setting one generator suddenly impacts on another one)?

I can try to implement this magic in the CMakeDeps class only, but something is necessary. Otherwise no-one will be able to adopt the new CMakeDeps generator until all recipes in ConanCenter have started to declare it, and it is unlikely that contributors will do these changes if nobody starts using CMakeDeps.

@SSE4
Copy link
Contributor

SSE4 commented Mar 1, 2021

@solvingj WDYT?

my personal opinion: I'd like to avoid hidden magic in 2.0 as much as possible, and prefer explicit approach for everything.
I suppose CMakeDeps is conan 2.0 generator. it was already stated several times 2.0 will be breaking, meaning recipes, scripts, etc. may need to change to adapt for changes in 2.0.
if conan center recipes will need to change, I have no objections at all - they will likely have to change for 2.0 anyway (at least because of new msvc compiler, for instance).

@memsharded
Copy link
Member Author

my personal opinion: I'd like to avoid hidden magic in 2.0 as much as possible, and prefer explicit approach for everything.
I suppose CMakeDeps is conan 2.0 generator. it was already stated several times 2.0 will be breaking, meaning recipes, scripts, etc. may need to change to adapt for changes in 2.0.
if conan center recipes will need to change, I have no objections at all - they will likely have to change for 2.0 anyway (at least because of new msvc compiler, for instance).

The problem is that we need users to try it now. There are users that are already using it (@mpusz, for example), and we are breaking them (in the past version CMakeDeps "name" was cmake_find_package_multi). If we don't do this, users will not be able to test this generator probably in a few months, and we need the feedback now if we want CMakeDeps to be relatively stable when Conan 2.0 is to be released.

I am fine with not mapping CMakeDeps => cmake_find_package_multi, but I strongly believe that at least a mapping cmake_find_package_multi => CMakeDeps is necessary in this release, and until a majority of ConanCenter recipes have started to use cpp_info.names["CMakeDeps"]..... I agree it might be better to hide it in CMakeDeps.

@SSE4
Copy link
Contributor

SSE4 commented Mar 1, 2021

The problem is that we need users to try it now. There are users that are already using it (@mpusz, for example), and we are breaking them (in the past version CMakeDeps "name" was cmake_find_package_multi).

probably, it's fair, the CMakeDeps generator is labelled as experimental, so it's a subject for a breaking changes.

anyway, up to you, my point is just try to move away from implicit magic things in 2.0, instead of adding more of them.

@memsharded
Copy link
Member Author

anyway, up to you, my point is just try to move away from implicit magic things in 2.0, instead of adding more of them.

Totally, this is a temporary thing, to be removed once ConanCenter has catched up.

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add components to the test.


As this magic is going to be removed in Conan 2.0, I think we should condition this to the conan_v2_mode (by default the fallback is activated, in CONAN_V2_MODE it isn't). It would be useful to show a warning (or error) if it is consuming the information from cmake_find_package_multi instead of CMakeDeps. At some point in time in conan-center-index we could force the change by activating something that would make PRs fail.

@memsharded memsharded requested a review from jgsogo March 2, 2021 10:32
@danimtb
Copy link
Member

danimtb commented Mar 3, 2021

I was thinking about a different approach for this. The issue arises when hardcoding generator-related stuff in recipes and then a generator wanting to use that same information despite the recipe not declaring so. This case is the same when using a custom generator (with a name that it is not built-in in Conan).

As a solution, we could provide the information of available generators at package_info() time, so the recipe is able to model that specific information without being so restrictive. For example (just a made-up syntax);

def package_info(self):
   for generator in self.generators.get_family("cmake"):
       self.cpp_info.components["mycomp"].names[generator] = "MySuperPkg1"  # Set nfo for all CMake related generators
    self.cpp_info.components["mycomp"].names["cmake_find_package"] = "MySuperPkg1"  # Override info specific for that generator

Just an idea. But probably something to consider regarding this issue with CMakeDeps generator. Probably defining this kind of things without specifying a generator but instead using "properties" like "cmake_target_names" could also be an alternative so any generator can use it cc/ @solvingj


Anyway, I am not blocking this PR and I feel this is something needed to experiment with the CMakeDeps generator probably. As it is experimental I guess this is something we can play with.

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that Conan v2 should use a different strategy, maybe it is possible to use just cmake
(or the build system name) as the entry in the dictionary and the generators choose the information to consume. Now it's harder because of the cmake generator.

I'm not sure if making it too fine-grained (cmake-targets, cmake-extra, cmake-???) will be needed or not.

@memsharded
Copy link
Member Author

Ok, lets merge it at the moment, and open an issue for discussion.

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.

4 participants