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

[Proposal] Add info.shared_library_package_id() in package_id() of recipes with shared option and requirements #8987

Open
SpaceIm opened this issue Jan 19, 2022 · 7 comments
Labels
question Further information is requested

Comments

@SpaceIm
Copy link
Contributor

SpaceIm commented Jan 19, 2022

I would like to add self.info.shared_library_package_id() to package_id() of all recipes which have both:

  • shared option
  • at least one dependency

info.shared_library_package_id() experimentations

Inputs

  • coin-osi/0.108.6 depends on coin-utils/2.11.4

  • coin-utils/2.11.4 depends on bzip2/1.0.8 and zlib/1.2.11

  • bzip2/1.0.8 and zlib/1.2.11 have no dependency

  • all these 4 recipes have a shared option

  • self.info.shared_library_package_id() added to package_id() of coin-utils and coin-osi recipes

  • semver_direct_mode

  • 1 profile:

    [settings]
    os=Macos
    arch=x86_64
    compiler=apple-clang
    compiler.version=13.0
    compiler.libcxx=libc++
    build_type=Release
    [options]
    [build_requires]
    [env]
    

Results

package id of coin-utils coin-utils:shared bzip2:shared zlib:shared
e967b6f7ee847ba2cdb4c6a8f494f465b893beb3 False False False
e967b6f7ee847ba2cdb4c6a8f494f465b893beb3 False False True
e967b6f7ee847ba2cdb4c6a8f494f465b893beb3 False True False
e967b6f7ee847ba2cdb4c6a8f494f465b893beb3 False True True
551fd2a7f34e247b845cc00ebdfd1ffff6cc2343 True False False
38d7a56d80d99e4037126c5b45d850f3a4e2cc02 True False True
cd1c15360b13e1597a290ea32632dedf5d71963d True True False
89d156d64d036c8e2b28fafbec121e33631892d2 True True True
package id of coin-osi coin-osi:shared coin-utils:shared bzip2:shared zlib:shared
75e8bb5968025afef8576672befb122c7606ad79 False False False False
75e8bb5968025afef8576672befb122c7606ad79 False False False True
75e8bb5968025afef8576672befb122c7606ad79 False False True False
75e8bb5968025afef8576672befb122c7606ad79 False False True True
75e8bb5968025afef8576672befb122c7606ad79 False True False False
75e8bb5968025afef8576672befb122c7606ad79 False True False True
75e8bb5968025afef8576672befb122c7606ad79 False True True False
75e8bb5968025afef8576672befb122c7606ad79 False True True True
733a6fa355b9d21e21ce6290a46a4487954dd885 True False False False
cdf81eb3ff7d05565c7c3af845bd9d66e1d08293 True False False True
2f8dbd5837bea12540b17735960a694616678487 True False True False
8767850661d445c39b779ec7f22fe7a9d4ee5164 True False True True
1a4689d6cd2560c6b7bbfd5093774ea10ed3053e True True False False
72a57b433abcaeafddd8a2ffe5369b156d94600c True True False True
e959a4b7c79174a75291bc10205f41d69367c06f True True True False
74185bfff702d97df31fda8c8885b2b02fd7a0e1 True True True True

Additional informations and analysis:

  • the package id of a package if static is the same whether self.info.shared_library_package_id() is in the recipe or not => no breaking change to package id of static binaries in CCI.
    Basically it means that these 2 sentences are identical, and it's fine:

     def package_id(self):
         self.info.shared_library_package_id()
     def package_id(self):
         if self.options.shared:
             self.info.shared_library_package_id()
  • the package id of a package if shared and all dependencies static is different depending on whether self.info.shared_library_package_id() is in the recipe or not. It makes sense, shared options of its dependencies (direct & transitive) now contribute to its package id, and it's fine. Moreover there is likely no side effect for CCI, almost all shared packages in CCI are built against static dependencies.

  • if shared and self.info.shared_library_package_id() in the recipe, all combinations of shared values in dependencies (direct and transitive) will lead to a different package id.

  • When coin-osi and coin-utils are shared (the 4 last lines in the 2nd table), binaries of coin-osi are the same regardless of bzip2:shared and zlib:shared values, but package id are different. It comes from the fact that conan treats all dependencies of a recipe as public dependencies, so it can't know that bzip2 & zlib are private dependencies of coin-utils. Otherwise conan would have known that a private dependency of a shared library (ie a dependency which doesn't leak in public headers of this library) can't leak into the binaries of downstreams of this shared library.

  • I've not tested how it behaves when another option than shared is changed in a dependency (since I've used semver_direct_mode I would not expect a rebuild of the downstream shared packages).

  • see also these discussions:
    [question] Usage of shared_library_package_id #1609
    [bug] impossible to consume all shared conan#9712
    Feature: update conan new to latest guidelines conan#8106 (comment)
    https://cpplang.slack.com/archives/C41CWV9HA/p1642585377350200

  • Currently, 486 recipes in CCI have dependencies (but I don't know how many of them are not header only libs or pure tools). By the way, regarding pure tools, they should also have a different package id depending on shared options of their dependencies, but it's out of scope of this proposal.

WDYT? Can we safely add this feature in CCI recipes?

/cc @madebr @SSE4 @uilianries @memsharded @lasote @jgsogo

@SpaceIm SpaceIm added the question Further information is requested label Jan 19, 2022
@SpaceIm
Copy link
Contributor Author

SpaceIm commented Jan 19, 2022

  • I've not tested how it behaves when another option than shared is changed in a dependency (since I've used semver_direct_mode I would not expect a rebuild of the downstream shared packages).

Ok so I've tested, I was not sure looking at the documentation: https://docs.conan.io/en/latest/reference/conanfile/methods.html#self-info-shared-library-package-id
If a recipe has shared=True and self.info.shared_library_package_id(), and if shared=False in a dependency, all options of this dependency will contribute to package id of the recipe.
Is it too much for CCI?

In terms of internal build in CCI, I think it's fine. For consumers it means that if they just change shared to True in one recipe A, and any option in another static/header only dependency B of this recipe (even an option which doesn't contribute to package id of B I think), it would not match package id of pre build shared package of recipe A in conan-center. But I guess it's also what we would like, isn't it?

But let's say a recipe A depends on boost header-only.
Since boost recipe is not modularized, this recipe A just depends on boost recipe which has far too much for it. Consumer decides to depend on the shared recipe A, and also change boost:header_only=True. Since in CCI we build recipes against boost:shared=False and boost:header_only=False => rebuild of shared recipe A for consumer?

@ericLemanissier
Copy link
Contributor

It would be an improvement for sure. I have one concern: does it imply that in a situation when a shared lib requires a static lib, it is impossible to override the version of the static requirement without rebuilding the shared lib? It would make 100% sense, but basically banish version overriding from CCI.
It would imply that when a dependency A is bumped in a package B, all the packsges in dependency graphs requiring B and A will need to bump A too.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Jan 20, 2022

I have one concern: does it imply that in a situation when a shared lib requires a static lib, it is impossible to override the version of the static requirement without rebuilding the shared lib?

Good point.
I would say yes. I didn't try, but for shared build package_revision_mode is applied to static dependencies.

But I don't think it's incompatible with CCI. It would mean that we wouldn't add self.info.shared_library_package_id() to the few recipes with shared=True by default, because the consequences for shared builds of downstream recipes would be unmanageable in CCI.

@SSE4
Copy link
Contributor

SSE4 commented Jan 20, 2022

there are several things I personally dislike with shared_library_package_id:

  • it doesn't scale, it practically has to be added to every recipe that 1) has a shared option 2) has at least one dependency with shared option
  • I am not sure it works correctly for all the cases, e.g. on Windows, static/shared vs static/static should gain different package ids (because of different imports like _jpeg_init vs _imp_jpeg_init)
  • it doesn't handle other options besides shared, but they are still erased the similar way
  • as you have mentioned, it's not clear if it works for the executables/tools that don't have a shared option

given the above reasons, I am also not sure about its future, e.g. will this helper stay in 2.0, or will it get a proper fix for package ID calculation (conan-io/conan#9712).

nevertheless, we're going to discuss this internally and let you know about the overall strategy going forward.

@fanselm
Copy link

fanselm commented Jan 24, 2022

So I recently tried to roll out the shared_library_package_id()for all my recipes. I built all the packages and uploaded to our internal artifactory. Then I tried to install the dependencies for our product on a different machine downloading binaries from the artifactory. This worked great for all the packages that were shared libraries. However, suddenly for some Conan complained it couldn't find binaries for a few packages. These packages were shared libraries were shared libraries, but depended on packages built as static libraries. The reason it turned out is that they required both a specific recipe hash, package ID and a package binary hash. The package binary hash would for some reason be different on the one build on one machine (and thus uploaded to artifactory) and on the machine that tried to install the package. I have not completely found the reason why this is the case - but I think it has to do with how Conan determines the package hash on the machine you try to install the package on. If you already have a recipe for the package in the cache, but no binaries, it will suddenly depend on a package revision of #0, but if a package binary already exists (built locally) it will take the hash of this which even if all actual package files are identical, will be different from the one built on another machine as the hash is calculated from all files including conaninfo.txt which may include settings that include [env] from the machine on which it was built, which should have no impact on the generated binary.

Anyway, it would be nice if when using shared_library_package_id() that it only uses full_package_mode() for static library dependencies instead of package_revision_mode which I think is flawed or too strict. Or at least a parameter to allow for easily changing the behavior would be nice, so that the current default behavior could be kept.

@jgsogo
Copy link
Contributor

jgsogo commented Jan 26, 2022

@fanselm , it looks like it could be useful feedback to Conan client. If you feel like there is a bug of something that can be improved, please do, if it is not possible for Conan v1.x at least that feedback will be considered for the Conan v2 version which is right now under development.


Regarding the topic of this issue. We asked and we've been said that Conan v2 won't have this problem, the dependency graph is improved and this situation is managed properly. So probably some of the tools or helpers won't be available (the best situation would be that they exist but are noop during the transition, but better if we don't need them at all in v2 codebase). Also, we have been working under this situation since the very beginning.

Also, as it has been said, shared_library_package_id uses package_revision_mode for affected requirements (here):

    def shared_library_package_id(self):
        if "shared" in self.full_options and self.full_options.shared:
            for dep_name in self.requires.pkg_names:
                dep_options = self.full_options[dep_name]
                if "shared" not in dep_options or not dep_options.shared:
                    self.requires[dep_name].package_revision_mode()

and pakcage_revision_mode uses every single bit of information (recipe-revision and package-revision as well): https://docs.conan.io/en/latest/creating_packages/define_abi_compatibility.html. That means that we would get a different package-id whenever anything changes on those requirements... and it means many missing dependencies error when trying to consume packages from ConanCenter. IMHO, this is a blocker 🚨 .

We can document this behavior, I'm fine to start a section in the documentation of CCI about known issues where we can describe the problem and the possible workaround (better if we promote the one that doesn't require modifying recipes).

Thinking about the future we can talk about using *:shared=True instead of library:shared=True when building binaries here. Probably it is more expected (or at least less prone to unexpected behaviour) than current behavior. I think this is not something we can just change, it will probably break lots of builds... but if we feel like it is needed, we can try to figure out how to do the migration.

@SSE4
Copy link
Contributor

SSE4 commented Mar 3, 2022

for the record, shared_library_package_id internally uses pakcage_revision_mode under the hood.
package_revision_mode is already planned for the removal in conan v2 as it has some fundamental design flaws: https://github.com/conan-io/tribe/blob/main/design/030-deprecate_package_revision_mode.md
we already have issues with shared_library_package_id (see #9565), so we cannot recommend usage of shared_library_package_id in CCI recipes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants