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] In the CMakeDeps Generator IMPORTED_LOCATION is overridden by IMPORTED_LOCATION${config_suffix} #13504

Closed
DanielDewald opened this issue Mar 22, 2023 · 6 comments · Fixed by #13841
Milestone

Comments

@DanielDewald
Copy link

DanielDewald commented Mar 22, 2023

Environment details

  • Operating System+version: Windows 10
  • Compiler+version: MSVC 19
  • Conan version: 2.0.2
  • Python version: 3.10

Steps to reproduce

  1. Download a dependency (e.g. thrift) with conan using the CMakeDeps generator
  2. Have a look at the created cmakedeps_macros.cmake

In the function conan_package_library_targets line 48 and 49 for windows the correct target properties are set for IMPORTED_IMPLIB and IMPORTED_LOCATION.

set_target_properties(${_LIB_NAME} PROPERTIES IMPORTED_LOCATION ${CONAN_SHARED_FOUND_LIBRARY}) set_target_properties(${_LIB_NAME} PROPERTIES IMPORTED_IMPLIB ${CONAN_FOUND_LIBRARY})

Unfortunately those values are superseded by the line 61 where a per config value for IMPORTED_LOCATION_${config_suffix} is set, making the previous setting useless.

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

  1. Create a cmake project using

add_custom_command(TARGET mytarget POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy_if_different $<TARGET_RUNTIME_DLLS:mytarget> $<TARGET_FILE_DIR:mytarget>)

  1. Instead of copying the dlls files to the binary folder upon building, the .lib files are copied.

Logs

No response

@hgraeber
Copy link

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.

@Untzelmann
Copy link
Contributor

Hey,

I have a fix for it here.
fix-cmakedeps-import-library-config-suffix.patch

I also tried to upload this into a branch, to start a pull request, but it seems like I have no permissions for that. Could somebody grant this permission to me, so that I can upload this change?

@memsharded
Copy link
Member

I also tried to upload this into a branch, to start a pull request, but it seems like I have no permissions for that. Could somebody grant this permission to me, so that I can upload this change?

Hi @Untzelmann

you cannot upload branches to the main repo, you need to push your branches to your Github fork, and do a PRs from your fork. That is the contribution flow to Conan, we all, including the maintainers, do PRs from our forks, not from the central repo.

Untzelmann added a commit to Untzelmann/conan that referenced this issue May 8, 2023
Currently the imported libraries are set without config suffix first.
At the end there is one additional set using the config suffix, which
overwrites the already set libraries.

Fixes conan-io#13504
@Untzelmann
Copy link
Contributor

Sorry, I actually never really worked on github, so this is all new to me.

I think this time I got it to work: #13841

@memsharded
Copy link
Member

Sorry, I actually never really worked on github, so this is all new to me.

No prob, thanks for wanting to contribute :) Feel free to ask any further question that you might have.

memsharded pushed a commit that referenced this issue May 12, 2023
* CMakeDeps: Fix imported library config suffix

Currently the imported libraries are set without config suffix first.
At the end there is one additional set using the config suffix, which
overwrites the already set libraries.

Fixes #13504

* cmakedeps: add test to ensure CMake TARGET_RUNTIME_DLLS works

* test_cmake_target_runtime_dlls: requires cmake 3.21

* Update conans/test/functional/toolchains/cmake/cmakedeps/test_cmakedeps.py

---------

Co-authored-by: Luis Caro <3535649+jcar87@users.noreply.github.com>
Co-authored-by: Francisco Ramírez <franchuti688@gmail.com>
@memsharded
Copy link
Member

Fixed by #13841 by @Untzelmann , will be in next 2.0.5

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