Skip to content

Conversation

@olivier-lacroix
Copy link
Contributor

@olivier-lacroix olivier-lacroix commented Apr 20, 2025

This PR refactors the recursion resolution of dependency-groups into a trait, and implements it for both

  • dependency groups, and
  • optional dependencies as well.

Let me know what you think.

@konstin
Copy link
Member

konstin commented Apr 22, 2025

I would prefer handling this without self_reference_name and instead have the caller pass in the name of the package if they want to have self-referential optional dependencies resolved.

Can we implement this without introducing a new trait? The logic should be small enough that we should be able to implement without public helper types.

@olivier-lacroix
Copy link
Contributor Author

@konstin okay. Let me do that.

@olivier-lacroix
Copy link
Contributor Author

@konstin done. I kept the trait but made it internal to this crate. Hopefully this aligns with what you had in mind!

@olivier-lacroix olivier-lacroix force-pushed the recurse branch 3 times, most recently from b8e5da2 to c9506f3 Compare April 23, 2025 10:35
@konstin
Copy link
Member

konstin commented Apr 24, 2025

I've tried to simplify the code by inlining the trait (https://github.com/PyO3/pyproject-toml-rs/compare/konsti/refactor-resolve?expand=1), but hit a bigger problem: When we want to resolve a dependency group for a project foo that depends on foo[bar], we need to have the optional dependencies in the dependency group resolution to resolve bar, so I think the methods need to move to the PyProjectToml top level to provide proper resolution.

@olivier-lacroix
Copy link
Contributor Author

Goddam it. That’s a diabolical corner case…

@dhirschfeld
Copy link

Super exciting to see this passing, but for the lint:

    Checking pyproject-toml v0.13.5 (/home/runner/work/pyproject-toml-rs/pyproject-toml-rs)
error: very complex type used. Consider factoring parts into `type` definitions
  --> src/resolution.rs:15:10
   |
15 |       ) -> Result<
   |  __________^
16 | |         (
17 | |             Option<IndexMap<String, Vec<Requirement>>>,
18 | |             Option<IndexMap<String, Vec<Requirement>>>,
19 | |         ),
20 | |         RecursionResolutionError,
21 | |     > {
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity
   = note: `-D clippy::type-complexity` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::type_complexity)]`

error: could not compile `pyproject-toml` (lib) due to 1 previous error

Being able to support pip's nested dependencies will make transitioning to pixi easier.

@olivier-lacroix
Copy link
Contributor Author

olivier-lacroix commented Jul 29, 2025

@konstin I found the time to get back to this.

  • there is now a single top level PyProjectToml::resolve method, that resolves both dependency groups and optional dependencies
  • name collision between dependency groups and optional dependencies will raise an error during that resolution
  • resolved dependency groups and optional dependencies are returned as a tuple: it would feel maybe cleaner to wrap dependency groups and optional dependencies into a Resolved / Unresolved Enum, but let me know what you think.

ps: I somehow missed your refactor 🫨 ... I'll integrate it if you are happy with the direction and feel it would still be beneficial.

@olivier-lacroix
Copy link
Contributor Author

I have tried to integrate your proposed refactor @konstin

@dhirschfeld
Copy link

CI is green, @olivier-lacroix I assume this is ready for review/merge from your PoV?

@olivier-lacroix
Copy link
Contributor Author

Indeed 👍

@olivier-lacroix
Copy link
Contributor Author

@konstin any chance you could take a look at this PR ?

@konstin konstin changed the title Add a method to resolve recursion in optional dependencies Support resolving optional dependencies and dependency groups Sep 12, 2025
@konstin konstin enabled auto-merge (squash) September 12, 2025 08:57
@konstin konstin merged commit 261b5d2 into PyO3:main Sep 12, 2025
5 checks passed
@dhirschfeld
Copy link

Thank you @olivier-lacroix & @konstin! I'm super excited to be able to make use of this (via pixi)!

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