Skip to content

Proof of concept of adding options per-requirement#10112

Merged
memsharded merged 12 commits into
conan-io:develop2from
memsharded:feature/develop2_build_requires_options
Dec 13, 2021
Merged

Proof of concept of adding options per-requirement#10112
memsharded merged 12 commits into
conan-io:develop2from
memsharded:feature/develop2_build_requires_options

Conversation

@memsharded
Copy link
Copy Markdown
Member

@memsharded memsharded commented Dec 1, 2021 •
edited
Loading

Responding to #9839, and to the need of defining different options in build_requires and requires:

  • This approach can actually set any option for any type of requirement, at the individual level (can work for multiple regular requires of different version of same package (if no conflicts))

Close #9839

@memsharded memsharded added this to the 2.0.0-alpha2 milestone Dec 1, 2021
lasote
lasote approved these changes Dec 9, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@lasote lasote left a comment

Choose a reason for hiding this comment

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

I know these tests are pretty straightforward but we must introduce the dev rule of a docstring on every test. That would save a lot of review time and debug time when a feature breaks some test.

Comment thread conans/test/integration/options/test_options_build_requires.py
@memsharded
Copy link
Copy Markdown
Member Author

memsharded commented Dec 9, 2021

Agree with the docs on tests. This is the review I did to other PR after @franramirez688 suggestion: 5ec4b5a. Lets do it always.

@memsharded
Copy link
Copy Markdown
Member Author

memsharded commented Dec 9, 2021

Missing @czoido, but the idea seems to be ok? I submitted this as a PoC for the interface, I will try to extend a bit more the tests and mark it ready for review & merge

@lasote
Copy link
Copy Markdown
Contributor

lasote commented Dec 10, 2021

OMG so happy to see these docstrings

memsharded
memsharded commented Dec 10, 2021
View reviewed changes
Comment thread conans/client/graph/graph_builder.py Outdated
down_options = Options(options_values=require.options)
down_options.scope(new_ref.name)
# TODO: discuss, together with build_require override, if it is "build" or "visible"
if require.visible: # Only visible requirements propagate options from downstream
Copy link
Copy Markdown
Member Author

@memsharded memsharded Dec 10, 2021

Choose a reason for hiding this comment

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

Changed this from not require.build => require.visible., thinking about what makes more sense...

@memsharded
Copy link
Copy Markdown
Member Author

memsharded commented Dec 10, 2021

I have added tests to validate that visible=False requires cannot be overriden/forced from downstream.
Also including the options propagation condition to not apply to visible=False and to apply only to context=HOST.

Please re-review.

lasote
lasote reviewed Dec 10, 2021
View reviewed changes
Comment thread conans/client/graph/graph_builder.py
@memsharded memsharded marked this pull request as ready for review December 13, 2021 08:42
@memsharded memsharded mentioned this pull request Dec 13, 2021
Closed
@memsharded memsharded merged commit 6708d0a into conan-io:develop2 Dec 13, 2021
@memsharded memsharded deleted the feature/develop2_build_requires_options branch December 13, 2021 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@franramirez688 franramirez688 franramirez688 approved these changes

@czoido czoido czoido approved these changes

+1 more reviewer

@lasote lasote lasote approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type: look into

Projects

None yet

Milestone

2.0.0-alpha2

Development

Successfully merging this pull request may close these issues.

4 participants

@memsharded @lasote @franramirez688 @czoido