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

[bug] conan_package_library_targets does not set IMPORTED_CONFIGURATIONS #14606

Closed
ilya-lavrenov opened this issue Aug 29, 2023 · 7 comments · Fixed by #16964
Closed

[bug] conan_package_library_targets does not set IMPORTED_CONFIGURATIONS #14606

ilya-lavrenov opened this issue Aug 29, 2023 · 7 comments · Fixed by #16964

Comments

@ilya-lavrenov
Copy link

Environment details

  • Operating System+version: macOS
  • Conan version: 1.60
  • Python version: 3.11

Steps to reproduce

Install TBB with Conan and observe cmakedeps_macros.cmake file. It contains

function(conan_package_library_targets libraries package_libdir deps_target out_libraries_target config_suffix package_name)
    set(_out_libraries_target "")

    foreach(_LIBRARY_NAME ${libraries})
        find_library(CONAN_FOUND_LIBRARY NAMES ${_LIBRARY_NAME} PATHS ${package_libdir}
                     NO_DEFAULT_PATH NO_CMAKE_FIND_ROOT_PATH)
        if(CONAN_FOUND_LIBRARY)
            message(VERBOSE "Conan: Library ${_LIBRARY_NAME} found ${CONAN_FOUND_LIBRARY}")

            # Create a micro-target for each lib/a found
            # Allow only some characters for the target name
            string(REGEX REPLACE "[^A-Za-z0-9.+_-]" "_" _LIBRARY_NAME ${_LIBRARY_NAME})
            set(_LIB_NAME CONAN_LIB::${package_name}_${_LIBRARY_NAME}${config_suffix})
            if(NOT TARGET ${_LIB_NAME})
                # Create a micro-target for each lib/a found
                add_library(${_LIB_NAME} UNKNOWN IMPORTED)
            endif()
            # Link library file
            set_target_properties(${_LIB_NAME} PROPERTIES IMPORTED_LOCATION${config_suffix} ${CONAN_FOUND_LIBRARY})
            list(APPEND _out_libraries_target ${_LIB_NAME})
            message(VERBOSE "Conan: Found: ${CONAN_FOUND_LIBRARY}")
        else()
            message(FATAL_ERROR "Library '${_LIBRARY_NAME}' not found in package. If '${_LIBRARY_NAME}' is a system library, declare it with 'cpp_info.system_libs' property")
        endif()
        unset(CONAN_FOUND_LIBRARY CACHE)
    endforeach()

    # Add the dependencies target for all the imported libraries
    foreach(_T ${_out_libraries_target})
        set_property(TARGET ${_T} PROPERTY INTERFACE_LINK_LIBRARIES ${deps_target} APPEND)
    endforeach()

    set(${out_libraries_target} ${_out_libraries_target} PARENT_SCOPE)
endfunction()

Which sets IMPORTED_LOCATION_<config>, but does not set IMPORTED_CONFIGURATIONS.

As a I users, sometimes I need an access to library location. So, I have to understand imported configurations and then take appropriate IMPORTED_LOCATION_<config>, but Conan does not do it.

It's a good practice to set this property (done by cmake automatically)

Logs

No response

@memsharded
Copy link
Member

Hi @ilya-lavrenov

Thanks for reporting. I think this is a known issue, would be a duplicated of #12654. I'd recommended closing this issue as duplicated and centralized discussion in the other one.

In short: this is a limitation of the available information in the cpp_info of the dependencies, for shared libraries, as well as the CMakeDeps target creation. There is pending a new model and CMakeDeps implementation for the 2.X roadmap.

@marlamb
Copy link
Contributor

marlamb commented Oct 4, 2023

Hi @memsharded

I might be mistaken, but in the linked issue it is mainly about the IMPORTED_LOCATION_<CONFIG> , which is from my point of view already solved by the ${config_suffix} in the version of the file posted by @ilya-lavrenov

set_target_properties(${_LIB_NAME} PROPERTIES IMPORTED_LOCATION${config_suffix} ${CONAN_FOUND_LIBRARY})

But what I am lacking as well is the IMPORTED_CONFIGURATIONS property, such that it is possible in user CMake code to first query the available configurations and then ask for the respective imported location. One example could look like this, which looks like a reasonable logic for me to resolve imported targets.

A possible patch could look like this:

string(REGEX_REPLACE "^_" "" config_tmp ${config_suffix})
string(TOUPPER ${config_tmp} config)
set_target_properties(${_LIB_NAME}
  PROPERTIES
    IMPORTED_LOCATION${config_suffix} ${CONAN_FOUND_LIBRARY}
    IMPORTED_CONFIGURATIONS ${config}
)

There are surely various ways to write this in a nicer way (e.g. by not having _ as part of the config_suffix), however I hope the snippet shows the intention and as far as I can see it should be also sufficiently explicit to not disturb any other use-cases (but my range of vision is unfortunately still very short for conan 😆 ).

@Stadik
Copy link

Stadik commented Jul 19, 2024

The missing IMPORTED_CONFIGURATIONS is leading to CMP0111 for me as explained in #16688

marlamb pushed a commit to marlamb/conan that referenced this issue Jul 20, 2024
Some frameworks use the property in order to find libraries, e.g. like
https://github.com/ros/catkin/blob/noetic-devel/cmake/catkin_libraries.cmake#L150

In order to support these kind of usages, this commit adds the property
to all targets.

This should solve conan-io#14606 and conan-io#16688.
@marlamb
Copy link
Contributor

marlamb commented Jul 20, 2024

I created a PR #16705 in order to (hopefully) solve this issue. Following the contribution guide I want to ask @conan-io/barbarians to "queue" this issue (although I have to admit, I don't exactly know what this means in this context).

@memsharded
Copy link
Member

@marlamb regarding your comment that some tools like catkin is requiring this, maybe you want to be aware that we are working in a ros2 integration (cc @danimtb), with rosdep & colcon, not catkin

marlamb pushed a commit to marlamb/conan that referenced this issue Jul 21, 2024
Some frameworks use the property in order to find libraries, e.g. like
https://github.com/ros/catkin/blob/noetic-devel/cmake/catkin_libraries.cmake#L150

In order to support these kind of usages, this commit adds the property
to all targets.

This should solve conan-io#14606 and conan-io#16688.
@marlamb
Copy link
Contributor

marlamb commented Jul 21, 2024

@memsharded that sounds great, I really appreciate!

Indeed the lifetime of catkin ends soon with the latest distros still supporting it running out of support (e.g. Ubuntu 20.04 next year). I just took it as an example of a logic that independent of the example looks is a valid CMake logic, that should be supported by conan.

@memsharded
Copy link
Member

We are releasing in Conan 2.9 a completely new CMakeDeps generator in #16964 that has closed this ticket, with many pending features and fixes:

  • Allow defining cpp_info.default_components and using that information to generate CMake targets by default for packages
  • Allow defining cpp_info.exe to model executable imported targed generation
  • Creating always actual SHARED/STATIC/INTERFACE imported targets, with their IMPORTED_LOCATION, the IMPORTED_IMPLIB, the IMPORTED_CONFIGURATIONS
  • This solves issues with try_compile and other CMake source-compiling checks
  • No artificial library or package targets
  • Allow using the package languages attribute or a new cpp_info.languages information to define IMPORTED_LINK_INTERFACE_LANGUAGES
  • Define config files for executable targets for the "build" context automatically, without any special configuration
  • Use the correct CONFIGURATION from the package instead of the consumer one
  • Generate a new conan_cmakedeps_paths.cmake file, that can be used for integrations that cannot use a CMakeToolchain, like cmake-conan
  • Allow using executable targets from the host context if not cross-building (and not tool-requires that would prioritize those executable targets)

Current known pending functionality (to be added soon):

  • Not managing Apple frameworks
  • Not generating find modules, only CMake Config files

The new CMakeDeps generator is intended for testing and validation only, being a transparent replacement of the old one, so it is behind a new conf. To use it, use the -c tools.cmake.cmakedeps:new=will_break_next, and that will use the new generator instead of the old one. Note the will_break_next value means exactly that, that value will change in next release to force a break, so no one can depend on this generator in production yet.

Your feedback is very important

As this is a major change, we will only remove the conf gate when we get confirmation from users that it works and solve the issues. Please try the new generator for your project, and let us know if it works. If it doesn't, please re-open this ticket and let us know what failed. Thanks very much!

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 a pull request may close this issue.

4 participants