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

Bugfixes 11743 and 11755 #11756

Merged
merged 2 commits into from
Aug 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 8 additions & 14 deletions conan/tools/cmake/cmakedeps/templates/target_configuration.py
Expand Up @@ -65,26 +65,14 @@ def template(self):
set(_{{ pkg_name }}_DEPENDENCIES{{ config_suffix }} "{{ deps_targets_names }}")

set({{ pkg_name }}_LIBRARIES_TARGETS{{ config_suffix }} "") # Will be filled later
set({{ pkg_name }}_LIBRARIES{{ config_suffix }} "") # Will be filled later
conan_package_library_targets("{{ '${' }}{{ pkg_name }}_LIBS{{ config_suffix }}}" # libraries
"{{ '${' }}{{ pkg_name }}_LIB_DIRS{{ config_suffix }}}" # package_libdir
"{{ '${' }}_{{ pkg_name }}_DEPENDENCIES{{ config_suffix }}}" # deps
{{ pkg_name }}_LIBRARIES_TARGETS # out_libraries_targets
"{{ config }}" # DEBUG, RELEASE ...
"{{ pkg_name }}") # package_name

# The XXXX_LIBRARIES_RELEASE/DEBUG is used for the module (FindXXX.cmake)
foreach(_FRAMEWORK {{ '${' }}{{ pkg_name }}_FRAMEWORKS_FOUND{{ config_suffix }}})
list(APPEND {{ pkg_name }}_LIBRARIES{{ config_suffix }} ${_FRAMEWORK})
endforeach()

foreach(_SYSTEM_LIB {{ '${' }}{{ pkg_name }}_SYSTEM_LIBS{{ config_suffix }}})
list(APPEND {{ pkg_name }}_LIBRARIES{{ config_suffix }} ${_SYSTEM_LIB})
endforeach()

# We need to add our requirements too
set({{ pkg_name }}_LIBRARIES{{ config_suffix }} "")
list(APPEND {{ pkg_name }}_LIBRARIES{{ config_suffix }} {{ deps_targets_names }})

# FIXME: What is the result of this for multi-config? All configs adding themselves to path?
set(CMAKE_MODULE_PATH {{ '${' }}{{ pkg_name }}_BUILD_DIRS{{ config_suffix }}} {{ '${' }}CMAKE_MODULE_PATH})
set(CMAKE_PREFIX_PATH {{ '${' }}{{ pkg_name }}_BUILD_DIRS{{ config_suffix }}} {{ '${' }}CMAKE_PREFIX_PATH})
Expand Down Expand Up @@ -121,6 +109,9 @@ def template(self):
$<$<CONFIG:{{configuration}}>:${{'{'}}{{pkg_name}}_LIB_DIRS{{config_suffix}}}> APPEND)
{%- endif %}

# For the modules (FindXXX)
list(APPEND {{ pkg_name }}_LIBRARIES{{ config_suffix }} {{root_target_name}})
Copy link
Contributor Author

@lasote lasote Aug 2, 2022

Choose a reason for hiding this comment

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

Note, here I'm attaching the target instead of the path to the libraries, like in previous versions. That is because the macro conan_package_library_targets was simplified and only returns the targets.

When someone uses the Boost_LIBRARIES is typically to link a target with them so it should be ok and it is wide easier in code, and also, the transitive dependencies of the package were always added as targets to the list.


{%- else %}

########## COMPONENTS TARGET PROPERTIES {{ configuration }} ########################################
Expand Down Expand Up @@ -163,6 +154,9 @@ def template(self):
{{tvalue(pkg_name, comp_variable_name, 'COMPILE_OPTIONS_CXX', config_suffix)}}> APPEND)
set({{ pkg_name }}_{{ comp_variable_name }}_TARGET_PROPERTIES TRUE)

# For the modules (FindXXX)
list(APPEND {{ pkg_name }}_LIBRARIES{{ config_suffix }} {{comp_target_name}})

{%- if set_interface_link_directories %}
# This is only used for '#pragma comment(lib, "foo")' (automatic link)
set_property(TARGET {{ comp_target_name }} PROPERTY INTERFACE_LINK_DIRECTORIES
Expand All @@ -175,7 +169,7 @@ def template(self):
########## AGGREGATED GLOBAL TARGET WITH THE COMPONENTS #####################
{%- for comp_variable_name, comp_target_name in components_names %}

target_link_libraries({{root_target_name}} INTERFACE {{ comp_target_name }})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what it was causing the #11755 error. When using an add_subdirectory the target exists but fails to use target_link_libraries. This is apparently related to the scope of the target. Creating the targets as INTERFACE GLOBAL worked too but looked more dangerous.

set_property(TARGET {{root_target_name}} PROPERTY INTERFACE_LINK_LIBRARIES {{ comp_target_name }} APPEND)

{%- endfor %}

Expand Down
Expand Up @@ -322,3 +322,63 @@ def build(self):
'$<$<STREQUAL:$<TARGET_PROPERTY:TYPE>,EXECUTABLE>:>>') in t.out
# NOTE: If there is no "conan install -s build_type=Debug", the properties won't contain the
# <CONFIG:Debug>


@pytest.mark.tool_cmake
def test_cmake_add_subdirectory():
"""https://github.com/conan-io/conan/issues/11743
https://github.com/conan-io/conan/issues/11755"""

t = TestClient()
boost = textwrap.dedent("""
from conan import ConanFile

class Consumer(ConanFile):
name = "boost"
version = "1.0"

def package_info(self):
self.cpp_info.set_property("cmake_file_name", "Boost")
self.cpp_info.components["A"].system_libs = ["A_1", "A_2"]
self.cpp_info.components["B"].system_libs = ["B_1", "B_2"]
""")
t.save({"conanfile.py": boost})
t.run("create .")
conanfile = textwrap.dedent("""
from conans import ConanFile
from conan.tools.cmake import CMake, cmake_layout

class Consumer(ConanFile):
name = "consumer"
version = "0.1"
requires = "boost/1.0"
generators = "CMakeDeps", "CMakeToolchain"
settings = "os", "arch", "compiler", "build_type"

def layout(self):
cmake_layout(self)

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

cmakelists = textwrap.dedent("""
cmake_minimum_required(VERSION 3.15)
project(hello CXX)
find_package(Boost CONFIG)
add_subdirectory(src)

""")
sub_cmakelists = textwrap.dedent("""
find_package(Boost REQUIRED COMPONENTS exception headers)

message("AGGREGATED LIBS: ${Boost_LIBRARIES}")
""")

t.save({"conanfile.py": conanfile,
"CMakeLists.txt": cmakelists, "src/CMakeLists.txt": sub_cmakelists})
t.run("install .")
# only doing the configure failed before #11743 fix
t.run("build .")
assert "AGGREGATED LIBS: boost::B;boost::A" in t.out
Copy link
Contributor Author

@lasote lasote Aug 2, 2022

Choose a reason for hiding this comment

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

These are the targets, in previous versions of Conan in that variable appeared the path to the libraries files, except for the dependencies that were also targets and are kept as targets

Expand Up @@ -167,6 +167,10 @@ def build(self):
message("MYPKGB_INCLUDE_DIRS: ${{MYPKGB_INCLUDE_DIRS}}")
message("MYPKGB_INCLUDE_DIR: ${{MYPKGB_INCLUDE_DIR}}")
message("MYPKGB_LIBRARIES: ${{MYPKGB_LIBRARIES}}")

get_target_property(linked_libs pkgb::pkgb INTERFACE_LINK_LIBRARIES)
message("MYPKGB_LINKED_LIBRARIES: '${{linked_libs}}'")

message("MYPKGB_DEFINITIONS: ${{MYPKGB_DEFINITIONS}}")
""")

Expand All @@ -183,7 +187,11 @@ def build(self):
assert "MYPKGB_VERSION_STRING: 1.0" in client.out
assert "MYPKGB_INCLUDE_DIRS:" in client.out
assert "MYPKGB_INCLUDE_DIR:" in client.out
assert "MYPKGB_LIBRARIES: pkga::pkga" in client.out
# The MYPKG_LIBRARIES contains the target for the current package, but the target is linked
# with the dependencies also:
assert "MYPKGB_LIBRARIES: pkgb::pkgb" in client.out
assert "MYPKGB_LINKED_LIBRARIES: '$<$<CONFIG:Release>:>;$<$<CONFIG:Release>:pkga::pkga>" in client.out

assert "MYPKGB_DEFINITIONS: -DDEFINE_MYPKGB" in client.out
assert "Conan: Target declared 'pkga::pkga'"

Expand Down
Expand Up @@ -114,7 +114,8 @@ def package_info(self):
$<$<CONFIG:Release>:${hello_OBJECTS_RELEASE}>
${hello_LIBRARIES_TARGETS}""" not in content
# But the global target is linked with the targets from the components
assert "target_link_libraries(hello::hello INTERFACE hello::say)" in content
assert "set_property(TARGET hello::hello PROPERTY " \
"INTERFACE_LINK_LIBRARIES hello::say APPEND)" in content

with open(os.path.join(client.current_folder, "hello-release-x86_64-data.cmake")) as f:
content = f.read()
Expand Down