Add support for package-level conflicts in workspaces#14906
Conversation
This makes a lot more sense as a name IMO. And I think also works better for the immediate future, where I plan to add a `Project` kind.
Basically, this new conflict kind means that the entire project conflicts with a dependency group or an extra. This just adds the variant. In the next commit, we'll actually make it work.
Supporting project level conflicts ends up being pretty tricky, mostly because depenedency groups are represented as dependencies of the project you're trying to declare a conflict for. So by filtering out the project in the fork for the conflicting group, you end up filtering out the group itself too. To work-around this, we add a `parent` field to `PubGrubDependency`, and use this to filter based on project conflicts. This lets us do "delayed" filtering by one level. The rest of the changes in this commit are for reporting errors when you try to activate the group without disabling the project.
There was a fair bit of support for project level conflicts missing from `UniversalMarker`. I think that's because my initial PR pre-dated the beefing up of `UniversalMarker`.
This reverts commit 3c1057d.
62a084f to
7927fb3
Compare
0691c7a to
84fdb18
Compare
84fdb18 to
1dc7840
Compare
BurntSushi
left a comment
There was a problem hiding this comment.
Nice, I think this LGTM. I think it would still be good to get @charliermarsh's review on the parent package stuff I added (I left a comment noting where).
I was curious what else you did here to make the remaining cases work?
| /// group as a dependency of that package. So if you filter out the package | ||
| /// in a fork due to a conflict, you also filter out the group. Therefore, | ||
| /// we introduce this parent field to enable "delayed" filtering. | ||
| pub(crate) parent: Option<PackageName>, |
There was a problem hiding this comment.
IIRC, this is the thing I was most uncertain about @charliermarsh
There was a problem hiding this comment.
I'm also not 100% sure on the implications here...
| "); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
@zanieb These locks LGTM I think. What did you do to make this case work?
There was a problem hiding this comment.
To get detect_conflicts to pass, I needed to add
uv/crates/uv/src/commands/project/mod.rs
Lines 2513 to 2516 in 1dc7840
which required, more broadly, understanding the subset of package we were reasoning about during conflict detection.
Then there were some minor problems in the forking logic. The main thing was
uv/crates/uv-resolver/src/resolver/mod.rs
Lines 3792 to 3798 in 1dc7840
| { package = "example" }, | ||
| # TODO(zanieb): Technically, we shouldn't need to include the extra in the list of | ||
| # conflicts however, the resolver forking algorithm is not currently sophisticated | ||
| # enough to pick this up by itself |
There was a problem hiding this comment.
Oh interesting. Could this be added outside of the resolver automatically? Hmm, no, I don't think so. It could only be done in the "direct" case I think? Is there a test for the indirect/transitive case? I looked below but I don't think I see this work-around used anywhere else.
There was a problem hiding this comment.
I looked into that a bit but decided to cut scope. It seems quite plausible to update the resolver to handle this case when forking.
There was a problem hiding this comment.
There is lock_conflicting_workspace_members_depends_transitive_extra but in that case there's a conflict that should not be resolved. I didn't add a transitive case like this one — I felt like I was going to end up playing whackamole with a bunch of complicated test cases. I think if it wasn't in preview, I wouldn't be comfortable without doing a lot more testing.
| const JSON_OUTPUT = 1 << 2; | ||
| const PYLOCK = 1 << 3; | ||
| const ADD_BOUNDS = 1 << 4; | ||
| const PACKAGE_CONFLICTS = 1 << 5; |
There was a problem hiding this comment.
What's the thinking behind releasing this under a flag?
There was a problem hiding this comment.
It's in the summary
As with our existing support for conflicting extras, there are edge-cases here where the resolver will not fail even if there are conflicts that render a particular install target unusable. There's test coverage for some of these. We'll still error at install-time when the conflicting groups are selected. Due to the likelihood of bugs in this feature, I've marked it as a preview feature.
I'm just not sure we'll be able to meet our bar for correctness here in the first iteration.
| match conflicting_item.kind() { | ||
| // We should not filter entire projects unless they're a top-level dependency | ||
| // Otherwise, we'll fail to solve for children of the project, like extras | ||
| ConflictKindRef::Project => { |
There was a problem hiding this comment.
Nit: use a matches! since there's only one active branch here?
There was a problem hiding this comment.
I wanted it to be exhaustive so it needs to be revisited if we add another kind, but I don't feel strongly.
| /// group as a dependency of that package. So if you filter out the package | ||
| /// in a fork due to a conflict, you also filter out the group. Therefore, | ||
| /// we introduce this parent field to enable "delayed" filtering. | ||
| pub(crate) parent: Option<PackageName>, |
There was a problem hiding this comment.
I'm also not 100% sure on the implications here...
package-level conflictspackage-level conflicts in workspaces
39f6ac4 to
9f23e5f
Compare
Revives #9130
Previously, we allowed scoping conflicting extras or groups to specific packages, e.g. ,
{ package = "foo", extra = "bar" }for a conflict infoo[bar]. Now, we allow dropping theextraorgroupbit and using{ package = "foo" }directly which declares a conflict withfoo's production dependencies.This means you can declare conflicts between workspace members, e.g.:
would not allow
fooandbarto be installed at the same time.Similarly, a conflict can be declared between a package and a group:
which would mean, e.g., that
--only-group lintwould be required for the invocation.As with our existing support for conflicting extras, there are edge-cases here where the resolver will not fail even if there are conflicts that render a particular install target unusable. There's test coverage for some of these. We'll still error at install-time when the conflicting groups are selected. Due to the likelihood of bugs in this feature, I've marked it as a preview feature.
I would not recommend reading the commits as there's some slop from not wanting to rebase Andrew's branch.