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

Feature/graph model fixes #5886

Merged
merged 9 commits into from Oct 24, 2019

Conversation

@memsharded
Copy link
Member

memsharded commented Oct 10, 2019

Changelog: Bugfix: Fix incorrect propagation of build-requires to downstream consumers, resulting in missing dependencies in deps_cpp_info.
Docs: Omit

Close #5682

NOTES:

  • I have used the transitive_closure naming, which is not very clear. After this is merged or approved, I will do a pure refactor renaming all variables public_deps, public_closure and transitive_closure to something a bit more correct.

#tags: slow

@memsharded memsharded marked this pull request as ready for review Oct 11, 2019
@memsharded memsharded added this to the 1.20 milestone Oct 11, 2019
@memsharded memsharded requested a review from jgsogo Oct 13, 2019
@jgsogo jgsogo self-assigned this Oct 14, 2019
@jgsogo

This comment has been minimized.

Copy link
Member

jgsogo commented Oct 14, 2019

Can you please add a test with the broken example provided in the issue that this PR fixes? Thanks!

@memsharded

This comment has been minimized.

Copy link
Member Author

memsharded commented Oct 14, 2019

Yes, @jgsogo you are totally right, this deserved a test. Added it based on the @sztomi repro case in https://github.com/sztomi/transitive_br_deps_cpp_info

Copy link
Member

jgsogo left a comment

IMO this PR goes in the right direction as it is moving the graph model towards a more standard approach that differentiates data from the algorithms (I would keep pushing on that direction).

All existing tests pass and I'm sure it is a step forward. Have a look at my comments (mostly naming and some doubts), I will have a look at this PR from the cross-building perspective, but probably I'll come back and approve it.

conans/client/graph/graph.py Outdated Show resolved Hide resolved
conans/client/graph/graph.py Show resolved Hide resolved
conans/client/graph/graph.py Show resolved Hide resolved
conans/client/graph/graph.py Show resolved Hide resolved
conans/client/graph/graph.py Show resolved Hide resolved
@memsharded memsharded requested a review from jgsogo Oct 14, 2019
memsharded added 3 commits Oct 15, 2019
@jgsogo
jgsogo approved these changes Oct 23, 2019
Copy link
Member

jgsogo left a comment

I've updated my PR about cross-building #5592 with the changes on this one and I'm able to make it work again with a couple of changes (see diffs here https://github.com/memsharded/conan/compare/feature/graph_model_fixes...jgsogo:feat/xbuild-graph-pr5886?expand=1).

The most important change is that I needed to add a br_host_closure (next to transitive_closure) to keep track of the build requires with force_host_context=True in order to propagate cpp_info or env_info from build-requires to consumers.

So, from my POV, let's merge this PR 👍


Just a comment, please refactor/rename on top of the cross-building PR (i will open a new one when this gets merged).

@jgsogo jgsogo removed their assignment Oct 23, 2019
@lasote lasote merged commit a323291 into conan-io:develop Oct 24, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
@memsharded memsharded deleted the memsharded:feature/graph_model_fixes branch Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.