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] Linking a package with two dlls in the lib directory fails #14225

Closed
musto opened this issue Jul 5, 2023 · 6 comments · Fixed by #14253
Closed

[bug] Linking a package with two dlls in the lib directory fails #14225

musto opened this issue Jul 5, 2023 · 6 comments · Fixed by #14253
Assignees
Milestone

Comments

@musto
Copy link

musto commented Jul 5, 2023

Environment details

  • Operating System+version: Windows 10
  • Compiler+version: msvc 193
  • Conan version: 2.0.6
  • Python version: 3.10

Steps to reproduce

  1. Export a prebuilt package on windows with following characteristics:
  • The conanfile has either option shared=True or package_type = "shared-library"
  • The package has at least two dlls with import libs.
  • The dlls and import libs copied to the same lib/ directory inside the package.
  1. Use the package in another project that uses CMakeDeps generator and Visual Studio.
  2. The build fails because Visual Studio will try to link to the second dll instead of the second import lib.

For example with "prebuilt_local_project" conan tutorial modified to contain 2 dlls:
C:\conan_examples2\tutorial\creating_packages\other_packages\prebuilt_local_project>conan new cmake_lib -d name=hello -d version=0.1

Modify CMakeLists.txt:

project(hello2 CXX)
add_library(hello2 src/hello.cpp)
target_include_directories(hello2 PUBLIC include)
set_target_properties(hello2 PROPERTIES PUBLIC_HEADER "include/hello.h")

Modify conanfile.py:

import os
from conan.tools.files import copy

    def package(self):
        copy(self, pattern="*.lib", src=self.build_folder, dst=os.path.join(self.package_folder, "lib"), keep_path=False)
        copy(self, pattern="*.dll", src=self.build_folder, dst=os.path.join(self.package_folder, "lib"), keep_path=False)
        copy(self, pattern="*.h", src=self.recipe_folder, dst=self.package_folder, keep_path=True)
    def package_info(self):
        self.cpp_info.libs = ["hello", "hello2"]
        self.cpp_info.bindirs = ["lib"]

C:\conan_examples2\tutorial\creating_packages\other_packages\prebuilt_local_project>conan build . -s build_type=Debug -o shared=True
C:\conan_examples2\tutorial\creating_packages\other_packages\prebuilt_local_project>conan export-pkg . -s build_type=Debug -o shared=True

...
hello/0.1 (test package): RUN: cmake --build "C:\conan_examples2\tutorial\creating_packages\other_packages\prebuilt_local_project\test_package\build\msvc-193-x86_64-14-debug" --config Debug
MSBuild version 17.5.1+f6fdcf537 for .NET Framework
  Checking Build System
  Building Custom Rule C:/conan_examples2/tutorial/creating_packages/other_packages/prebuilt_local_project/test_package/CMakeLists.txt
  example.cpp
C:\Users\tmusto\.conan2\p\b\hellocbc01ad6d9aa3\p\lib\hello2.dll : fatal error LNK1107: invalid or corrupt file: cannot read at 0x340 [C:\conan_examples2\tutorial\creating_packages\other_packages\prebuilt_local_project\test_package\build\msvc-193-x86_64-14-debug\example.vcxproj]

Logs

No response

@musto
Copy link
Author

musto commented Jul 5, 2023

The problem is that in conan_package_library_targets every iteration adds .dll to CMAKE_FIND_LIBRARY_SUFFIXES that leaks to subsequent calls to find_library

set(CMAKE_FIND_LIBRARY_SUFFIXES .dll ${CMAKE_FIND_LIBRARY_SUFFIXES})

@memsharded
Copy link
Member

Hi @musto

Thanks for your report.

I'd say this is not really a bug, but the problem is self.cpp_info.bindirs = ["lib"] and doing the copy() of the .dll to the "lib" folder instead of the "bin" folder. You are packaging the binaries in the libraries directories. Windows DLLs are considered runtime libraries, not compile time libraries and they must go to the "bin" directory (or at least not put together with the importing .lib libraries). I'd strongly recommend to follow the recommended practices and common layouts, and not going against them. If you have another need or issue that you are trying to solve by doing this, maybe file a new ticket to ask for advice and discuss the issue.

Said that, I am not against of making sure that CMAKE_FIND_LIBRARY_SUFFIXES change doesn't leak beyond its intended usage. As you already did the research, would you like to contribute a PR yourself? Storing and resetting the value of CMAKE_FIND_LIBRARY_SUFFIXES after the find_library() would be enough. Because testing this would be quite heavy, as long as it doesn't break anything else, I am good with that change, no need for extra testing.

@musto
Copy link
Author

musto commented Jul 6, 2023

It is Visual Studio default behavior to generate the dll and lib files to the same directory when building the project.
So the same issue happens also when requiring a conan editable package that is using cmake_layout(self).

@memsharded memsharded added this to the 2.0.8 milestone Jul 6, 2023
@memsharded
Copy link
Member

It is Visual Studio default behavior to generate the dll and lib files to the same directory when building the project.
So the same issue happens also when requiring a conan editable package that is using cmake_layout(self).

Sure, you are right about the editables. Still, it is recommended that in the final package the location of the DLLs would be the bin folder and not the lib folder, as that is the most used and standard layout.

Regarding my comment above, as you already did the research and identified the location and the leak, would you like to contribute the fix yourself with a PR? That is just an offer, please let us know if you don't, and we will manage. Thanks!

@musto
Copy link
Author

musto commented Jul 8, 2023

Regarding my comment above, as you already did the research and identified the location and the leak, would you like to contribute the fix yourself with a PR?

I would appreciate it if someone else made a PR. Thank you.

AbrilRBS added a commit that referenced this issue Jul 8, 2023
Changelog: Bugfix: fix leaking of ``CMakeDeps``
``CMAKE_FIND_LIBRARY_SUFFIXES`` variable
Docs: Omit

Close #14225
@memsharded
Copy link
Member

Fixed in #14253, it will be in next 2.0.8 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants