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] Define file name for CMake find generators #7254

Closed
danimtb opened this issue Jun 25, 2020 · 11 comments
Closed

[feature] Define file name for CMake find generators #7254

danimtb opened this issue Jun 25, 2020 · 11 comments

Comments

@danimtb
Copy link
Member

danimtb commented Jun 25, 2020

Coming from #7217 (comment)

closes #6069

There is one limitation regarding the name of the find_package files and targets in CMake. Currently, the name specified in self.cpp_info.names["cmake_find_package"] is the one used to generate the Find.cmake as well as the namespace for the targets <name>::component.

There are use cases where those names are different, for example protobuf has FindProtobuf.cmake while the targets are protobuf::libprotobuf.

The proposal for this would be to add something like self.cpp_info.filename["cmake_find_package"] = "Protobuf" so both names can be defined in the recipe:

    def pacakge_info(self):
        self.cpp_info.filename["cmake_find_package"] = "Protobuf"
        self.cpp_info.names["cmake_find_package"] = "protobuf"
        self.cpp_info.components["libprotobuf"] = "libprotobuf"

cc/ @TimSimpson @uilianries

@jgsogo
Copy link
Contributor

jgsogo commented Jun 25, 2020

This one is related to #6069, probably this one should close that one once fixed/added/closed

@TimSimpson
Copy link
Contributor

There's another case where multiple CMake packages might use a common namespace, such as a theoretical modularized Boost. To restate that example, there may exist a Conan package for Boost-coroutine and another one for Boost-asio. From CMake these would be used by calling find_package(Boost-coroutine), which would bring in target Boost::coroutine, and find_package(Boost-asio) which would bring in target Boost::asio.

If filename is added, it would be possible to make this work, however self.cpp_info.names["cmake_find_package"] would be Boost for both packages.

I would guess this might lead to adverse affects on other generators as they could create files or other build components all named "Boost" which would conflict with each other.

Would it be better to instead create a property like cmake_namspace? This way it only affects the generated CMake. So it would be:

def pacakge_info(self):
        self.cpp_info.names["cmake_find_package"] = "Protobuf"
        self.cpp_info.cmake_target = "protobuf"
        self.cpp_info.components["libprotobuf"] = "libprotobuf"

Then for the boost example you'd have

def pacakge_info(self):
        self.cpp_info.names["cmake_find_package"] = "Boost-coroutine"
        self.cpp_info.cmake_target = "boost"
        self.cpp_info.components["libprotobuf"] = "coroutine"

and

def pacakge_info(self):
        self.cpp_info.names["cmake_find_package"] = "Boost-asio"
        self.cpp_info.cmake_target = "boost"
        self.cpp_info.components["libprotobuf"] = "asio"

@jgsogo
Copy link
Contributor

jgsogo commented Jun 25, 2020

Your modularized boost example would work as follows with the initial proposal:

    def pacakge_info(self):
        self.cpp_info.filename["cmake_find_package"] = "Boost-asio"
        self.cpp_info.names["cmake_find_package"] = "boost"
        self.cpp_info.components["asio"].libs = ["asio"]
    def pacakge_info(self):
        self.cpp_info.filename["cmake_find_package"] = "Boost-coroutine"
        self.cpp_info.names["cmake_find_package"] = "boost"
        self.cpp_info.components["coroutine"].libs = ["coroutine"]

@TimSimpson
Copy link
Contributor

@jgsogo I'm wondering if there would be unforeseen issues with other generators since self.cpp_info.names is so well-established and it may be hard to predict all the effects of having multiple packages set it to the same value. If people start trying to share the same setting for names across multiple packages just to change the CMake target namespace, that seems more intrusive than if they could change a property that would just change the CMake target namespace.

@TimSimpson
Copy link
Contributor

Another idea: what if the include statements for the build modules which appear in the generate find modules were moved to after the current targets were created? which would make it possible to clone them? Build modules can be used to create "clones" of the find module targets today, but doing so involves relying on the same variables Conan happens to use to create the targets which may be brittle.

In my mind this seems safer as it lets all the other code in Conan use the targets Conan already generates today, while allowing CMake superfans to use alternate targets which look exactly how they wish.

@jgsogo
Copy link
Contributor

jgsogo commented Jun 26, 2020

The names approach is already working, names are assigned per generator, so adding to CMake doesn't mean it will affect other generators. The feature (together with the filename) does exactly what you are suggesting: "just change the CMake target namespace". Name collision could eventually be an issue, but there is nothing we can do from the Conan side (maybe an error instead of overriding), it is up to the recipe maintainers to manage that situation, we'll try to do our best with the recipes in conan-center-index.

The idea for the build_modules is to provide functions/macros to the CMake consumers that were already provided by the CMake's module files, not to impostate the targets defined in the recipe. Now that we will be able to replicate the name of the FindXXXX.cmake file, the namespace of the provided targets and the name of the targets themselves, there is no need to "clone" those targets. If a consumer needs different names that snippet should go on the consumer's side.

@TimSimpson
Copy link
Contributor

@jgsogo you're right: names are assigned per generator, so adding to CMake doesn't mean it will affect other generators. Sorry, I was in some sort of fugue state when I wrote the above and was got self.cpp_info.names (plural) mixed up with self.cpp_info.name which, of course, can can keep doing whatever it already does today.

TimSimpson added a commit to TimSimpson/conan that referenced this issue Jun 29, 2020
Relates to [this issue](conan-io#7254)
@TimSimpson
Copy link
Contributor

@danimtb @jgsogo I can contribute this code if that's helpful. I've started a work on a branch here though it needs another few rounds of polish. Full disclosure: I don't have much time to work on this stuff outside of nights and weekends though I'd certainly like to see this in Conan soon. It's also no big deal if someone else picks it up before me.

I've also created this project as a recreate to test the situation of having multiple Conan packages which use the same target namespace in their find module exported targets. Right now my branch linked above can successfully build both projects in the recreate, but when I scan the files I can spot a couple of places where it's creating the wrong targets in the dependent project's find modules still.

One observation I'd like to share is that there are a lot of global variables in the generated find modules which I had to change to use the new filenames attribute instead to make sure the two modules didn't create the same variables as each other. I'm probably beating a dead horse but it made me wonder again if having an attribute specifically for the CMake namespace target would be more optimal in the long run.

Second, the requires attribute of the components look like they refer to CMake targets- and in most cases they do - but actually the string before the :: is the package name defined from within Conan and not the CMake target namespace name.

This is probably intuitive from a heavy Conan user's perspective but could confuse new users coming from a strictly CMake packaging world and should probably get a call-out in the docs. For example, the recreate's project B's package_info method refers to acme-a::a, while in CMake no such target exists (there it's called ACME::a).

@jgsogo
Copy link
Contributor

jgsogo commented Jun 29, 2020

Hi! Contributions are more than welcome. If you are working on this, feel free to open the PR as soon as possible so we can see how it is going and provide feedback to you as early as possible. I'm assigning it to the next release, we have four weeks ahead, if you think you won't have time to finish it, just tell us and we will resume your work.

About the values in the requires field, more/better docs are always needed (PRS more than welcome 🙄 ), but yes, as you have realized they use the name of the package and components, not any other name assigned via names["<generator>"].

@jgsogo jgsogo added this to the 1.28 milestone Jun 29, 2020
TimSimpson added a commit to TimSimpson/conan that referenced this issue Jul 5, 2020
Relates to [this issue](conan-io#7254)
TimSimpson added a commit to TimSimpson/docs that referenced this issue Jul 7, 2020
This documents the new `filename` attribute as described by
[issue 7254](conan-io/conan#7254).
TimSimpson added a commit to TimSimpson/conan that referenced this issue Jul 7, 2020
Relates to [this issue](conan-io#7254)
TimSimpson added a commit to TimSimpson/conan that referenced this issue Jul 12, 2020
Relates to [this issue](conan-io#7254)
@prince-chrismc
Copy link
Contributor

There's one other scenario, where the upstream does not provide a namespace. The issue is highlighted here conan-io/conan-center-index#2151

danimtb pushed a commit that referenced this issue Jul 29, 2020
* adds `filenames` to cpp_info

Relates to [this issue](#7254)

* use `filenames` attribute, not name, as prefix to global vars

* `filenames` should fall back to `names`, not package name

* removes debug statements

* Makes `cmake_find_package_multi` work with `filenames` attribute

* adds a test for the filename attribute

* Update find_package generators to not use `filename` in global var names

* change `name` used in cmake_find_package_test, since sharing them isn't possible now

* Set both `{name}_FOUND` and `{filename}_FOUND` vars so code will work

* reverts `filename` of `name` in some spots (danimtb feedback)

* fixes bug in cmake_fine_package_multi_test

* adds a test for changing filename in cmake_find_package_multi generator

* use filename version of _FOUND in if statements...

This allows CMake exported target namespaces to be shared.

* set upper case version of _FOUND vars
@danimtb
Copy link
Member Author

danimtb commented Jul 29, 2020

This has been implemented in #7320. Thanks a lot to everyone for the collaboration!!

@danimtb danimtb closed this as completed Jul 29, 2020
kralicky added a commit to kralicky/conan that referenced this issue Aug 4, 2020
* Feature: add settings for clang-cl (clang on Windows) (conan-io#5705)

* - add settings for clang-cl (clang on Windows)

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

* - add new VS toolsets for Clang

* fix migration test

* Feature/toolchain without dump (conan-io#7435)

* toolchain without dump()

* working

* fixing tests

* relax pluginbase requirements (conan-io#7441)

* New lockfiles iteration. Including version-only lockfiles. (conan-io#7243)

* try to reproduce error in lockfile

* iterate build_order

* package in requires and build_requires

* rename test file

* removing constraint multiple packages

* minor improvements

* first lock-recipe version working

* working

* working

* working 75%, but revisions not managed

* working

* working

* working...

* allow no user/channel match in --build

* build order

* working

* working

* working

* not working...

* working

* tests passing without revisions

* working

* review and patterns for rrevs

* new tests

* some fixes

* more fixes

* skip build-info

* fix revision test msg

* removed global activation of revisions

* multiple br test

* cli changes

* working

* new cli syntax

* fixing tests

* partial lockfiles

* CI partial lock working

* make sure partial lockfiles are used completely and from its root

* version ranges and py_requires working

* partial CI with python_requires

* comment

* tests passing

* working

* working in build_requires

* CI for build-requires test

* recovering conan_build_info tests

* broken py27

* clean-modified, stricter locks

* partial arranged

* minor style fixes

* compacting lockfile search for nodes

* remove unnecessary --build=missing

* processing --update

* testing the --update necessary for revisions non matching

* make the lockfile-out compulsory

* fixed conan_build_info

* forcing --lockfile-out

* workspace change

* Update conans/client/command.py

Co-authored-by: Javier G. Sogo <[email protected]>

* make lock_node.path relative

* new tests and fixes

* remove hardcoded os.environ

* make lockfile-out required

* fix test

* fixing os.path.relpath in Win

* more tests

* fix test

* fix test

* checking --lockfile-out requires --lockfile

* working

* review

* rename get_consumer

* a partial lock can match more than 1 require

* error msg

* working

* check that profile is not passed when using lockfiles

* working

* working

* recover profile_build from lockfile

* add new test for --update

* make sure modified is propagated

* with revisions

* format

* changed build-order order of output

* update cmd line

* review

* Update conans/client/command.py

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

* Update conans/client/command.py

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

* Update conans/client/command.py

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

* forbidding version ranges when using lockfiles, in command line

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

* Support for components in pkg_config generator (conan-io#7413)

* Support for components in pkg_config generator

* capitalize

* remove code not needed

* use real pkg-config for tests

* try without skip

* [feature] Define file name for CMake find generators (conan-io#7320)

* adds `filenames` to cpp_info

Relates to [this issue](conan-io#7254)

* use `filenames` attribute, not name, as prefix to global vars

* `filenames` should fall back to `names`, not package name

* removes debug statements

* Makes `cmake_find_package_multi` work with `filenames` attribute

* adds a test for the filename attribute

* Update find_package generators to not use `filename` in global var names

* change `name` used in cmake_find_package_test, since sharing them isn't possible now

* Set both `{name}_FOUND` and `{filename}_FOUND` vars so code will work

* reverts `filename` of `name` in some spots (danimtb feedback)

* fixes bug in cmake_fine_package_multi_test

* adds a test for changing filename in cmake_find_package_multi generator

* use filename version of _FOUND in if statements...

This allows CMake exported target namespaces to be shared.

* set upper case version of _FOUND vars

* Add Conan version to HTML output (conan-io#7443)

* conan-io#7365 Add Conan version in search table

Signed-off-by: Uilian Ries <[email protected]>

* conan-io#7365 Add Conan version in info graph

Signed-off-by: Uilian Ries <[email protected]>

* conan-io#7365 Add shebang for python2.7

Signed-off-by: Uilian Ries <[email protected]>

* conan-io#7365 Use JS for current date

Signed-off-by: Uilian Ries <[email protected]>

* conan-io#7365 Do not rename old tests

Signed-off-by: Uilian Ries <[email protected]>

* conan-io#7365 Remove copyright symbol to allow py27

Signed-off-by: Uilian Ries <[email protected]>

* Refactor filenames and simplify tests (conan-io#7447)

* Refactor filenames and simplify tests

* fix

* small fixes

* review fix

* rename variables

* fix linux tests

* version 1.28.0

* version 1.29.0-dev

* fix migration test

* Hotfix/lockfile errors (conan-io#7453)

* improve error msgs

* better error msgs

* minor fixes to error msgs and better checks

* Fixes bad powershell separators on Linux (conan-io#7472)

* minor improvements in components checks (conan-io#7486)

* fix message missing components (conan-io#7483)

* fix message missing components

* changed error message

Co-authored-by: SSE4 <[email protected]>
Co-authored-by: memsharded <[email protected]>
Co-authored-by: David Roman <[email protected]>
Co-authored-by: jgsogo <[email protected]>
Co-authored-by: Carlos Zoido <[email protected]>
Co-authored-by: Daniel <[email protected]>
Co-authored-by: Tim Simpson <[email protected]>
Co-authored-by: Uilian Ries <[email protected]>
Co-authored-by: mjvankampen <[email protected]>
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

4 participants