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

fixing transitive order of privates #4987



Copy link

@memsharded memsharded commented Apr 15, 2019

Changelog: Bugfix: Fix skipping binaries because of transitive private requirements
Docs: omit

@tags: slow

Close: #4970

@memsharded memsharded added this to the 1.14.4 milestone Apr 15, 2019
@ghost ghost assigned memsharded Apr 15, 2019
@ghost ghost added the stage: review label Apr 15, 2019
@memsharded memsharded assigned lasote and unassigned memsharded Apr 15, 2019
@ghost ghost assigned memsharded Apr 16, 2019
Copy link
Member Author

@memsharded memsharded commented Apr 16, 2019

Hi @theodelrieu @DoDoENT @puetzk @thorntonryan @jallenwork

Please find here a new patch for Conan 1.14.4 for the processing of private, build-requires and mixed types dependencies.
It would be great if you could try it and give it a go with your projects, to check before merging and releasing.

It happens that the build-requires and private usage is way more complicated that what the model was designed for. We are trying to propose fixes and improvements to achieve both a safe dependency resolution (linking with the right dependency, conflicting if not defined) and those complex graphs Conan didn't intended to support initially (but we are doing our best to try to adapt).

Many thanks!!

@memsharded memsharded mentioned this pull request Apr 16, 2019
3 tasks
Copy link

@theodelrieu theodelrieu commented Apr 17, 2019

All I can say is that nothing broke on our end 😅

Copy link

@jallenwork jallenwork commented Apr 17, 2019

This fixes our larger reproducer. Seems to be all good on our end. Thanks for putting in the time and effort to expand the requires/build_requires functionality for the more complex cases! This is really helpful. 😄

Copy link

@DoDoENT DoDoENT commented Apr 18, 2019

I've tried that patch on both may old codebase and new one (after making update as suggested in your comment. The old one reports conflict, just like with conan v1.13+ and as demonstrated in minimum sample attached to the same issue and the new one works the same as with conan v1.14.3. Basically, I haven't seen any changes to that part of my package management from v1.14.3 to v1.14.4.

Basically, for us its the same as for @theodelrieu - nothing broke 😅

@lasote lasote merged commit 3c45a3b into conan-io:release/1.14.4 Apr 24, 2019
2 checks passed
@ghost ghost removed the stage: review label Apr 24, 2019
@memsharded memsharded deleted the fix/private_order_transitive branch Apr 24, 2019
Copy link

@thorntonryan thorntonryan commented Apr 24, 2019

@memsharded ,

Oye, I apologize for being late to the party. I had an example thrown together last night, but it was getting late so I opted to committing it in the morning... looks like that was a mistake.

As for my original example with our shared COM dlls, I think the solution is to use requirements instead of build_requirements, which will get the override behavior we're after.
Original example discussed here #4753 (comment)

So in that specific case, I think we were misusing build_requires and there's not a strong enough case to argue for the use of "private" requires with these COM dlls.

However, in the same project, we're also using the header only library Boost, and I believe that raises some questions about the way "private" requirements are resolved that are worth considering.

So, yesterday, I worked with @puetzk to come up with a more illustrative example of some potential issues we see with the new approach to "private" requirements in 1.14.4.

The primary concern is that the 1.14.4 "private" requirements could result in One Definition Rule (ODR) violations if a shared package uses header only or static libraries in such a way that they are fully embedded and hidden from consumers of the package. and not exposed through the public interface and the consuming app also uses these same header only or static libraries.

My new examples attempt to illustrate this by using "mock" packages of two popular libraries, Boost and zlib.

When using Boost in header only mode, all users of Boost must maintain full_version_mode compatibility, otherwise they risk violating ODR.

On the other hand, libraries like zlib make very strong semver ABI guarantees. A consuming app using a newer semver compatible version of zlib could, in theory, override the version of zlib used by one of its required packages.

But in both cases, Conan 1.14.4 resolves these requirements and "private" requirements in such a way that both versions of Boost and zlib are present in the consuming package... which seems problematic.

You can find that here:
Specifically added here, thorntonryan/conan-requirements-overriding-change-in-behavior@6d00020

We can work around this issue by promoting these "private" requirements to full on requirements, and then the overriding will work, but it seems a little odd to make "public" requirements on Boost / zlib when they're not at all exposed through the public API of our shared library.

Copy link

@theodelrieu theodelrieu commented Apr 26, 2019

Hi, I should have tried on my side project beforehand... I get the following assert:

ERROR: printing with scope==virtual

With 1.14.4, sorry about that!

You can reproduce by checking out the conan_bug branch here and running conan workspace install conanws.yml.

Copy link
Member Author

@memsharded memsharded commented Apr 29, 2019

Hi @theodelrieu

#5056 has a possible fix to this issue, to be included if ok in 1.14.5 release.

If you could please run that branch from source against your project, to make sure everything is ok, that would be great. Thanks!

Copy link

@theodelrieu theodelrieu commented Apr 30, 2019

Seems to work, thanks! :)

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

Successfully merging this pull request may close these issues.

None yet

6 participants