-
Notifications
You must be signed in to change notification settings - Fork 418
Add support for repodata v3 conditionals #4098
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
|
Hey @baszalmstra, since you are very familiar with the Rattler implementation, would you have a few minutes to review this? |
|
Wow, this is quite the PR! A thorough review is definitely required! I checked the codebase a little bit, and I think this method is fundamentally flawed. Although it will handle cases with virtual packages because they are known upfront, it will not work with version checks. E.g. I think the code around checking these "static" virtual packages could also be improved a lot. There is a custom check in there now but you could also just match the condition against prebuild virtual packages. On the C++ front there are also definitely some things to improve. Although my C++ is very Rusty (pun intended), I think the use of raw pointers and unique_ptrs, could use some tweaking. There were multiple Regardless, libsolv actually does already support conditional dependencies, and I think it's just a matter of wiring them up properly. Rattler also has a libsolv implementation where I am experimenting with this, see this pr. If I were a maintainer of mamba (which, to be clear, I am not), I would first try to split this PR up into more individualized components before merging. Start with the parsing, potentially make conditionals optional initially, and then work on integrating the solver. I would definitely not merge this PR with the current functionality because it contains a lot of unhandled edge cases. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4098 +/- ##
==========================================
+ Coverage 63.57% 64.34% +0.76%
==========================================
Files 315 318 +3
Lines 38562 39685 +1123
Branches 2950 3026 +76
==========================================
+ Hits 24517 25535 +1018
- Misses 13976 14074 +98
- Partials 69 76 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Partially closes #4089 by adding conditional support based on the draft CEP. This will need to be updated once we agree on the syntax, if we use the bracket style of passing in a conditional key it will require less parsing for this implementation.
I do not have experience with the mamba code base - this was my first time contributing and setting up a development environment. I also only limited experience with C++ - so all feedback is greatly appreciated.
This PR was assisted by a LLM in order to help gauge the amount of work this would be. The initial prompt was "What changes would be needed to support repodata v3, similar to how Rattler supports the implementation?" Follow-up context using the following steps:
MatchSpecConditiondata structure (header and implementation) with testsMatchSpecto include condition field and parsingA follow-up change could also add support for extras and flags.
Type of Change
Checklist
pre-commit run --alllocally in the source folder and confirmed that there are no linter errors.