-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add per-requirement --config-settings #11634
Conversation
Hmm, tests are failing but they seem to be unrelated. Something may be wrong in the CI? |
I've tried to investigate, whats wrong with ssl but unsuccessfully |
ee0ac23
to
6177722
Compare
I tentatively added this to the 23.1 milestone as it'd be nice to have it at the same time as the removal of It looks like this PR is doing two things:
It seems (2.) was not done for I don't have a ton of practical experience with these options, but my instinct says it might be more flexible not to do that and let the user precisely specify which req must get which config settings. |
I don’t have any practical experience to draw on either, but I strongly believe that we shouldn’t be introducing (2) silently. I’d rather see it as a separate PR so the implications can be discussed in isolation, but even if that isn’t possible I’d want to see some clear justification for the behaviour. It should also be explained clearly in the docs (as it’s far from obvious - see the comment by @uranusjr above) and explicitly noted as new behaviour in the changelog. I don’t believe we have any other case of settings being inherited by dependencies like this. I’m personally -1 on behaviour (2), but mostly because it feels really confusing to me, and I expect it to be a source of bugs as users misunderstand it as well. |
So should I remove the build flag propagation? |
@q0w I think so yes. Can you also add a test where config settings are passed both on the CLI and as per-requirement options? You may also want to base your work on #11876. |
|
This comment was marked as resolved.
This comment was marked as resolved.
Keep in mind that this PR should not be merged before #11876 is. |
after #11876 i cant pass merged cli/reqs config_settings to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @q0w ! A last round comment and it it should be good to go.
1dd2328
to
6ea91c8
Compare
This looks pretty close to being ready for merging. If we can get this done in the next week or so, it'll make it into 23.1 which would be excellent. |
@q0w following the discussion in #11915 (comment) we'll need to rework this PR to remove the merging with CLI requirements. Sorry again for the confusion. If you think you will not have time to do it by mid next week, just let me know and I'll finalize it. |
@sbidoul Removed merging config settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Closes #11325