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

[question] Is IMPORTED_LOCATION missing for conan_package_library_targets? #16688

Closed
1 task done
Stadik opened this issue Jul 17, 2024 · 10 comments · Fixed by #16964
Closed
1 task done

[question] Is IMPORTED_LOCATION missing for conan_package_library_targets? #16688

Stadik opened this issue Jul 17, 2024 · 10 comments · Fixed by #16964
Assignees

Comments

@Stadik
Copy link

Stadik commented Jul 17, 2024

What is your question?

I am not sure if this a bug but in conan_package_library_targets or if I am missing something
https://github.com/conan-io/conan/blob/develop2/conan/tools/cmake/cmakedeps/templates/macros.py

line 86 an imported target is generated
line 89 specifies its IMPORTED_LOCATION_RELEASE

But per CMP0111 an imported target always needs to define its IMPORTED_LOCATION, why is it not set?

Now I am getting a warning in my case due to cmake_minimum_required being smaller then 3.19

CMake Warning (dev) at 'file' (get_target_property):
  Policy CMP0111 is not set: An imported target missing its location property
  fails during generation.  Run "cmake --help-policy CMP0111" for policy
  details.  Use the cmake_policy command to set the policy and suppress this
  warning.

  IMPORTED_LOCATION not set for imported target
  "CONAN_LIB::package_library_RELEASE".

Have you read the CONTRIBUTING guide?

  • I've read the CONTRIBUTING guide
@Stadik
Copy link
Author

Stadik commented Jul 17, 2024

adding before or after line 89

set_target_properties(${_LIB_NAME} PROPERTIES IMPORTED_LOCATION ${CONAN_FOUND_LIBRARY} IMPORTED_NO_SONAME ${no_soname_mode})

Fixes the issue.

I guess this means IMPORTED_LOCATION needs to be set for any IMPORTED target, and setting its _CONFIG specification does not cancel the need to set it? Or am I missing a method of choosing the IMPORTED_CONFIGURATION?

Edit: this seems to be related to #14606

@Stadik
Copy link
Author

Stadik commented Jul 17, 2024

With conan 2.0.4 it is working, seems this was changed in
113cf5a

Some remarks in the connected issue #13504 seem to be wrong on linux, like

"The right thing is to not set IMPORTED_LOCATION at all.

Instead IMPORTED_LOCATION_DEBUG or IMPORTED_LOCATION_RELEASE shall be set to the Library for LIBRARY targets and to the dll (or so) for a shared library. For the latter IMPORTED_IMPLIB_DEBUG or IMPORTED_IMPLIB_RELEASE has to be set to the library. IMPORTED_LOCATION and IMPORTED_IMPLIB will then be automatically determined by cmake."

This is not happening, but cmake complains about IMPORTED_LOCATION being undefined.

13504 is about windows but it seems to create a problem on Linux, could this be due to single-target generator on linux and multi-target-generator on windows?

@Corristo
Copy link

Corristo commented Jul 18, 2024

You misread the explanation of CMP0111 in the CMake documentation. To quote (emphasis mine):

Imported Targets for library files and executables require that their location on disk is specified in a target property such as IMPORTED_LOCATION, IMPORTED_IMPLIB, or a per-configuration equivalent.

If configuration-specific locations are set, IMPORTED_LOCATION doesn't have to be set.

I don't know why you're getting the CMake warning, it might be due to a conan bug, but setting IMPORTED_LOCATION isn't the correct fix even if it is a conan bug.

@Stadik
Copy link
Author

Stadik commented Jul 19, 2024

"or a per-configuration equivalent." this seems to be only true under certain conditions.

After some testing I observed cmake won't throw CMP0111 when

  1. When IMPORTED_LOCATION is set as explained above.

  2. If the available CONFIGURATIONS_TYPES are set, above line 89

set_property(TARGET ${_LIB_NAME} APPEND PROPERTY IMPORTED_CONFIGURATIONS <config>)

As explained in #14606

Following this it seems only when a target has its IMPORTED_CONFIGURATIONS property set will cmake accept IMPORTED_LOCATION_CONFIG instead of the configuration-less IMPORTED_LOCATION, otherwise one is getting CMP0111

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 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.
@memsharded
Copy link
Member

Hi @Stadik

I am trying to understand where the warning comes from, trying to reproduce, but I don't get it, even if my cmake_minimum_required(VERSION 3.15)

Could you please provide fully reproducible details to get that warning?

What I have done so far:

  • Conan 2.7.1, in Windows,
  • CMake 3.25
  • conan new cmake_lib -d name=pkg -d version=0.1
  • Add in the recipe some requires, then find_package() and target_link_libraries() in the CMakeLists.txt
  • conan build .

Should I see that warning too?

@Stadik
Copy link
Author

Stadik commented Sep 30, 2024

I couldn't fully analyse this issue until know, but I explained roughly in #16705 (comment) the process of how it generates issues.

Following this, most important is that it needs to be a real physical object e.g. what cmakedeps_macros.cmake generates as
set(_LIB_NAME CONAN_LIB::${package_name}_${_LIBRARY_NAME}${config_suffix})
Further (which I only found out now) the warning is thrown when we probe for property LOCATION which should work for both unspecific (or single target generator) and specified s (or multi target generator)

I could reproduce it by this ... the hdf5 package is a random package one of our repos generated via conan, but it should work with any package which should have a physical library

cmake_minimum_required(VERSION 3.15)
project(Test)

find_package(hdf5)

### This fixes the issue by defining unspecific IMPORTED_LOCATION by the value of IMPORTED_LOCATION_<CONFIG>
#get_target_property(IMP_LOC CONAN_LIB::hdf5_hdf5_hdf5-shared_libhdf5.so_RELEASE IMPORTED_LOCATION_RELEASE)
#set_target_properties(CONAN_LIB::hdf5_hdf5_hdf5-shared_libhdf5.so_RELEASE PROPERTIES IMPORTED_LOCATION ${IMP_LOC})

### OR

### This fixes it by defining the available IMPORTED_CONFIGURATIONS
# set_target_properties(CONAN_LIB::hdf5_hdf5_hdf5-shared_libhdf5.so_RELEASE PROPERTIES IMPORTED_CONFIGURATIONS "Release")

### This generates the warning if both above are not in use
get_target_property(LOC CONAN_LIB::hdf5_hdf5_hdf5-shared_libhdf5.so_RELEASE LOCATION)

### This gives correct physical location if one of above is used otherwise CONAN_LIB::hdf5_hdf5_hdf5-shared_libhdf5.so_RELEASE-NOTFOUND
message(${LOC})

@Stadik
Copy link
Author

Stadik commented Sep 30, 2024

Should I see that warning too?

The warning simply comes because the CONAN_LIB::${package_name}_${_LIBRARY_NAME}${config_suffix} target seems to be a faulty target, there is no need to link it in any way.

@jcar87
Copy link
Contributor

jcar87 commented Sep 30, 2024

Typically the main purposes of target abstractions in CMake is to avoid precisely having to be aware of the files at all.

so target_link_libraries(mytarget PRIVATE foo::libfoo) - it shouldn't matter if libfoo itself has an associated object or not, or if it is a static library that causes even more libraries to be linked, as long as the linker and the compiler receive the right flags and the right things are actually linked.

We would be very interested in learning what the use cases are for querying the target properties at configure time. We don't typically see this in Conan Center (1700+ recipes), as it would be an anti-pattern as it breaks the encapsulation that imported targets provide. It's may also not be robust to multi-config generators - hence these sort of thing is best handled with generator expressions instead (different files for different configurations).

For the use case of gathering shared library dependencies to copy them - CMake's own built in functionality should work, e.g.

These other two variations, either:

  • $<TARGET_RUNTIME_DLLS:tgt> the target_runtime_dlls generator expression (docs here)
  • or IMPORTED_RUNTIME_ARTIFACTS in the install command (docs here)

These two may benefit from improved IMPORTED_LOCATION - if these don't work please let us know.

But otherwise, if you could provide more insights as to why it may be needed to query the location property of targets at configure time, that would be great

@Stadik
Copy link
Author

Stadik commented Sep 30, 2024

We have a cmake based dependency walker from a time before any of the options you mentioned existed, for this we probe IMPORTED_LOCATION, LOCATION or IMPORTED_LOCATION_CONFIG if CONFIGURATION_TYPES / IMPORTED_CONFIGURATIONS are defined.

We tried in the past modernizing this code by said built in functionalities, but all proved to have conditional bugs / miss some important logic or something similar, so they generate faulty packages under certain conditions without additional effort and workarounds. So it seems not worth the effort as long as any of the functionalities used are not fully deprecated.

But apart from the reason why, did I make it clear that the failure comes from simple get_target_property of a base property LOCATION?
As I said I didn't fully analyse this issue, but it seems like the failure of extracting LOCATION is because it internally also probes either IMPORTED_LOCATION or IMPORTED_LOCATION_CONFIG for all CONFIG in IMPORTED_CONFIGURATIONS (or something along that line)
So I would expect other functions in cmake could also use need IMPORTED_CONFIGURATIONS to be defined together with IMPORTED_LOCATION_CONFIG or similar properties

I explained our reason why IMPORTED_CONFIGURATIONS for the current definition of the CONAN_LIB targets in #16705 (comment)

@memsharded memsharded self-assigned this Oct 18, 2024
@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