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

Fix profile repeated build requires #8463

Merged

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Feb 8, 2021

Changelog: Bugfix: Fix repeated build_requires, including conflicting versions in profile composition or inclusion that repeats [build_requires] values.
Docs: Omit

Comes from #8205 (comment)

@memsharded memsharded added this to the 1.34 milestone Feb 8, 2021
conans/model/profile.py Outdated Show resolved Hide resolved
conans/model/profile.py Outdated Show resolved Hide resolved
@jgsogo
Copy link
Contributor

jgsogo commented Feb 8, 2021

I like the changes, I think they make sense, IMO existing test was testing the bug.

@@ -137,7 +137,7 @@ def _load_profile(text, profile_path, default_folder):
for include in profile_parser.get_includes():
# Recursion !!
profile, included_vars = read_profile(include, cwd, default_folder)
inherited_profile.update(profile)
inherited_profile.update_profile(profile)
Copy link
Contributor

@SSE4 SSE4 Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO profile.update_profile() looks too redundand. profile.update() is nice and self-explaining.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until you try to search for occurrences, or code calling this method and it becomes almost impossible to know, and you need to rely on CI running to find all occurrences.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that the update_profile() is not great and redundant. I have replaced it for compose() because it doesn't conflict with common dicts update() and also because in our docs we refer to this operation as "Profile composition".

memsharded and others added 4 commits February 8, 2021 17:44
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
@memsharded memsharded requested a review from SSE4 February 15, 2021 17:11
@jgsogo jgsogo merged commit d71b159 into conan-io:develop Feb 16, 2021
@memsharded memsharded deleted the fix/profile_repeated_build_requires branch February 16, 2021 08:39
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.

3 participants