Model groups as a property of requirements #9545
Conversation
charliermarsh
added
the
internal
charliermarsh
force-pushed
the
charlie/transitive
branch
2 times, most recently
from
215cce4 to
7413668
Compare
charliermarsh
force-pushed
the
charlie/group
branch
from
320fb55 to
d79161d
Compare
charliermarsh
force-pushed
the
charlie/transitive
branch
from
7413668 to
374649f
Compare
charliermarsh
force-pushed
the
charlie/group
branch
from
d79161d to
1c972a9
Compare
charliermarsh
force-pushed
the
charlie/transitive
branch
from
374649f to
042f88a
Compare
charliermarsh
force-pushed
the
charlie/group
branch
2 times, most recently
from
42353b6 to
0f2d004
Compare
| And because foo depends on anyio==4.1.0, we can conclude that bar and foo are incompatible. | ||
| And because your workspace requires bar and foo, we can conclude that your workspace's requirements are unsatisfiable. | ||
| ╰─▶ Because bar:dev depends on anyio==4.2.0 and foo depends on anyio==4.1.0, we can conclude that foo and bar:dev are incompatible. | ||
| And because your workspace requires bar:dev and foo, we can conclude that your workspace's requirements are unsatisfiable. |
There was a problem hiding this comment.
\cc @zanieb -- I think you pointed out this general problem long ago.
charliermarsh
force-pushed
the
charlie/group
branch
from
0f2d004 to
212f9e8
Compare
charliermarsh
force-pushed
the
charlie/group
branch
from
212f9e8 to
d83f066
Compare
| } else { | ||
| Either::Right(requirement.extras.clone().into_iter().map(Some)) | ||
| Either::Right(iter::once((None, None))) |
There was a problem hiding this comment.
As with #9582, I suspect you'll want to do the same thing for groups as I did for extras: conditionally add the base package dependency if the group is declared as conflicting.
| .extras | ||
| .clone() | ||
| .into_iter() | ||
| .map(|extra| (Some(extra), None)), |
There was a problem hiding this comment.
If requirement.groups isn't empty at this point, does this mean that all of those groups get skipped?
There was a problem hiding this comment.
The assumption is that they’re mutually exclusive
There was a problem hiding this comment.
Interesting. Hard to tell from the type definition. But encoding that invariant in the types is probably annoying.
There was a problem hiding this comment.
I could do it? It's a bit tedious because these also get serialized so we'd want to maintain backwards compatibility, but I could do it. Somewhat prefer to just document it clearly though.
There was a problem hiding this comment.
Documenting it seems fine, yeah.
I am pretty much always torn about how much to encode in the types. I do like it, because I find it makes refactoring without fear a lot easier. It prevents you from getting the case analysis wrong. But as you rightly point out, it's a pain. And if the invariants change, it can make refactoring a pain (albeit higher confidence) too.
It's a constant balancing act.
charliermarsh
force-pushed
the
charlie/group
branch
2 times, most recently
from
7904a3d to
4848055
Compare
charliermarsh
force-pushed
the
charlie/group
branch
from
4848055 to
7e9042f
Compare
Summary
Today, our dependency group implementation is a little awkward... For each package
P, we check ifPcontains dependencies for each enabled group, then add a dependency onPwith the group enabled. There are a few issues here:PtoPwith the group enabled. ThenPwith the group enabled adds a dependency on the base package.Instead, our internal requirement type can now include dependency groups, which makes dependency groups look much, much more like extras in the resolver.