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] Why is full_deploy skipping binaries #14541

Closed
1 task done
tgurriet opened this issue Aug 22, 2023 · 32 comments · Fixed by #14673
Closed
1 task done

[question] Why is full_deploy skipping binaries #14541

tgurriet opened this issue Aug 22, 2023 · 32 comments · Fixed by #14673
Assignees
Milestone

Comments

@tgurriet
Copy link

What is your question?

Hello,

I have a quick question about the full_deploy deployer in conan2.

In particular, it seems to skip generating CMake config files for some of my dependencies:

Requirements
    abseil/20230125.3#5431a4c609f5fb48bb8d8567e953243f:10ea01c153d79b79b48eb4b2179aea156c8ced32#9a185abaa0f7bc0dcbc8d8508036f41b - Cache
    cityhash/cci.20130801#60d7fcb16cbac5004d688486161ba05d:10ea01c153d79b79b48eb4b2179aea156c8ced32#adc7fef25d909c36936e60d7e15c25cf - Cache
    clickhouse-cpp/2.4.0#df2bc418dda2bbefaa0dc67ae9380618:76aa131b1fda412b23c10873062af85b8a1af809#5b9d9767814643544b06975527a28d0e - Cache
    lz4/1.9.4#bce1f314775b83c195dffc8e177ff368:d152c3753ced4eedbb7024e2e948e5f19a896716#b74799b113fbbd585981d17d8896f904 - Cache
    openssl/3.1.1#3a25e05b364f335633143656dc265841:b24936b82648bf158642bac0d8eadcb966155d54#989d2e84562e8cb7e90459084e2d4595 - Cache
    stduuid/1.2.3#c21050b4815449b12b3adb16ec6aef51:da39a3ee5e6b4b0d3255bfef95601890afd80709#3088b5244288f1db735498b1377f863e - Cache
    yaml-cpp/0.7.0#85b409c274a53d226b71f1bdb9cb4f8b:d28f5a842b6e2762ba1af59efe511e1859302f93#ece6b226512c888f9806c2d46c91f655 - Cache
    zlib/1.2.13#e377bee636333ae348d51ca90874e353:d152c3753ced4eedbb7024e2e948e5f19a896716#8450f6ee7a36eb99058ce01d76450ad1 - Cache
Test requirements
Build requirements
Skipped binaries
    boost/1.79.0, cpp-httplib/0.12.2, eigen/3.4.0, fmt/10.0.0, catch2/2.13.9, gtest/1.10.0, autoconf/2.71, automake/1.16.5, gnu-config/cci.20210814, libtool/2.4.7, m4/1.4.19


What controls which packages get exported during a full_deploy?

Thanks in advance,
Tom

Have you read the CONTRIBUTING guide?

  • I've read the CONTRIBUTING guide
@memsharded
Copy link
Member

Hi @tgurriet

Conan, when installing dependencies, knows that some binaries are not necessary. For example, if some package needs a tool_requires, but there is already a binary for that package, it doesn't need to build from source, and then it will not need those tool_requires. Same for library dependencies that are being "embedded" into another binary, for example, if you depend on some executable binary that exists, and that links with a static library, it is not necessary to retrieve the static library, as it has already been linked into the executable.

The idea is that the conan install --deployer=full_deploy will only deploy the binaries that you need. If you add the --build=*, for example, it will use all the binaries as all will be necessary. Also next Conan 2.0.10 is getting a conf to avoid the skipping of binaries, check: #14466

@tgurriet
Copy link
Author

tgurriet commented Aug 23, 2023

Good to know, thanks!

In our case it's an issue because cpp-httplib is a header-only library present in the public interface of our static library and requiring OpenSSL.
It therefore needs to be deployed so its headers can be found by our consumer application, and the consumer application can link to openSSL.
When adding the --build=* to the deploy command, it does indeed add the deployed path to the "CMAKE_INCLUDE_PATH" in the conan_toolchain.cmake for the headers to be found.
However, the CMakeDeps generator does not generate a Findhttplib.cmake of httplib-config.cmake.
Nor does it link our library to httplib in the generated OurLib-Target-debug.cmake, creating a linking error about missing OpenSSL symbols.

Wouldn't it make more sense for the default behavior to keep the complete dependency tree during deployment, unless explicitly allowed via requires traits (visible=False, transitive_header=False, etc...), or are we missing something in how we deploy our package.

Maybe it's the package_type which is misleading, and packages flagged as header-library should not have any dependencies? In which case it should maybe be enforced to not let faulty packages exist in the wild!?

@memsharded
Copy link
Member

In our case it's an issue because cpp-httplib is a header-only library present in the public interface of our static library and requiring OpenSSL.
It therefore needs to be deployed so its headers can be found by our consumer application, and the consumer application can link to openSSL.

If this is the case, it is important to mark it with transitive_headers=True, not sure if you did it. If you mark it that way, it is possible that it is not skipped.

However, the CMakeDeps generator does not generate a Findhttplib.cmake of httplib-config.cmake.
Nor does it link our library to httplib in the generated OurLib-Target-debug.cmake, creating a linking error about missing OpenSSL symbols.

This shouldn't be the case if transitive_headers=True. If this is happening, could we please have some more details to reproduce?

Wouldn't it make more sense for the default behavior to keep the complete dependency tree during deployment, unless explicitly allowed via requires traits (visible=False, transitive_header=False, etc...), or are we missing something in how we deploy our package.

This seems not to be the preferred approach, and according to SW engineering, the transitive dependencies shouldn't be able to be explicitly #included by consumers, unless consumers define a direct depenendy. This is explained in this talk: https://youtu.be/kKGglzm5ous

Maybe it's the package_type which is misleading, and packages flagged as header-library should not have any dependencies?

Not sure what you mean. Header-only libraries often have dependencies, from other header-onlys, but also depend on other compiler libraries.

@tgurriet
Copy link
Author

tgurriet commented Aug 23, 2023

This seems not to be the preferred approach, and according to SW engineering, the transitive dependencies shouldn't be able to be explicitly #included by consumers, unless consumers define a direct dependency.

You mean that the public headers of my library should not include any header of any of its dependencies? Seems a bit restrictive!? Could you point to where in the talk you address this point?

Indeed transitive_headers=True does the job of making the CMakeDeps generator exposes that dependency downstream after deployment through both linking at the target level, and creation of a config.cmake.

I'm not sure this completely solves my problem though. Let's say I remove cpp-httplib from my public interface, my consumer still needs to link to OpenSSL.

The doc states for transitive_headers: If True the headers of the dependency will be visible downstream.

So in this case it would feel legitimate to have transitive_headers=False, but this wouldn't propagate the httplib dependency and hence OpenSSL downstream.

The doc also states (likely with a typo!?) for transitive_libs: If True the libraries to link with of the dependency will be visible downstream.

It although not 100% obvious, it would sound reasonable having to set transitive_libs=True to propagate library dependencies of header only libraries, but only transitive_headers=True seems to do the job.

Or would it be at the level of the cpp-httplib recipe that transitive_lib should be set to true for OpenSSL, which at the moment isn't the case?

@memsharded
Copy link
Member

Libs are automatically transitive when a static library depends on another static library. It is only not propagated when it is a shared library linking a static library, because the shared library will embed that static library, which by default will be an internal implementation detail of the shared library, unless we explicitly mark it as transitive_libs=True.

It although not 100% obvious, it would sound reasonable having to set transitive_libs=True to propagate library dependencies of header only libraries, but only transitive_headers=True seems to do the job.

header-only libraries already always define transitive_headers=True and transitive_libs=True, because obviously they can't embed the dependencies

Or would it be at the level of the cpp-httplib recipe that transitive_lib should be set to true for OpenSSL, which at the moment isn't the case?

The transitive_libs=True is only necessary in some exceptional cases in which the transitive headers that are public have some linkage requirements to the lib, and are not a "pure" interface thing.

Please let me know if this clarifies a bit more.

@tgurriet
Copy link
Author

tgurriet commented Aug 23, 2023

It does clarifies things a bit, but something is still off in the case I'm talking about.

I've created a little test for you to see what I'm talking about: https://github.com/tgurriet/conan_deploy_test

You can just execute run.sh, and you'll see the error I'm talking about.

Thanks for your help!

@tgurriet
Copy link
Author

@memsharded Should I maybe create an issue for this given that it's doesn't seem to be about skipping binaries but deployers

@memsharded
Copy link
Member

Thanks for the project with the code to reproduce, it really helps.

I think the main issue is that you are installing the default dependencies, which are -s build_type=Release, but then trying to compile the app with Debug time. Either install the -s build_type=Debug dependencies, or build your app in Release. Please try that and let us know.

@tgurriet
Copy link
Author

@memsharded Sorry about the typo. I updated the example, but it still doesn't work 😞

@memsharded
Copy link
Member

Why harcoding the .a libname? In most cases it is good (and recommended) to:

    def package_info(self):
        self.cpp_info.libs = ["my_lib"]

No need to define the properties either, those are the default already. Changing it I managed to move forward, but not completely, but got a final linkage error of SSL things, need to check that

@tgurriet
Copy link
Author

tgurriet commented Aug 25, 2023

Good to know, thanks for the info. I've updated the test repo.

It seems to me that the issue is with the full_deployer assuming that header only dependencies don't need to be propagated to the consumer by default.
This is not the case as even though they should get consumed by a static or dynamic libraries, they might still have dependencies linking requirements that won't get embedded in the case of a static library.

@memsharded
Copy link
Member

Lets get rid of the deployer just to try. What happens if you do the same but without the deployer, just linking with the Cache packages?

  • If it works, it might be a deployer thing
  • If it doesnt' work, it is possible that the usage of transitive dependencies is missing some qualifiers like self.requires("cpphttlib", transitive_headers=True) which is necessary if your own library includes and exposes the dependencies headers in the public headers of your library.

@tgurriet
Copy link
Author

My apologies for previous deleted message, spoke too fast.
It does not work even without deployers...

@tgurriet
Copy link
Author

tgurriet commented Aug 25, 2023

@memsharded I do confirm that it works in both cases when using self.requires("cpp-httplib/0.12.2", transitive_headers=True).

But as you can see, my_lib doesn't "expose the dependencies headers in the public header". They are consumed in my_lib.cpp. Only the dependency on openSSL needs to be propagated downstream.

Maybe there should be another qualifier like transitive_dependencies for such cases. Or the logic for what "embedded" means should be revised.

@tgurriet
Copy link
Author

tgurriet commented Sep 5, 2023

@memsharded It seems like it's not a problem with the deployer but a fundamental issue with how header only libraries are considered in the dependency transitivity model of conan. Should I create a specific feature request or bug issue about it?

@memsharded
Copy link
Member

Hi @tgurriet

I think we can follow in this same thread, not sure if if is fundamentally different.
It would be good to summarize the case though, if you could please try to provide some minimal reproducible case, with some conanfiles that we could reproduce the issue, that would help a lot. Thanks!

@tgurriet
Copy link
Author

tgurriet commented Sep 5, 2023

@memsharded Sounds good.
The issue arises when I have a static_library package my_lib that requires a header only library header_only_lib, itself requiring a static library static_lib.

   static_lib
       ^
       |
header_only_lib
       ^
       |
    my_lib
       ^
       |
    my_app

Even if my_lib doesn't expose the header_only_lib headers in its interface and consumes everything in the cpp, my_app still need to link with static_lib.
Currently, this only happens if my_lib requires header_only_lib with the transitive_headers=True option.
This is misleading, because is has nothing to do with transitivity of headers, but transitivity of dependencies.

I've updated the minimal example repo : https://github.com/tgurriet/conan_deploy_test

In that repo, there are 2 scripts to build with deployers run_with_deployer.sh or directly from the conan cache run_without_deployer.sh.

See https://github.com/tgurriet/conan_deploy_test/tree/82c2c28b15c310d9d7bba6db959059301bcd5c9c for a commit without transitive_headers=True, and https://github.com/tgurriet/conan_deploy_test/tree/911f33e0b8488dc00b186b63caec3fb42decf037 for a commit with.

@memsharded
Copy link
Member

I am trying to reproduce with a minimal unit-test, and so far it seems to be working fine, adding it in a PR in #14673

This test is validating in

        # node, headers, lib, build, run
        _check_transitive(app, [(libc, True, True, False, False),
                                (libb, False, False, False, False),
                                (liba, False, True, False, False)])

The tratis that app receives for each dependency. It can be seen that receives headers=False and libs=True from liba, which is the desired behavior.

The code you are providing is a bit more complicated because it depends on third parties packages, but I'll try to have a look.

@memsharded
Copy link
Member

Ok, I think I found something:

  • You have removed the shared option in your my_lib
  • You haven't declared any package_type in my_lib
  • Conan cannot know that my_lib is a static library, and it is assuming it is unknown type. You can visualize this information with conan graph info . --filter=package_type
  • Unknown libraries might need more manual work. In your case it can be solved, not by adding transitive_headers=True, but by adding transitive_libs=True instead

Then, the solution would be for this case to add package_type = "static-library" to your my_lib

@tgurriet
Copy link
Author

tgurriet commented Sep 5, 2023

My bad!

I did add the package_type = "static-library", and it does fix the unknown package type, but I still get the same linking issue.

Here is the output:

======== Computing dependency graph ========
Graph root
    conanfile.txt: /home/tom/tmp/conan_tests/conan_deploy_test/my_app/conanfile.txt
Requirements
    cpp-httplib/0.12.2#455905521b4e4a99722a5855cc529379 - Cache
    my_lib/1.0#9eba545372486a17c9755b899b3235a2 - Cache
    openssl/3.1.2#ef79f25fe0968d69538feca6a6b3bc7f - Cache
    zlib/1.2.13#e377bee636333ae348d51ca90874e353 - Cache
Resolved version ranges
    openssl/[>=1.1 <4]: openssl/3.1.2

======== Computing necessary packages ========
Requirements
    my_lib/1.0#9eba545372486a17c9755b899b3235a2:17616e1f44a3e497c1a25e9410c427298c437968#359b37a8ce24be2b0a983c737ed402fb - Cache
    openssl/3.1.2#ef79f25fe0968d69538feca6a6b3bc7f:b24936b82648bf158642bac0d8eadcb966155d54#50c98e007c622e482206ca83ebba6e3d - Cache
    zlib/1.2.13#e377bee636333ae348d51ca90874e353:d152c3753ced4eedbb7024e2e948e5f19a896716#31a69f5197bb967ec70cc4647ff1e2ca - Cache
Skipped binaries
    cpp-httplib/0.12.2

======== Basic graph information ========
conanfile:
  ref: conanfile
  package_type: unknown
my_lib/1.0#9eba545372486a17c9755b899b3235a2:
  ref: my_lib/1.0#9eba545372486a17c9755b899b3235a2
  package_type: static-library
cpp-httplib/0.12.2#455905521b4e4a99722a5855cc529379:
  ref: cpp-httplib/0.12.2#455905521b4e4a99722a5855cc529379
  package_type: header-library
openssl/3.1.2#ef79f25fe0968d69538feca6a6b3bc7f:
  ref: openssl/3.1.2#ef79f25fe0968d69538feca6a6b3bc7f
  package_type: static-library
zlib/1.2.13#e377bee636333ae348d51ca90874e353:
  ref: zlib/1.2.13#e377bee636333ae348d51ca90874e353
  package_type: static-library

I'll try to recreate the issue in the PR you created.

@tgurriet tgurriet closed this as completed Sep 5, 2023
@tgurriet tgurriet reopened this Sep 5, 2023
@memsharded
Copy link
Member

For some reason you still have

Skipped binaries
    cpp-httplib/0.12.2

But this definitely shouldn't happen, I cannot see it happening in your code either.

@tgurriet
Copy link
Author

tgurriet commented Sep 5, 2023

Are you saying that when you run run_without_deployer.sh on https://github.com/tgurriet/conan_deploy_test/tree/master , it doesn't skip cpp-httplib/0.12.2 ?

@memsharded
Copy link
Member

I am running in Windows, I get with conan install . --build=missing:

Requirements
    cpp-httplib/0.12.2#455905521b4e4a99722a5855cc529379:da39a3ee5e6b4b0d3255bfef95601890afd80709#e2876922f59c7c2d5e0559af51c450bd - Cache
    my_lib/1.0#7e2d0062c8b8770e3cc841f7354181ee:856aa321df05c24316cd121c0e4ce62110be0f5c - Build
    openssl/3.1.2#ef79f25fe0968d69538feca6a6b3bc7f:0315d5f1ccef6b8aa549998919d6932429593006#3cab9c3b5f04f387cf26f3dd8c789bdb - Cache
    zlib/1.2.13#e377bee636333ae348d51ca90874e353:7bfde258ff4f62f75668d0896dbddedaa7480a0f#6612117c7b5ed9d05294c47602331be1 - Cache
Build requirements
Skipped binaries
    nasm/2.15.05, strawberryperl/5.32.1.1

@tgurriet
Copy link
Author

tgurriet commented Sep 5, 2023

Interesting...
I run on Ubuntu 22.04 LTS, with conan 2.0.9, and python Python 3.10.12
Is there a way to create a test that can be run in different environments?

@memsharded
Copy link
Member

Now I managed to reproduce, not sure why I couldn't see that earlier.

@memsharded memsharded added this to the 2.0.11 milestone Sep 6, 2023
@memsharded
Copy link
Member

Thanks for all the feedback above, very useful! I have changed the tag to bug trying to fix it for next 2.0.11.
I have been investigating the root cause and working on a fix for it since yesterday in #14673, getting closer, I will keep updating.

@tgurriet
Copy link
Author

tgurriet commented Sep 6, 2023

Thanks for all this hard work, you guys rock!
Let me know if there is anything I can do to help.

@memsharded
Copy link
Member

I think the PR in #14673 is ready and should fix the issue.
If you know how to run conan from source from a given fork-branch, running from my PR branch, that would be valuable feedback.

@tgurriet
Copy link
Author

tgurriet commented Sep 7, 2023

It does work on the PR branch, both with and without deployer!
Thanks!

@memsharded
Copy link
Member

Amazing! :) :) :) Thanks for the feedback!

@DoDoENT
Copy link
Contributor

DoDoENT commented Sep 12, 2023

@memsharded , your branch also fixes one more bug I just discovered. I have self.test_requires('header_only_package/1.0') in one my project and conan 2.0.10 doesn't generate cmake find_package instructions for it at all (the package is header-only, but also contains some cmake build modules that should be executed during find_package).
Interestingly, the same package is also test_required in other places, where this bug didn't appear.

All in all, I've tried using conan from your PR, and it works correctly.

@memsharded
Copy link
Member

Thanks @tgurriet and @DoDoENT for the testing and the feedback, really appreciated!

We have merged #14673, it will be in next 2.0.11

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.

3 participants