Try to combine lists of non-installable and non-upgradable packages

[grayjay]

Issue #8489 showed that cabal contains two separate lists of packages that cannot be installed. This issue is for determining whether the lists have the same meaning and combining them if they are redundant. #8501 synced the contents of the two lists.

One is in the solver and is described as non-installable:

cabal/cabal-install-solver/src/Distribution/Solver/Modular/Solver.hs

Lines 158 to 173 in 09c90de

	-- 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"
	]

The other is in Distribution.Client.Dependency and is described as non-upgradable:

cabal/cabal-install/src/Distribution/Client/Dependency.hs

Lines 387 to 414 in 09c90de

	-- | Some packages are specific to a given compiler version and should never be
	-- upgraded.
	dontUpgradeNonUpgradeablePackages :: DepResolverParams -> DepResolverParams
	dontUpgradeNonUpgradeablePackages params =
	addConstraints extraConstraints params
	where
	extraConstraints =
	[ LabeledPackageConstraint
	(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"
	]
	, isInstalled pkgname ]
	isInstalled = not . null
	. InstalledPackageIndex.lookupPackageName
	(depResolverInstalledPkgIndex params)

If only one is needed, I think we should keep the one in Distribution.Client.Dependency, since it is better to make the solver more general and pass in constraints rather than hard code package names.

[wismill]

Does somebody has this info?

Else now that the two lists are synced, we could give it a try with proper comments. It would be easy to revert and desync the two list again if there is some issue. I think this would help addressing #7993. In what module should this list go?

[Mikolaj]

@wismill: thank you for taking interest and the willingness to contribute. I hope at some point @grayjay or @gbaz find a moment to conspire with you. If not, please ping me and I will try to find help elsewhere.

[grayjay]

@wismill I think it would be fine to try out this change and see whether the tests pass. I think that we should keep the list in Distribution.Client.Dependency. A fix for #7993 could use the ConstraintSource (ConstraintSourceNonUpgradeablePackage) to determine whether a package version is disallowed because it is not upgradable.

It may also help to add more unit tests to this test group:

cabal/cabal-install/tests/UnitTests/Distribution/Solver/Modular/Solver.hs

Lines 151 to 162 in 605a3c6

	, testGroup "Base and Nonupgradable" [
	runTest $ mkTest dbBase "Refuse to install base without --allow-boot-library-installs" ["base"] $
	solverFailure (isInfixOf "only already installed instances can be used")
	, runTest $ allowBootLibInstalls $ mkTest dbBase "Install base with --allow-boot-library-installs" ["base"] $
	solverSuccess [("base", 1), ("ghc-prim", 1), ("integer-gmp", 1), ("integer-simple", 1)]
	, runTest $ mkTest dbNonupgrade "Refuse to install newer ghc requested by another library" ["A"] $
	solverFailure (isInfixOf "rejecting: ghc-2.0.0 (constraint from non-upgradeable package requires installed instance)")
	, runTest $ mkTest dbNonupgrade "Refuse to install newer ghci requested by another library" ["B"] $
	solverFailure (isInfixOf "rejecting: ghci-2.0.0 (constraint from non-upgradeable package requires installed instance)")
	, runTest $ mkTest dbNonupgrade "Refuse to install newer ghc-boot requested by another library" ["C"] $
	solverFailure (isInfixOf "rejecting: ghc-boot-2.0.0 (constraint from non-upgradeable package requires installed instance)")
	]

I think that the test "Refuse to install base without --allow-boot-library-installs" could at least be improved by testing for a larger part of the error message that rejects base. The error message may also change due to this refactoring.

[wismill]

@grayjay Distribution.Client.Dependency is in cabal-install, which depends on cabal-install-solver. Therefore the list should be in cabal-install-solver, should not it?

[grayjay]

@wismill I think that the list should stay in Distribution.Client.Dependency, because it is better to keep the solver more general and pass in constraints on specific packages. Handling "installed" constraints similarly to other constraints that are passed to the solver also simplifies the code.

[wismill]

@grayjay Then correct me if I am wrong, but given the package dependency we can close this issue as non-fixable.

[grayjay]

The list in Distribution.Client.Dependency is already passed into and used by the solver. The issue is that the solver uses two lists of packages (the other in Distribution.Solver.Modular.Solver), so they probably provide redundant functionality.

[grayjay]

Reopening, because the requireInstalled function can probably also be removed, because it is redundant with enforcing the installed constraints that are added in Distribution.Client.Dependency:

cabal/cabal-install-solver/src/Distribution/Solver/Modular/Preference.hs

Lines 321 to 322 in ab24689

	-- | Require installed packages.
	requireInstalled :: (PN -> Bool) -> EndoTreeTrav d c