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

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

Merged
merged 9 commits into from
Jan 18, 2021

Conversation

jgsogo
Copy link
Contributor

@jgsogo jgsogo commented Jan 14, 2021

Changelog: omit
Docs: omit

Adds a test using build_modules to create a target without namespace. This target consumes the targets provided by Conan.

closes #7615 - At this moment we are not implementing anything else in the model to create targets without namespaces (it is very CMake specific and cpp_info should have some level of abstraction over different build-systems). ping @madebr @SpaceIm

@jgsogo jgsogo added this to the 1.33 milestone Jan 14, 2021
conans/model/build_info.py Outdated Show resolved Hide resolved
Comment on lines +569 to +572
if(NOT TARGET nonamespace)
add_library(nonamespace INTERFACE IMPORTED)
target_link_libraries(nonamespace INTERFACE library::library)
endif()
Copy link
Contributor

@SpaceIm SpaceIm Jan 14, 2021

Choose a reason for hiding this comment

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

What happen if a consumer relies on another generator than cmake_find_package(_multi) (cmake for example)? library::library doesn't exist anymore, so I guess target_link_libraries(nonamespace INTERFACE library::library) will fail?

Maybe this?

Suggested change
if(NOT TARGET nonamespace)
add_library(nonamespace INTERFACE IMPORTED)
target_link_libraries(nonamespace INTERFACE library::library)
endif()
if(NOT TARGET nonamespace AND TARGET library::library)
add_library(nonamespace INTERFACE IMPORTED)
target_link_libraries(nonamespace INTERFACE library::library)
endif()

Copy link
Contributor Author

@jgsogo jgsogo Jan 14, 2021

Choose a reason for hiding this comment

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

I wrote a comment, but it went away with some of the commits 🤷 .

As you can see here, I can specify a different build-module per generator (f2163c6), otherwise it is the recipe author the one that needs to implement the required logic inside the build-module, of course, as you have suggested.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, so I understand that we could already (conan 1.32.0) provide non namespaced CMake imported targets in CCI recipe? conan 1.32.0 doesn't allow build_modules per generator but with some tricks it should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Limitation in 1.32 is that build_modules are included before the Conan targets. It was changed here: #8130

Copy link
Contributor

@SpaceIm SpaceIm Jan 14, 2021

Choose a reason for hiding this comment

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

Surprinsingly, it works in 1.32: conan-io/conan-center-index#4258
If I add message() in the build_modules file, I see that it's called twice: one before targets found (in cmake_find_package_multi) and another one after.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dammit! 😅 It should be some kind of (evil) CMake laziness resolution...

Copy link
Contributor

@madebr madebr Jan 14, 2021

Choose a reason for hiding this comment

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

So the idea is to create cmake build modules that are aliases to the conan generated ones?
This is supposed to happen in build, package or copied through from exports_sources?

Sounds good to me! 💯

My next question is to add a build helper, as this is going to be often needed in cci.

Something like:

from conan.tools.cmake import cmake_tools

def package(self):
    cmake_tools.write_alias_module
        {
            "library": "library::library",
            "library2": "library::library2",
        },
        filename=None,  # default is os.path.join("lib", "cmake", "build-modules.cmake"),
        generators=None,  # (generators' default is `cmake_find_package` and `cmake_find_package_multi`)
    )

def package_info(self):
    # ...
    cmake_tools.append_alias_module_filename(
        self.cpp_info,
        filename=None,  # default is os.path.join("lib", "cmake", "build-modules.cmake"),
        generators=None,  # default is os.path.join("lib", "cmake", "build-modules.cmake"),
    )

The idea is to need no extra arguments by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a helper ❤️

Copy link
Contributor

@SpaceIm SpaceIm Jan 15, 2021

Choose a reason for hiding this comment

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

Dammit! 😅 It should be some kind of (evil) CMake laziness resolution...

Actually, it seems to work in conan 1.32 if there are no components in the recipe. It fails in opencv recipe (lot of components): conan-io/conan-center-index#4267

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About the helper, something for a feature request, not for this PR. Here we only wanted to check that adding targets without namespaces was possible.

}
""")

@classmethod
Copy link
Member

Choose a reason for hiding this comment

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

why not using pytest fixtures? I have started to used them and so far look great, if we are going pytest, probably we want to start adopting its practices?

Copy link
Contributor Author

@jgsogo jgsogo Jan 14, 2021

Choose a reason for hiding this comment

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

because the purpose here is to execute these lines once for all the tests in this class (I could use class-scoped fixture).

A fixture typically provides a way to execute (and inject) something before/after tests, but IMHO something intended to be reusable by many tests in the test suite. This one probably applies only to this test-class, I think these class methods are better than fixtures when they apply only to one specific test.

@jgsogo jgsogo merged commit 4e9d8b0 into conan-io:develop Jan 18, 2021
@jgsogo jgsogo deleted the example/7615-no-namespace-target branch January 18, 2021 14:50
memsharded pushed a commit to memsharded/conan that referenced this pull request Jan 18, 2021
…o#8338)

* use target without namespace

* add marks

* use target from the build-module

* remove line

* build-module per generator

* revert changes in build_info

* make it explicit it is PATH

* even without build-type, compiler.runtime is different for Release/Debug

* use x64 for Windows generator
@jgsogo
Copy link
Contributor Author

jgsogo commented Jan 18, 2021

Hi! I would say it should be more a built-in feature than something for a custom build-module. I'd vote for this issue (#7240) and then cmake generators can create those targets as well.

memsharded added a commit that referenced this pull request Jan 18, 2021
* working in msvc new settings

* preliminary support for msvc compiler

* fix migration

* adding msvc for msbuild toolchain

* changing to version 19.1, 19.11, etc

* working on static/dynamic runtime

* extracting toolchain checkers to use in CMake

* removed unused import

* adding checks

* adding check

* first proposal

* msvc proposal for runtime

* fix migration test

* fixing tests

* binary compatibility

* command conan new generates files with new toolchains

* removed conanfile

* removed files

* msbuild working for msvc

* fixing tests

* fixing tests

* refactors

* Update conan/tools/microsoft/toolchain.py

Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>

* Update conan/tools/microsoft/toolchain.py

Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>

* Fix #7715, in rpath_flags use host OS instead of build OS to determine -rpath separator (#7716)

* Fix #7715, in rpath_flags use host OS instead of build OS to determine the separator for the ld -rpath flag which is neccessary for cross-compiling to Mac OS

* For #7715, don't pass os_host to rpath_flags but extract from settings, pass os_build again and only set rpath_separator to , if compiling for Mac OS from Linux or Mac OS via GCC_LIKE

* Fix test_compiler_args for cross-compiling Mac OS X -> Linux

* Update conans/client/build/compiler_flags.py

Co-authored-by: SSE4 <tomskside@gmail.com>

* Update conans/client/build/compiler_flags.py

Co-authored-by: SSE4 <tomskside@gmail.com>

* Update conans/test/unittests/client/generators/compiler_args_test.py

* Update conans/client/build/compiler_flags.py

* Use -rpath, in all tests for all platforms

* fixing tests

Co-authored-by: James <james@conan.io>
Co-authored-by: SSE4 <tomskside@gmail.com>

* modernizing tests (#8340)

* extracting some common code to base class (#8341)

* modernizing tests (#8345)

* Fix help message in command remove --outdated (#8350)

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

* use target without namespace

* add marks

* use target from the build-module

* remove line

* build-module per generator

* revert changes in build_info

* make it explicit it is PATH

* even without build-type, compiler.runtime is different for Release/Debug

* use x64 for Windows generator

* update tests (#8354)

* added integration test for msbuild

Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>
Co-authored-by: Niklas Gürtler <Erlkoenig90@users.noreply.github.com>
Co-authored-by: SSE4 <tomskside@gmail.com>
Co-authored-by: Daniel <danimanzaneque@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>

if(NOT TARGET nonamespace)
add_library(nonamespace INTERFACE IMPORTED)
target_link_libraries(nonamespace INTERFACE library::library)
Copy link
Contributor

@SpaceIm SpaceIm Jan 26, 2021

Choose a reason for hiding this comment

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

It seems that set_property(TARGET nonamespace PROPERTY INTERFACE_LINK_LIBRARIES library::library) has wider support across CMake versions (because nonamespace is an IMPORTED target I guess):
conan-io/conan-center-index#4369

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] cmake_find_package[_multi] generators: provide a way to create non namespaced imported targets
7 participants