Don't override a package if all versions are deprecated

[amesgen]

Closes #8502, specifically by implementing #8502 (comment).

TODO:

• Consensus that these semantics make sense (reaction to ttps://github.com/Name security in additional package repositories #8502#issuecomment-1262067495 is positive)
• Maybe it is better to introduce a new modifier (eg :override-nondeprecated) rather than to change an existing one?
• Tests
• Documentation

---

I don't see any existing tests for the active-repositories functionality; hence, here is how I tested these changes with a custom repo:

This repo contains a single package, namely tagged-0.8.6.1, which is marked as deprecated. https://github.com/amesgen/cabal-repo-override-example now contains a Cabal project with

active-repositories:
  , hackage.haskell.org
  , cabal-repo-override-example:override

Now, with e.g. GHC 9.2.8, running cabal update and cabal build will result in the following version of tagged being used:

• With cabal-3.10.1.0: tagged-0.8.6.1. This is because only packages from cabal-repo-override-example are considered.
• With cabal built from this PR: tagged-0.8.7, the (currently) newest version on Hackage, as now also versions from Hackage are considered.

Also, to check that :override still works when not all versions are deprecated, change the index-state of cabal-repo-override-example to 2023-06-02T18:10:14Z where tagged-0.8.6.1 was not yet deprecated, and observe that also with the cabal from this PR, it is then chosen by cabal build.

---

Will write a changelog entry and update the docs if the implementation approach looks okay.

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary.
• Include manual QA notes if your PR relates to cabal-install.

Bonus points for added automated tests!

[andreabedini]

Thanks @amesgen !

I think this is a good chance to add some tests for active-repositories 🙂 you can follow along other tests in test-suite and ask here if something is not clear.

Looking at the implementation. Is there any chance this can be implemented purely in IndexUtils?

If the already exported merge and override functions are not enough, maybe PackageIndex can export an mergeWith function?

[amesgen]

I think this is a good chance to add some tests for active-repositories slightly_smiling_face you can follow along other tests in test-suite and ask here if something is not clear.

Will try to take a stab 👍

Looking at the implementation. Is there any chance this can be implemented purely in IndexUtils?

If the already exported merge and override functions are not enough, maybe PackageIndex can export an mergeWith function?

The already existing merge and override functions are not enough by themselves; but there are slightly different approaches of course:

• Delete all packages that should be overriden (i.e. those with at least one non-deprecated version) using deletePackageName from acc and then merge idx into it.
• Generalize overrideOrMerge to a mergeWith (or mergeWithKey) function as suggested, with a type signature like

  mergeWith ::
    (Package pkg) =>
    (forall pkgs. (Semigroup pkgs) => PackageName -> pkgs -> pkgs -> pkgs) ->
    PackageIndex pkg ->
    PackageIndex pkg ->
    PackageIndex pkg

  or

  mergeWith ::
    (Package pkg) =>
    (PackageName -> SortedByPkgId pkg -> SortedByPkgId pkg -> SortedByPkgId pkg) ->
    PackageIndex pkg ->
    PackageIndex pkg ->
    PackageIndex pkg

  where the Semigroup for pkgs or SortedByPkgId would be backed by mergeBuckets.

I don't have a preference, happy to go with any of these (or alternative) ways.

[andreabedini]

I had a look at the code. The problem is that we merge "by-index" not "by-package". This wasn't clear to me. So what if we pre-process RepoData to remove from rdIndex the packages that are entirely deprecated according to rdPrefs? In that way we don't have to worry when we merge (there's no package that can override anything).

But now I am worried about the comment at the end of the function:

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

Lines 399 to 407 in a191f0a

	-- Note: preferences combined without using CombineStrategy
	let prefs :: Map PackageName VersionRange
	prefs =
	Map.fromListWith
	intersectVersionRanges
	[ (name, range)
	| (RepoData _n _ts _idx prefs', _strategy) <- pkgss'
	, Dependency name range _ <- prefs'
	]

It seems as deprecating a package version in a repo will deprecate it in any other repo anyway. It's really unclear what the semantic should be here. When overriding, it would be natural to assume that packages "bring along" their preferences (so if pkg-a comes from repo-1, pkg-a prefs will be exactly the ones specified by repo-1). But what about merging? I guess it can only make sense if the preferences are a conjunction of notThisVersion 🤔

[amesgen]

So what if we pre-process RepoData to remove from rdIndex the packages that are entirely deprecated according to rdPrefs? In that way we don't have to worry when we merge (there's no package that can override anything).

Right now, deprecating a version will just tell the solver to try to find a plan that does not include it, but if it can not do that, deprecated versions can still end up in the build plan. Your suggestion would mean that deprecated versions vanish entirely, which I am not necessarily opposed to (also see #8863), but it might be out-of-scope for this particular PR (but OTOH, your suggestion would solve #8502 as a side effect, superseding this PR).

[andreabedini]

Right now, deprecating a version will just tell the solver to try to find a plan that does not include it, but if it can not do that, deprecated versions can still end up in the build plan.

You are right. I am clearly confused about how this works :-/ I'll familiarise myself with the code ...

[Mikolaj]

Hello! Is there a way to move this PR forward? Can we help?

[amesgen]

Hello! Is there a way to move this PR forward? Can we help?

One thing would be to turn the test scenario described in the PR description into a proper test in Cabal; I sadly don't think I currently have the motivation to do that myself. Last time I had a look, there were no tests involving the active-repositories feature, so some infrastructure for setting those up might be required.

[erikd]

When the added active-repositories test in #11684 gets merged, I will rebase this and help push it though.