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
Merged
122 changes: 122 additions & 0 deletions conans/test/functional/generators/cmake_find_package_multi_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -526,3 +526,125 @@ def build(self):
self.assertIn("component libs: $<$<CONFIG:Debug>:;>;$<$<CONFIG:MinSizeRel>:;>;"
"$<$<CONFIG:RelWithDebInfo>:;>;$<$<CONFIG:Release>:system_lib_component;",
t.out)


@pytest.mark.tool_cmake
class TestNoNamespaceTarget:
""" This test case uses build-modules feature to create a target without a namespace. This
target uses targets create by Conan (build_modules are included after Conan targets)
"""

conanfile = textwrap.dedent("""
import os
from conans import ConanFile, CMake

class Recipe(ConanFile):
settings = "os", "compiler", "arch", "build_type"
exports_sources = ["src/*", "build-module.cmake"]
generators = "cmake"

def build(self):
cmake = CMake(self)
cmake.configure(source_folder="src")
cmake.build()

def package(self):
self.copy("*.h", dst="include", src="src")
self.copy("*.lib", dst="lib", keep_path=False)
self.copy("*.dll", dst="bin", keep_path=False)
self.copy("*.dylib*", dst="lib", keep_path=False)
self.copy("*.so", dst="lib", keep_path=False)
self.copy("*.a", dst="lib", keep_path=False)
self.copy("build-module.cmake", dst="share/cmake")

def package_info(self):
self.cpp_info.libs = ["library"]
module = os.path.join("share", "cmake", "build-module.cmake")
self.cpp_info.build_modules['cmake_find_package'] = [module, ]
self.cpp_info.build_modules['cmake_find_package_multi'] = [module, ]
""")

build_module = textwrap.dedent("""
message(">> Build-module is included")

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

endif()
Comment on lines +570 to +573
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.

""")

consumer = textwrap.dedent("""
cmake_minimum_required(VERSION 3.0)
set(CMAKE_CXX_COMPILER_WORKS 1)
set(CMAKE_CXX_ABI_COMPILED 1)
project(consumer)

find_package(library)

get_target_property(LIBS1 library::library INTERFACE_LINK_LIBRARIES)
message(">> library::library libs: ${LIBS1}")

get_target_property(LIBS2 nonamespace INTERFACE_LINK_LIBRARIES)
message(">> nonamespace libs: ${LIBS2}")

add_executable(consumer main.cpp)
target_link_libraries(consumer nonamespace)
""")

main = textwrap.dedent("""
#include "library.h"

int main() {
library();
}
""")

@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.

def setup_class(cls):
cls.t = t = TestClient()
# Create a library providing a build-module
t.run('new library/version -s')
t.save({'conanfile.py': cls.conanfile,
'build-module.cmake': cls.build_module})
t.run('create conanfile.py library/version@ -s build_type=Debug')
t.run('create conanfile.py library/version@ -s build_type=Release')
# Prepare project to consume the targets
t.save({'CMakeLists.txt': cls.consumer, 'main.cpp': cls.main}, clean_first=True)

@pytest.mark.tool_compiler
def test_non_multi_generator(self):
t = self.t
with t.chdir('not_multi'):
t.run('install library/version@ -g cmake_find_package -s build_type=Release')
generator = '-G "Visual Studio 15 Win64"' if platform.system() == "Windows" else ''
t.run_command(
'cmake .. {} -DCMAKE_MODULE_PATH:PATH="{}"'.format(generator, t.current_folder))
assert str(t.out).count('>> Build-module is included') == 1
assert '>> nonamespace libs: library::library' in t.out
t.run_command('cmake --build .') # Compiles and links.

@pytest.mark.skipif(platform.system() != "Windows", reason="Only windows")
@pytest.mark.tool_visual_studio
def test_multi_generator_windows(self):
t = self.t
with t.chdir('multi_windows'):
t.run('install library/version@ -g cmake_find_package_multi -s build_type=Release')
t.run('install library/version@ -g cmake_find_package_multi -s build_type=Debug')
generator = '-G "Visual Studio 15 Win64"'
t.run_command(
'cmake .. {} -DCMAKE_PREFIX_PATH:PATH="{}"'.format(generator, t.current_folder))
assert str(t.out).count('>> Build-module is included') == 2 # FIXME: Known bug
assert '>> nonamespace libs: library::library' in t.out
t.run_command('cmake --build . --config Release') # Compiles and links.

@pytest.mark.skipif(platform.system() != "Darwin", reason="Requires Macos")
@pytest.mark.tool_xcode
def test_multi_generator_macos(self):
t = self.t
with t.chdir('multi_macos'):
t.run('install library/version@ -g cmake_find_package_multi -s build_type=Release')
t.run('install library/version@ -g cmake_find_package_multi -s build_type=Debug')
t.run_command('cmake .. -G Xcode -DCMAKE_PREFIX_PATH:PATH="{}"'.format(t.current_folder))
assert str(t.out).count('>> Build-module is included') == 2 # FIXME: Known bug
assert '>> nonamespace libs: library::library' in t.out
t.run_command('cmake --build . --config Release') # Compiles and links.