-
Notifications
You must be signed in to change notification settings - Fork 314
feat: pypi-options.dependency-overrides #3948
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: HernandoR <[email protected]>
Signed-off-by: HernandoR <[email protected]>
…ization Signed-off-by: HernandoR <[email protected]>
Signed-off-by: HernandoR <[email protected]>
I'm a little bit concerned about the union strategy for For now, I followed a strategy like pypi-dependency: the requirements feature will override the default. The order of union logic (b overwrite a) is referred to pixi/crates/pixi_manifest/src/feature.rs Lines 305 to 326 in bb47111
But I'm not quite sure the order of features in this snippet pixi/crates/pixi_manifest/src/features_ext.rs Lines 198 to 218 in bb47111
|
…tion Signed-off-by: HernandoR <[email protected]>
We need some tests on this feature. @tdejager can you assign someone? |
@nichmor could you have a look seeing that I'm on holiday? Thanks! 🙏 |
Note you also need to add the specification to the full.toml file. Not sure about the name, on my phone currently. But contains all the available manifest options. |
Yeah so the confusing thing for PyPI-options is that even when using no-default one can have workspace PyPI dependencies that are always added. And I think, but maybe @ruben-arts can verify is that we wanted the last defined feature to take precedence. However, I remember us explicitly going back and forth on this, and maybe deviating from how dependencies function. So I could very well be wrong. But I think how it is defined in code for PyPI-options is the correct way, so I'd suggest following that for now. |
277816c
to
b02834e
Compare
Signed-off-by: HernandoR <[email protected]>
b02834e
to
8106c97
Compare
crates/pixi_command_dispatcher/tests/integration/snapshots/integration__simple_test.snap
Outdated
Show resolved
Hide resolved
I rely heavily on this feature. Plz let me know when and how we can push this forward. |
assigning to me. I will review it tomorrow and let you know how we can continue with it. |
I think we also need to write an integration test here. You can take a look at let me know if you will have questions |
Co-authored-by: nichmor <[email protected]>
Co-authored-by: nichmor <[email protected]>
1a92767
to
aa7c530
Compare
@nichmor. I added an integration test. But I don't know how to run it without uninstalling my existing installed |
There are pixi tasks that use the compiled version :) Check the pixi.toml for the list of them :) |
@tdejager i updated the test case and passed |
@nichmor I have added integration test, and verified that the generated lock file holds valid version. |
thanks for the update! I will take a look today. |
Signed-off-by: HernandoR <[email protected]>
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.
good to me
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.
two small stuff and it is ready to merge!
- use buble up the report instead of expect - add return type for the closure for more robust type inferer Signed-off-by: HernandoR <[email protected]>
Signed-off-by: HernandoR <[email protected]>
I merged main and added extra slow tests, these test a lot of PyPI integration stuff :) |
crates/pixi_command_dispatcher/tests/integration/snapshots/integration__simple_test.snap
Outdated
Show resolved
Hide resolved
Signed-off-by: HernandoR <[email protected]>
Signed-off-by: HernandoR <[email protected]>
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.
one small one and we are ready to go!
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.
Looks great! Thanks for the work!
I think if you fix the tests we can merge it
Signed-off-by: HernandoR <[email protected]>
@nichmor I can't reproduce this CI issue locally. It may be a temporary network problem. Could you please re-trigger the CI? |
I really got no clue of the relation between the failed CI test and changes in this PR. |
It seems that it is related with the timeout - I will take a look tomorrow and will take care of merging it |
add dependency-overrides for overriding.
only updated manifest for now
related #3917 #3890