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 application of build_requires to host context build_requires #7500

Merged

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Aug 5, 2020

Changelog: Bugfix: When using build_requires defined in a profile that is passed as profile_host, it was not being applied to build_requires that live in the host context (with force_host_context=True).
Docs: Omit

#tags: slow

Reported by @jasal82

@memsharded memsharded added this to the 1.28.1 milestone Aug 5, 2020
@memsharded memsharded requested a review from jgsogo Aug 5, 2020
@memsharded memsharded self-assigned this Aug 5, 2020
Copy link
Member

@jgsogo jgsogo left a comment

I haven't looked at the implementation, but I'd say that the test makes sense (I suggested a change to be 100% sure that the BR belongs to the build context).

Do you know the previous PR that changed this behavior? Maybe #7169?

@memsharded
Copy link
Member Author

memsharded commented Aug 5, 2020

yes, this was introduced in Conan 1.26, in #7169. The idea behind the change is correct, but it didn't fully covered the case of force_host_context=True. If the CI is green, I'll try to simplify/refactor the implementation, too much copy & paste

@memsharded memsharded marked this pull request as ready for review Aug 5, 2020
@memsharded memsharded requested a review from jgsogo Aug 5, 2020
jgsogo
jgsogo approved these changes Aug 5, 2020
Copy link
Member

@jgsogo jgsogo left a comment

Not a trivial change 😄


With this change, do you know what happens if the profile:build declares a build-requires and the profile:host declares another build-requires. Will the BR from the profile:build be applied to the BR from the profile:host? It was something broken/undecided in #7169

@memsharded
Copy link
Member Author

memsharded commented Aug 5, 2020

With this change, do you know what happens if the profile:build declares a build-requires and the profile:host declares another build-requires. Will the BR from the profile:build be applied to the BR from the profile:host? It was something broken/undecided in #7169

I have added a new test. Indeed there is a limitation, and the build_requires required in the profile_build do not apply to other build_requires that come from a profile_host, because the algorithm does not propagate build requires from profiles, as otherwise they easily induce infinite recursion.

I think we should probably leave this limitation in place, trying to improve that has a high chance to break something.

jgsogo
jgsogo approved these changes Aug 6, 2020
@memsharded memsharded merged commit bdd2ce7 into conan-io:release/1.28 Aug 6, 2020
2 checks passed
@memsharded memsharded deleted the hotfix/xbuild_build_requires_host branch Aug 6, 2020
@jasal82
Copy link
Contributor

jasal82 commented Aug 6, 2020

Verified release branch against our cross build, works fine!

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

Successfully merging this pull request may close these issues.

None yet

3 participants