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

Feature: add property "cmake_target_aliases" to create some CMake alias targets (CMakeDeps) #8533

Merged
merged 9 commits into from
Oct 21, 2021

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Feb 19, 2021

Changelog: Feature: Add property "cmake_target_aliases" to create some CMake alias targets using CMakeDeps.
Docs: omit

closes: #8360

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@SSE4
Copy link
Contributor Author

SSE4 commented Mar 17, 2021

if someone has better ideas for UX, please share. right now it requires modifications in two methods: package and package_info:

                def package(self):
                    tools.create_cmake_module_alias_targets(self, self._module,
                        {"hello": "hello::hello"})

                def package_info(self):
                    self.cpp_info.builddirs = [os.path.dirname(self._module)]
                    self.cpp_info.build_modules = [self._module]

it would be nice, if one tool invocation could set everything needed (e.g. builddirs and build_modules), but:

  • CppInfo cannot be set from the package
  • build_modules requires existing file that has to be packaged on the package creation

I am not sure if there is a nice way to break a vicious circle, and reduce the chance of mistake and amount of code needed.

@uilianries
Copy link
Member

Up!

@danimtb
Copy link
Member

danimtb commented Jul 23, 2021

Would like to know the feedback from @memsharded and @lasote about this issue.

I am not sure, but I believe this use case is already considered in the new cpp_info model and doing this will not be necessary.
If that is correct, introducing this tool in current recipes will be another extra thing to migrate when we introduce conan 2.0 in ConanCenter. I think we should think about that as well as we want the recipes to be as compatible as possible.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Please try a different approach:

  • Define a cpp_info.set_property() that defines this data
  • Have the CMakeDeps generator to honor this property and generate the necessary alias or cmake code to implement this
  • Recipes code should be just 1 line with the set_property definition

@SSE4
Copy link
Contributor Author

SSE4 commented Sep 8, 2021

@memsharded I've spent some type to prototype something with better UX via set_property as you have suggested.
now it requires just a single line from the recipe author.
for global alias:

def package_info(self):
    self.cpp_info.set_property("aliases", ["hello"], "cmake_find_package_multi")

or for components alias:

def package_info(self):
    self.cpp_info.components["adios"].set_property("aliases", {"hola::adios": "hello::adios"}, "cmake_find_package_multi")

pros:

  • simple UX: requires just 1 intuitive line
  • no need to package a build module
  • the author may define as many aliases as he wants
  • the author may use arbitrary namespaces (or no namespace at all)
  • and even different namespaces per target

we may add custom CMake variables a similar way

@prince-chrismc
Copy link
Contributor

I am sold on the idea!

@prince-chrismc
Copy link
Contributor

👋 @SpaceIm @madebr (great feature to review)

Copy link
Contributor

@lasote lasote left a comment

Choose a reason for hiding this comment

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

Looks nice! but we cannot implement it in the cmake_find_package_multi and not in the CMakeDeps. It should be considered "the king" and "the king" goes first.

@SSE4
Copy link
Contributor Author

SSE4 commented Sep 10, 2021

Looks nice! but we cannot implement it in the cmake_find_package_multi and not in the CMakeDeps. It should be considered "the king" and "the king" goes first.

sure, I can implement it in CMakeDeps as well if overall UX looks good. pinging @memsharded for the feedback :)

@SSE4 SSE4 changed the title Feature: add tools. create_cmake_module_alias_targets Feature: create alias targets Sep 12, 2021
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

I would like this feature only in CMakeDeps.

It doesn't make much sense to provide it in the legacy, have some recipes update to support this in cmake_find_package_xxx, then remove that generator for CMakeDeps.

@SSE4 SSE4 force-pushed the create_module_official_cmake_targets branch from 2545236 to 03d110c Compare October 3, 2021 07:50
@SSE4
Copy link
Contributor Author

SSE4 commented Oct 3, 2021

@memsharded I finally had some time to revisit it, now I've changed code to your suggestions:

  • only CMakeDeps generator is supported
  • property name is cmake_target_aliases (so it indicates it is specific for the CMake only)

and a new UX, for global aliases:

self.cpp_info.set_property("cmake_target_aliases", {"hello": "hello::hello"}, "CMakeDeps")

for component aliases:

self.cpp_info.components["buy"].set_property("cmake_target_aliases",  {"hola::adios": "hello::buy"}, "CMakeDeps")

and I think only dict values make sense, as they are explicit and straightforward - alias target is always specified.
so now you can create as many arbitrary aliases as you want, for any targets, with or without namespaces.
I think it should cover 99% of use-cases in conan-center.

@madebr
Copy link
Contributor

madebr commented Oct 3, 2021

If this property should supposedly be used for each component to create an alias, why does it need to be a dict?
If I want to add an alias to a component, then Conan already knows the name of the original target?

self.cpp_info.set_property("cmake_target_aliases", ["alias1", "alias2::alias2"])

I also find having 2 properties for global aliases and component aliases potentially confusing.
Is it possible to have one property cmake_target_aliases?
From a recipe writer perspective, they shouldn't care about this. A cmake alias is a cmake alias.

@SSE4
Copy link
Contributor Author

SSE4 commented Oct 3, 2021

If this property should supposedly be used for each component to create an alias, why does it need to be a dict?

if you ever want to create an alias to the target other than the current component? maybe it's useless, in that case, array would be enough.

If I want to add an alias to a component, then Conan already knows the name of the original target?

yes, it knows the name of the original target. I am not sure if this extra flexibility is needed.

I also find having 2 properties for global aliases and component aliases potentially confusing. Is it possible to have one property cmake_target_aliases?

it is possible, but I think there are recipes that don't define any components, and there are recipes using components.

@madebr
Copy link
Contributor

madebr commented Oct 3, 2021

I also find having 2 properties for global aliases and component aliases potentially confusing. Is it possible to have one property cmake_target_aliases?

it is possible, but I think there are recipes that don't define any components, and there are recipes using components.

I don't know the Conan implementation, but can't it detect it from the context?

self.cpp_info.set_property("cmake_target_aliases", ["alias1", "alias2::alias2"])

versus

self.cpp_info.modules["comp"].set_property("cmake_target_aliases", ["alias1", "alias2::alias2"])

@madebr
Copy link
Contributor

madebr commented Oct 3, 2021

If this property should supposedly be used for each component to create an alias, why does it need to be a dict?

if you ever want to create an alias to the target other than the current component? maybe it's useless, in that case, array would be enough.

The only use I can think of would be to create an alias of a target of a dependency. But I haven't yet seen a need for this.

@SSE4
Copy link
Contributor Author

SSE4 commented Oct 3, 2021

@madebr I've added an automatic deduction of an alias target, now the UX is:

self.cpp_info.set_property("cmake_target_aliases", ["hello"], "CMakeDeps")
self.cpp_info.components["buy"].set_property("cmake_target_aliases",  ["hola::adios"] "CMakeDeps")

@lasote lasote added this to the 1.42 milestone Oct 4, 2021
Copy link
Contributor

@lasote lasote left a comment

Choose a reason for hiding this comment

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

I think this is good. I was not convinced about the alias, not distingüising the namespace, but now I see is simpler this way.

@SSE4 SSE4 force-pushed the create_module_official_cmake_targets branch from 1e1a966 to 16556f3 Compare October 20, 2021 14:25
@czoido czoido merged commit f7cfc22 into conan-io:develop Oct 21, 2021
@SSE4 SSE4 changed the title Feature: create alias targets Feature: add property "cmake_target_aliases" to create some CMake alias targets (CMakeDeps) Oct 21, 2021
memsharded pushed a commit to memsharded/conan that referenced this pull request Oct 22, 2021
* - CMakeDeps: add cmake_target_aliases property

Signed-off-by: SSE4 <[email protected]>

* - automatically deduce an alias target

Signed-off-by: SSE4 <[email protected]>

* Update conans/test/functional/toolchains/cmake/cmakedeps/test_cmakedeps_aliases.py

Co-authored-by: Luis Martinez de Bartolome Izquierdo <[email protected]>

* Update conans/test/functional/toolchains/cmake/cmakedeps/test_cmakedeps_aliases.py

Co-authored-by: Luis Martinez de Bartolome Izquierdo <[email protected]>

* - add comment

Signed-off-by: SSE4 <[email protected]>

* - use cpp_info instead of new_cpp_info

Signed-off-by: SSE4 <[email protected]>

* - remove useless checks

Signed-off-by: SSE4 <[email protected]>

* - show a warning if target already exists

Signed-off-by: SSE4 <[email protected]>

* - test case for cmake_target_name/cmake_target_namespace

Signed-off-by: SSE4 <[email protected]>

Co-authored-by: Luis Martinez de Bartolome Izquierdo <[email protected]>
memsharded added a commit that referenced this pull request Oct 25, 2021
…st profile and win_bash (#9755)

* tests that prove bad behavior

* wip

* workaround for win_bash for .bat files

* wip

* wip

* removed printing traceback

* fixing tests

* fixing tests

* fix test

* try fix py27

* wip

* wip

* Update conan/tools/env/environment.py

Co-authored-by: Luis Martinez de Bartolome Izquierdo <[email protected]>

* Update conan/tools/microsoft/subsystems.py

Co-authored-by: Luis Martinez de Bartolome Izquierdo <[email protected]>

* Fix unsafe check of the compiler in the CMakeToolchain generator (#9801)

The compiler variable here must not be a NoneType object to use `in`.
This will result in an error when attempting to package a header-only library.
This commit fixes this check.
It first verifies the compiler is not None before checking it.

* Fix typo where fPIC option is not templated, so it is always enabled (#9752)

* Fix typo where fPIC option is not templated, so it is always enabled

* Fix typo where fPIC option is not templated, so it is always enabled

* Add name to contributors

* Don't add fPIC functionality when it is not set

* Add unit tests

* Remove extra newlines

* Use "ON" and "OFF" for fPIC value in CMake

* Fix typos

* Correctly setup options for ConanFile Mocks

* Add missing fixture annotations

* Initialize options and default options dictionaries and fix Windows test

* Properly check if the OS is Windows

* Fix more typos

* Make sure to set OS as Windows for unit test ConanFiles

* Revert "Properly check if the OS is Windows"

This reverts commit 7eebbde.

* rename ConanFileInterface.new_cpp_info (#9800)

* rename ConanFileInterface.new_cpp_info

* wip

* wip

* fix policy=never (#9817)

* Feature: create alias targets (#8533)

* - CMakeDeps: add cmake_target_aliases property

Signed-off-by: SSE4 <[email protected]>

* - automatically deduce an alias target

Signed-off-by: SSE4 <[email protected]>

* Update conans/test/functional/toolchains/cmake/cmakedeps/test_cmakedeps_aliases.py

Co-authored-by: Luis Martinez de Bartolome Izquierdo <[email protected]>

* Update conans/test/functional/toolchains/cmake/cmakedeps/test_cmakedeps_aliases.py

Co-authored-by: Luis Martinez de Bartolome Izquierdo <[email protected]>

* - add comment

Signed-off-by: SSE4 <[email protected]>

* - use cpp_info instead of new_cpp_info

Signed-off-by: SSE4 <[email protected]>

* - remove useless checks

Signed-off-by: SSE4 <[email protected]>

* - show a warning if target already exists

Signed-off-by: SSE4 <[email protected]>

* - test case for cmake_target_name/cmake_target_namespace

Signed-off-by: SSE4 <[email protected]>

Co-authored-by: Luis Martinez de Bartolome Izquierdo <[email protected]>

* Fix pkgconfig tests for Windows (#9833)

* Fixed all the tests in Windows

* Added mingw32-g++ tool for Windows

* Get rid of mingw32-g++ specific tool

* Changed appending symbol to overwriting one

* renaming to scope

* more scopes

* fix test

* Update conans/test/integration/toolchains/env/test_virtualenv_winbash.py

Co-authored-by: Carlos Zoido <[email protected]>

Co-authored-by: Luis Martinez de Bartolome Izquierdo <[email protected]>
Co-authored-by: Jordan Williams <[email protected]>
Co-authored-by: SSE4 <[email protected]>
Co-authored-by: Francisco Ramírez <[email protected]>
Co-authored-by: Carlos Zoido <[email protected]>
@franramirez688
Copy link
Contributor

@SSE4 is there any kind of documentation for this feature? I think it could be interesting to have a little explanation there, couldn't it?

@SpaceIm
Copy link
Contributor

SpaceIm commented Nov 16, 2021

I would like this feature only in CMakeDeps.

It doesn't make much sense to provide it in the legacy, have some recipes update to support this in cmake_find_package_xxx, then remove that generator for CMakeDeps.

@memsharded Why? It prevents adoption of this new property in conan-center until conan v2, because it would break many downstream recipes or users (we can't know which generator is used by downstream). With this strategy, we have to keep ugly workarounds in CCI.

@SpaceIm
Copy link
Contributor

SpaceIm commented Nov 17, 2021

I have also another comment, slightly off-topic:

If cmake_target_name property was designed to generate exactly the target name (and not automatically adding a namespace), cmake_target_aliases and cmake_target_namespace properties wouldn't have been required. It would also have solved some issues of conflicting global targets between recipes (Vulkan & mongo recipes for example).

@SSE4
Copy link
Contributor Author

SSE4 commented Nov 17, 2021

If cmake_target_name property was designed to generate exactly the target name (and not automatically adding a namespace), cmake_target_aliases and cmake_target_namespace properties wouldn't have been required. It would also have solved some issues of conflicting global targets between recipes (Vulkan & mongo recipes for example).

my two cents, just a personal opinion
there are different use-cases for that properties:

  1. cmake_target_name is for declaring a canonical target name, if it's different from the default (which is package name::package name). for instance, if I have a package named sqlite3 but I want a canonical target named sqlite::sqlite (without version 3), then I would use cmake_target_name.
  2. cmake_target_namespace is just to control over namespace (in reality, there are no namespaces in CMake, it's just an integral part of the name). we already faced libraries that use different namespaces for different targets, for instance OpenEXR may use something like Imath::Imath and OpenEXR::Iex together.
  3. cmake_target_alias is if you want to declare more than one name for the same target. e.g. for sqlite3, you may want to define both sqlite::sqlite and sqlite (without namespace), because both are provided by the upstream. in that case, there are multiple canonical names for the same target.

so cmake_target_name is mostly for single target configuration, cmake_target_namespace is useful when you have multiple targets with different namespaces, and cmake_target_alias is useful then you have multiple canonical names for the same target. of course, there is an overlap between them, but ideologically they all serve different purposes.

@SpaceIm
Copy link
Contributor

SpaceIm commented Nov 17, 2021

cmake_target_namespace is just to control over namespace (in reality, there are no namespaces in CMake, it's just an integral part of the name). we already faced libraries that use different namespaces for different targets, for instance OpenEXR may use something like Imath::Imath and OpenEXR::Iex together.

From #10005 (comment), I understand that cmake_target_namespace doesn't work at component level. So you can't define both Imath::Imath and OpenEXR::Iex in the same recipe with this property. cmake_target_alias is needed here.
But I say that this complexity could have been avoided, to allow to define exactly the relevant targets, without intermediate useless targets which could conflict with targets of others recipes.

@SSE4
Copy link
Contributor Author

SSE4 commented Nov 17, 2021

cmake_target_namespace is just to control over namespace (in reality, there are no namespaces in CMake, it's just an integral part of the name). we already faced libraries that use different namespaces for different targets, for instance OpenEXR may use something like Imath::Imath and OpenEXR::Iex together.

From #10005 (comment), I understand that cmake_target_namespace doesn't work at component level. So you can't define both Imath::Imath and OpenEXR::Iex in the same recipe with this property. cmake_target_alias is needed here. But I say that this complexity could have been avoided, to allow to define exactly the relevant targets, without intermediate useless targets which could conflict with targets of others recipes.

I totally agree, we shouldn't pollute the CMake scope with targets no one needs.
I personally would love to see cmake_target_namespace implemented on a component level, I don't get the reason why it can't be done that way.
IMO you should be able to declare both Imath::Imath and OpenEXR::Iex without any intermediate targets.
as it's experimental, I think it's not late to change and avoid unnecessary complexity, if any.

@SSE4
Copy link
Contributor Author

SSE4 commented Nov 18, 2021

@SpaceIm we have discussed an issue internally, and have the following proposal:

  • we're going to reduce complexity and overlap, starting with 1.43+
  • changes might be breaking (expected, as set_property is experimental)
  • cmake_target_name will accept a list of targets to declare as many aliases as needed
  • cmake_target_name will always specify the full name as is (means, if you want a target named zlib::zlib, you specify zlib::zlib into the property)
  • cmake_target_namespace and cmake_target_aliases properties will be removed, as redundant (as same effect could be achieved with just cmake_target_name)
  • there will be no intermediate targets defined, you will get only targets you asked for, and cmake scope won't be polluted
  • cmake_tatget_name will work on both component and global level
  • if you don't specify property explicitly, there will be recipe name::recipe name and recipe name::component name targets generated by default

there are still details to be figured out, we're going to open new issue and invite you to the discussion

hopefully, another wave of set_property changes won't be so bad for ConanCenter, as there are not so many recipes already using set_property right now (maybe around 20 recipes).

@SpaceIm
Copy link
Contributor

SpaceIm commented Nov 18, 2021

I like this new logic.

cmake_target_name will always specify the full name as is (means, if you want a target named zlib::zlib, you specify zlib::zlib into the property)

Ok, so it's urgent to do nothing in CCI, except go back to .names to avoid breaking change, until set_property() is stable enough. One drawback for CMakeDeps: #9943

Question: What will be the name of the component when you call find_package(Package COMPONENT component1 component2)? Will you split :: and take the second part by default? Few libs have a different name for the component and the imported target (SFML for example), will there be a way to define this value without creating a fake target?

@SSE4
Copy link
Contributor Author

SSE4 commented Nov 18, 2021

Question: What will be the name of the component when you call find_package(Package COMPONENT component1 component2)? Will you split :: and take the second part by default? Few libs have a different name for the component and the imported target (SFML for example), will there be a way to define this value without creating a fake target?

that's an interesting question, and probably will require some investigation.
CMake documentation is a bit vague:

The set of available components and their meaning are defined by the target package. 
Formally, it is up to the target package how to interpret the component information given to it, 
but it should follow the expectations stated above. 
For calls where no components are specified,
 there is no single expected behavior and target packages should clearly define what occurs in such cases. 
Common arrangements include assuming it should find all components, 
no components or some well-defined subset of the available components.

is it possible that I do find_package(Package COMPONENT component_name) and get a target with name different from Package::component_name? IDK right now, maybe need to do some research with existing libraries

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.

[feature] Tool: create module official cmake targets
10 participants