Skip to content

Combine non-installable & non-upgradable packages #8712

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Feb 28, 2023
Merged

Combine non-installable & non-upgradable packages #8712

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
45aba44
Combine non-installable & non-upgradable packages
wismill Jan 30, 2023
feb52ad
Typo
wismill Feb 1, 2023
adfce24
Remove requireInstalled
wismill Feb 1, 2023
38785a6
Revert "Remove requireInstalled"
wismill Feb 1, 2023
3f9c53c
Comment only [skip ci]
wismill Feb 4, 2023
2a032fa
Merge branch 'master' into wip/fix8581
ulysses4ever Feb 26, 2023
88e768b
Merge branch 'master' into wip/fix8581
mergify[bot] Feb 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cabal-install-solver/src/Distribution/Solver/Modular.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ import Distribution.Verbosity
-- solver. Performs the necessary translations before and after.
modularResolver :: SolverConfig -> DependencyResolver loc
modularResolver sc (Platform arch os) cinfo iidx sidx pkgConfigDB pprefs pcs pns =
fmap (uncurry postprocess) $ -- convert install plan
uncurry postprocess <$> -- convert install plan
solve' sc cinfo idx pkgConfigDB pprefs gcs pns
where
-- Indices have to be converted into solver-specific uniform index.
Expand Down Expand Up @@ -275,7 +275,7 @@ tryToMinimizeConflictSet runSolver sc cs cm =
++ "conflict set: " ++ showCS cs') $
ExhaustiveSearch smallestKnownCS smallestKnownCM
BackjumpLimitReached ->
failWith ("Reached backjump limit while minimizing conflict set.")
failWith "Reached backjump limit while minimizing conflict set."
BackjumpLimitReached
where
varStr = "\"" ++ showVar v ++ "\""
Expand Down
20 changes: 2 additions & 18 deletions cabal-install-solver/src/Distribution/Solver/Modular/Solver.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ data SolverConfig = SolverConfig {
shadowPkgs :: ShadowPkgs,
strongFlags :: StrongFlags,
allowBootLibInstalls :: AllowBootLibInstalls,
nonInstallablePackages :: [PackageName],
onlyConstrained :: OnlyConstrained,
maxBackjumps :: Maybe Int,
enableBackjumping :: EnableBackjumping,
Expand Down Expand Up @@ -142,7 +143,7 @@ solve sc cinfo idx pkgConfigDB userPrefs userConstraints userGoals =
prunePhase = (if asBool (avoidReinstalls sc) then P.avoidReinstalls (const True) else id) .
(if asBool (allowBootLibInstalls sc)
then id
else P.requireInstalled (`elem` nonInstallable)) .
else P.requireInstalled (`elem` nonInstallablePackages sc)) .
(case onlyConstrained sc of
OnlyConstrainedAll ->
P.onlyConstrained pkgIsExplicit
Expand All @@ -155,23 +156,6 @@ solve sc cinfo idx pkgConfigDB userPrefs userConstraints userGoals =
pkgIsExplicit :: PN -> Bool
pkgIsExplicit pn = S.member pn allExplicit

-- packages that can never be installed or upgraded
-- If you change this enumeration, make sure to update the list in
-- "Distribution.Client.Dependency" as well
nonInstallable :: [PackageName]
nonInstallable =
L.map mkPackageName
[ "base"
, "ghc-bignum"
, "ghc-prim"
, "ghc-boot"
, "ghc"
, "ghci"
, "integer-gmp"
, "integer-simple"
, "template-haskell"
]

-- When --reorder-goals is set, we use preferReallyEasyGoalChoices, which
-- prefers (keeps) goals only if the have 0 or 1 enabled choice.
--
Expand Down
36 changes: 22 additions & 14 deletions cabal-install/src/Distribution/Client/Dependency.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,24 +395,31 @@ dontUpgradeNonUpgradeablePackages params =
(PackageConstraint (ScopeAnyQualifier pkgname) PackagePropertyInstalled)
ConstraintSourceNonUpgradeablePackage
| Set.notMember (mkPackageName "base") (depResolverTargets params)
-- If you change this enumeration, make sure to update the list in
-- "Distribution.Solver.Modular.Solver" as well
, pkgname <- [ mkPackageName "base"
, mkPackageName "ghc-bignum"
, mkPackageName "ghc-prim"
, mkPackageName "ghc-boot"
, mkPackageName "ghc"
, mkPackageName "ghci"
, mkPackageName "integer-gmp"
, mkPackageName "integer-simple"
, mkPackageName "template-haskell"
]
, pkgname <- nonUpgradeablePackages
, isInstalled pkgname ]

isInstalled = not . null
. InstalledPackageIndex.lookupPackageName
(depResolverInstalledPkgIndex params)

-- NOTE: the lists of non-upgradable and non-installable packages used to be
-- respectively in this module and in `Distribution.Solver.Modular.Solver`.
-- Since they were kept synced, they are now combined in the following list.
--
-- See: https://github.com/haskell/cabal/issues/8581
nonUpgradeablePackages :: [PackageName]
nonUpgradeablePackages =
[ mkPackageName "base"
, mkPackageName "ghc-bignum"
, mkPackageName "ghc-prim"
, mkPackageName "ghc-boot"
, mkPackageName "ghc"
, mkPackageName "ghci"
, mkPackageName "integer-gmp"
, mkPackageName "integer-simple"
, mkPackageName "template-haskell"
]

addSourcePackages :: [UnresolvedSourcePackage]
-> DepResolverParams -> DepResolverParams
addSourcePackages pkgs params =
Expand Down Expand Up @@ -469,7 +476,7 @@ removeBounds relKind relDeps params =
}
where
sourcePkgIndex' :: PackageIndex.PackageIndex UnresolvedSourcePackage
sourcePkgIndex' = fmap relaxDeps $ depResolverSourcePkgIndex params
sourcePkgIndex' = relaxDeps <$> depResolverSourcePkgIndex params

relaxDeps :: UnresolvedSourcePackage -> UnresolvedSourcePackage
relaxDeps srcPkg = srcPkg
Expand Down Expand Up @@ -710,7 +717,8 @@ resolveDependencies platform comp pkgConfigDB solver params =
$ fmap (validateSolverResult platform comp indGoals)
$ runSolver solver (SolverConfig reordGoals cntConflicts fineGrained minimize
indGoals noReinstalls
shadowing strFlags allowBootLibs onlyConstrained_ maxBkjumps enableBj
shadowing strFlags allowBootLibs nonUpgradeablePackages
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add some comment as to why the nonInstallablePackages field is initialized with nonUpgradeablePackages (there’s a clear mismatch here)? Perhaps, reference a place where the difference between noninstallable and nonupgradeable is discussed? @grayjay, do you happen to know such a place? Or if there’s none, perhaps you could help with writing the comment I’m asking for. This stuff is very confusing and I hope we can write up something that could clear things up for the future reference.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ulysses4ever I think that non-installable and non-upgradable may have the exact same meaning, which is the reason I opened #8581. If this PR is merged as is, then I think it can just have a comment pointing to #8581. If it is possible to remove requireInstalled, then this part of the change can be reverted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment

Copy link
Collaborator

@ulysses4ever ulysses4ever Feb 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we agree that "non-installable" and "non-upgradable" are the same, can we please rename one of these into the other?

@wismill thanks for the comment. Can you also update the comment on nonUpgradeablePackages to say that they are the same?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ulysses4ever I'm not completely sure that non-installable and non-upgradable are the same. I wasn't able to determine the difference by looking at the history of the code, so I think that the best way to answer the question is to try to remove one of them and see whether the tests pass. I think it would be better to just link to issue #8581 until we can try removing the rest of the redundant code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current code fullfils @grayjay comment, so I will wait you reach an agreement 😉.
Once this PR is merged, I can try the proposed follow-up.

onlyConstrained_ maxBkjumps enableBj
solveExes order verbosity (PruneAfterFirstSuccess False))
platform comp installedPkgIndex sourcePkgIndex
pkgConfigDB preferences constraints targets
Expand Down