Inject CMake build_modules after targets are created #8130

danimtb · 2020-11-30T16:56:56Z

Changelog: Fix: Inject build modules after CMake targets are created
Docs: conan-io/docs#1944

This allows uisng build modules to generate non-namespaced targets. --

Refer to the issue that supports this Pull Request: closes [feature] CMake generators: Move includes of build_modules after targets are created in generated CMake find modules #7261, closes [feature] cmake_find_package[_multi] generators: provide a way to create non namespaced imported targets #7615
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.}

conans/client/generators/cmake_find_package.py

conans/client/generators/cmake_find_package_common.py

conans/test/functional/generators/cmake_test.py

conans/client/generators/cmake_find_package_multi.py

memsharded

Looking good, I think it is ready, somethings could be improved now, but I wouldn't oppose if they are fixed later.

conans/test/functional/generators/cmake_find_package_test.py

conans/client/generators/cmake_find_package_multi.py

jgsogo

This PR is a bit challenging. I'm assuming these expectations:

build modules are included after the element they refer to:
- global build-modules at the very end
- build-modules per component, after corresponding component is defined (nice to have, but they could go at the bottom too).
per-component build-modules are not included as global ones as well.
build-modules for a given configuration are added only when that configuration is used (requires some generator expression).

Please, tell me if I'm wrong with any of the previous points.

conans/client/generators/cmake_find_package.py

jgsogo · 2021-01-12T16:36:55Z

conans/test/functional/generators/cmake_find_package_test.py

+class TestCMakeFindPackageGenerator:
+
+    @pytest.mark.parametrize("use_components", [False, True])
+    def test_build_modules_alias_target(self, use_components):


In this test, when use_components =True, I can see the following:

Findhello.cmake

... set(namespace_BUILD_MODULES_PATHS "/private/var/folders/fc/6mvcrc952dqcjfhl4c7c11ph0000gn/T/tmphcq2mp5yconans/path with spaces/data/hello/1.0/_/_/package/f83037eff23ab3a94190d7f3f7b37a2d6d522241/share/cmake/target-alias.cmake") ... set(namespace_comp_BUILD_MODULES_PATHS "/private/var/folders/fc/6mvcrc952dqcjfhl4c7c11ph0000gn/T/tmphcq2mp5yconans/path with spaces/data/hello/1.0/_/_/package/f83037eff23ab3a94190d7f3f7b37a2d6d522241/share/cmake/target-alias.cmake") ... ... foreach(_BUILD_MODULE_PATH ${namespace_BUILD_MODULES_PATHS}) include(${_BUILD_MODULE_PATH}) endforeach()

I would expect the build-module to appear only at the component level... and, of course, a loop calling include for each one of them.

Still the same, the same build-module appear at component and global level.

jgsogo · 2021-01-12T16:43:30Z

conans/client/generators/cmake_find_package_multi.py

+    include(${_BUILD_MODULE_PATH})
+endforeach()
+{%- endfor %}
+    """)


Here we are adding unconditionally all the build-modules. The resulting cmake file contain these lines:

# Build modules foreach(_BUILD_MODULE_PATH ${hello_BUILD_MODULES_PATHS_DEBUG}) include(${_BUILD_MODULE_PATH}) endforeach() foreach(_BUILD_MODULE_PATH ${hello_BUILD_MODULES_PATHS_MINSIZEREL}) include(${_BUILD_MODULE_PATH}) endforeach() foreach(_BUILD_MODULE_PATH ${hello_BUILD_MODULES_PATHS_RELWITHDEBINFO}) include(${_BUILD_MODULE_PATH}) endforeach() foreach(_BUILD_MODULE_PATH ${hello_BUILD_MODULES_PATHS_RELEASE}) include(${_BUILD_MODULE_PATH}) endforeach()

All of them are included regardless of the build_type used. We need a generator expression to expand and use only the build-modules for the matching build-type.

I have tried modifying this to use generator expressions here, but those only work in target properties as generator expressions are specific for CMake generators. I have also tried using CMAKE_BUILD_TYPE to make the conditionals but it is not available. I am leaving this as it is right now

That's right. Generator expressions only work with target-related properties... so for multi-generators this is unsolvable. 🤔 . Maybe something to document.

Then it is clearly a CMake limitation and I'd extend it to non-multi generators as well (for consistency). It means that the list of build-modules to include cannot be different based on the configuration name (typically build_type), recipe authors need to implement the build-module with this circumstance in mind. We'd go crazy trying to protect the user from it given our cpp_info object.

jgsogo · 2021-01-12T16:45:58Z

conans/client/generators/cmake_find_package_multi.py

+            include(${_BUILD_MODULE_PATH})
+        endforeach()
+
+        {%- endfor %}


Same comment as above, here we add all the build-modules unconditionally, we need a generator expression to include only the ones for the matching config.

This is current output:

########## BUILD MODULES #################################################################### ############################################################################################# ########## GLOBAL BUILD MODULES ############################################################# foreach(_BUILD_MODULE_PATH ${namespace_BUILD_MODULES_PATHS_DEBUG}) include(${_BUILD_MODULE_PATH}) endforeach() foreach(_BUILD_MODULE_PATH ${namespace_BUILD_MODULES_PATHS_MINSIZEREL}) include(${_BUILD_MODULE_PATH}) endforeach() foreach(_BUILD_MODULE_PATH ${namespace_BUILD_MODULES_PATHS_RELWITHDEBINFO}) include(${_BUILD_MODULE_PATH}) endforeach() foreach(_BUILD_MODULE_PATH ${namespace_BUILD_MODULES_PATHS_RELEASE}) include(${_BUILD_MODULE_PATH}) endforeach()

jgsogo · 2021-01-12T16:50:08Z

conans/test/functional/generators/cmake_find_package_multi_test.py

@@ -139,6 +139,105 @@ def build(self):
        client.run("create .")
        self.assertIn("Printing using a external module!", client.out)

+    @parameterized.expand([(False,), (True,)])
+    def test_build_modules_alias_target(self, use_components):


Same issue as the one with the cmake_find_package generator:

helloTarget-release.cmake

... set(namespace_BUILD_MODULES_PATHS_RELEASE "/private/var/folders/fc/6mvcrc952dqcjfhl4c7c11ph0000gn/T/tmpf8fefhlrconans/path with spaces/data/hello/1.0/_/_/package/f83037eff23ab3a94190d7f3f7b37a2d6d522241/share/cmake/target-alias.cmake") ... set(namespace_comp_BUILD_MODULES_PATHS_RELEASE "/private/var/folders/fc/6mvcrc952dqcjfhl4c7c11ph0000gn/T/tmpf8fefhlrconans/path with spaces/data/hello/1.0/_/_/package/f83037eff23ab3a94190d7f3f7b37a2d6d522241/share/cmake/target-alias.cmake") ...

I expect the build-module to appear listed only under the namespace_comp ones...

hello-config.cmake

... ########## BUILD MODULES #################################################################### ############################################################################################# ########## GLOBAL BUILD MODULES ############################################################# foreach(_BUILD_MODULE_PATH ${namespace_BUILD_MODULES_PATHS_DEBUG}) include(${_BUILD_MODULE_PATH}) endforeach() foreach(_BUILD_MODULE_PATH ${namespace_BUILD_MODULES_PATHS_MINSIZEREL}) include(${_BUILD_MODULE_PATH}) endforeach() foreach(_BUILD_MODULE_PATH ${namespace_BUILD_MODULES_PATHS_RELWITHDEBINFO}) include(${_BUILD_MODULE_PATH}) endforeach() foreach(_BUILD_MODULE_PATH ${namespace_BUILD_MODULES_PATHS_RELEASE}) include(${_BUILD_MODULE_PATH}) endforeach() ...

Build-modules per component are not included.

jgsogo

Agree we cannot decide which build-modules to include based on the configuration for multi-config generator (the include function cannot use generator expressions). Agree we shouldn't add the feature to non-multi generator for consistency too.

But, I think we can include only build-modules at the level where they are declared.

I'll have a look to the implementation.

conans/test/functional/generators/cmake_find_package_multi_test.py

jgsogo · 2021-01-14T08:54:20Z

conans/test/functional/generators/cmake_find_package_test.py

+class TestCMakeFindPackageGenerator:
+
+    @pytest.mark.parametrize("use_components", [False, True])
+    def test_build_modules_alias_target(self, use_components):


Still the same, the same build-module appear at component and global level.

jgsogo

Wow, I'm no longer sure if it is worth fixing/adding that in this PR. After having a look at the sources I can see that we are populating global variables even if we are using components. This is intended and it is good according to the docs where we list the CMake variables that are available for the different generators.

And I see current implementations include only the proper build-modules: global ones or components, depending on the generator.

Good!

[POC] Use build modules to generate non-namespaced targets

5460581

danimtb added this to the 1.32 milestone Nov 30, 2020

danimtb self-assigned this Nov 30, 2020

danimtb commented Nov 30, 2020

View reviewed changes

conans/client/generators/cmake_find_package.py Show resolved Hide resolved

memsharded reviewed Nov 30, 2020

View reviewed changes

conans/client/generators/cmake_find_package_common.py Show resolved Hide resolved

danimtb added 3 commits December 1, 2020 12:27

move build modules in components cmake_find_package impl

d089955

Merge branch 'develop' into feature/7615

08228c4

move all build modules after targets are created

5e49225

danimtb changed the title ~~[POC] Use build modules to generate non-namespaced targets~~ Inject CMake build_modules after targets are created Dec 1, 2020

danimtb commented Dec 1, 2020

View reviewed changes

conans/test/functional/generators/cmake_test.py Outdated Show resolved Hide resolved

fixes

03de1cf

danimtb mentioned this pull request Dec 1, 2020

CMake build modules are injected right after the targets are created conan-io/docs#1944

Merged

danimtb marked this pull request as ready for review December 1, 2020 17:03

danimtb assigned jgsogo and unassigned danimtb Dec 1, 2020

memsharded reviewed Dec 1, 2020

View reviewed changes

conans/client/generators/cmake_find_package_multi.py Outdated Show resolved Hide resolved

conans/client/generators/cmake_find_package_multi.py Show resolved Hide resolved

conans/client/generators/cmake_find_package_multi.py Show resolved Hide resolved

danimtb mentioned this pull request Dec 2, 2020

[feature] cmake_find_package[_multi] generators: provide a way to create non namespaced imported targets #7615

Closed

1 task

danimtb added 7 commits December 2, 2020 12:46

remove components build modules in cmake

30bfc71

Merge branch 'develop' into feature/7615

05ab7b8

fix target name

a2a7334

debugging print

1559525

print paths

f822829

use targets

63e9c85

build app

4bdbed2

czoido modified the milestones: 1.32, 1.33 Dec 2, 2020

Merge branch 'develop' into feature/7615

0866da8

danimtb force-pushed the feature/7615 branch from 390d329 to 0866da8 Compare December 18, 2020 14:27

Merge branch 'develop' into feature/7615

934bfd7

danimtb requested a review from jgsogo January 12, 2021 11:51

fix parametrized

071bcba

memsharded approved these changes Jan 12, 2021

View reviewed changes

conans/test/functional/generators/cmake_find_package_test.py Show resolved Hide resolved

conans/client/generators/cmake_find_package_multi.py Outdated Show resolved Hide resolved

jgsogo suggested changes Jan 12, 2021

View reviewed changes

danimtb added 4 commits January 12, 2021 18:12

avoid building

6db61f2

tests without building

39a7823

review

8544eb3

fix tests

81f181a

jgsogo suggested changes Jan 14, 2021

View reviewed changes

jgsogo approved these changes Jan 14, 2021

View reviewed changes

jgsogo mentioned this pull request Jan 14, 2021

[refact] Remove duplicated code #8334

Merged

jgsogo merged commit ad294d1 into conan-io:develop Jan 14, 2021

jgsogo mentioned this pull request Jan 14, 2021

[test] Add test creating a target without namespaces (CMake) #8338

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inject CMake build_modules after targets are created #8130

Inject CMake build_modules after targets are created #8130

danimtb commented Nov 30, 2020 •

edited by jgsogo

Loading

memsharded left a comment

jgsogo left a comment •

edited

Loading

jgsogo Jan 12, 2021

jgsogo Jan 14, 2021

jgsogo Jan 12, 2021

danimtb Jan 13, 2021

jgsogo Jan 13, 2021

jgsogo Jan 12, 2021

jgsogo Jan 12, 2021

jgsogo left a comment

jgsogo Jan 14, 2021

jgsogo left a comment •

edited

Loading

Inject CMake build_modules after targets are created #8130

Inject CMake build_modules after targets are created #8130

Conversation

danimtb commented Nov 30, 2020 • edited by jgsogo Loading

memsharded left a comment

Choose a reason for hiding this comment

jgsogo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgsogo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgsogo left a comment • edited Loading

Choose a reason for hiding this comment

danimtb commented Nov 30, 2020 •

edited by jgsogo

Loading

jgsogo left a comment •

edited

Loading

jgsogo left a comment •

edited

Loading