Fix qualifiers in cabal freeze #9811
Conversation
Add tests for haskell#9799 about freeze qualifying all packages with 'any' constraint scope.
alt-romes
force-pushed
the
wip/romes/9799-3
branch
2 times, most recently
from
ac13f9a
to
ac435fd
Compare
Qualifiers in the cabal freeze should definitely not always be "any".
(it can cause inconsistencies, such as
`cabal freeze --constraint=... && cabal build`
being different from
`cabal build --constraint=...`
)
In this commit we propagate information on the scopes in which each
package was solved to be able to generate a proper freeze file where
each package solved is constrained in the scope it was solved in.
Fixes haskell#9799
alt-romes
force-pushed
the
wip/romes/9799-3
branch
from
ac435fd
to
4f4f0f9
Compare
This reverts commit 65d880e.
This reverts commit 2ace585.
in the graph, the packages would already be distinct because if two different versions were picked they would get two different unit ids. this change makes it possible to have two different nodes in the map with the same unit-id (e.g. solver linked the setup and top-level package instances). this is not good
This reverts commit 4953db1.
There was a problem hiding this comment.
Despite these comments, I recall trying this and realizing it was not a good idea.
Here was the attempt at using (UnitId, ConstraintScope) as Key and extending SolverId with the scope: 4953db1
The comment I wrote at the time saying that this is wrong is:
In the graph, the packages would already be distinct because if two different versions were picked they would get two different unit ids. This change makes it possible to have two different nodes in the map with the same unit-id (e.g. solver linked the setup and top-level package instances). This is not good.
Why is this not good? I don't recall.
| -- The constraint scope field refers to the scope in which this package was resolved. | ||
| data ResolverPackage loc = PreExisting InstSolverPackage ConstraintScope | ||
| | Configured (SolverPackage loc) ConstraintScope |
There was a problem hiding this comment.
The packages that are part of the plan now also store information about the scope they were resolved in. This information is propagated down to cabal-install (see GenericInstallPlan).
| data GenericPlanPackage ipkg srcpkg | ||
| = PreExisting ipkg | ||
| | Configured srcpkg | ||
| | Installed srcpkg | ||
| = PreExisting ipkg ConstraintScope | ||
| | Configured srcpkg ConstraintScope | ||
| | Installed srcpkg ConstraintScope |
There was a problem hiding this comment.
In cabal-install our understanding of the install plan must also consider, for every package in the plan, the scope it was solved in.
| type Key (ResolverPackage loc) = SolverId | ||
| nodeKey (PreExisting ipkg) = PreExistingId (packageId ipkg) (installedUnitId ipkg) | ||
| nodeKey (Configured spkg) = PlannedId (packageId spkg) | ||
| nodeKey (PreExisting ipkg _) = PreExistingId (packageId ipkg) (installedUnitId ipkg) | ||
| nodeKey (Configured spkg _) = PlannedId (packageId spkg) |
There was a problem hiding this comment.
The problem is that when we construct a graph of resolver packages in the solver the keys don't have this information (even though the nodes do); thus same packages resolved in different constraint scopes will be collapsed and only one copy will survive.
In cabal-install, when we construct a GenericInstallPlan from the graph of resolver packages we have only kept one of each package, even if originally many could have existed in different scopes.
| type Key (GenericPlanPackage ipkg srcpkg) = UnitId | ||
| nodeKey (PreExisting ipkg) = nodeKey ipkg | ||
| nodeKey (Configured spkg) = nodeKey spkg | ||
| nodeKey (Installed spkg) = nodeKey spkg | ||
| nodeNeighbors (PreExisting ipkg) = nodeNeighbors ipkg | ||
| nodeNeighbors (Configured spkg) = nodeNeighbors spkg | ||
| nodeNeighbors (Installed spkg) = nodeNeighbors spkg | ||
| nodeKey (PreExisting ipkg _) = nodeKey ipkg | ||
| nodeKey (Configured spkg _) = nodeKey spkg | ||
| nodeKey (Installed spkg _) = nodeKey spkg |
There was a problem hiding this comment.
And here we lose this information as well..., a UnitId refers only to a package, not the Scope in which we use this UnitId
Qualifiers in the cabal freeze should definitely not always be "any".
Fixes #9799
Template Α: This PR modifies
cabalbehaviourInclude the following checklist in your PR: