launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Diff comments:

> === modified file 'lib/lp/archivepublisher/domination.py'
> --- lib/lp/archivepublisher/domination.py	2014-10-31 10:34:51 +0000
> +++ lib/lp/archivepublisher/domination.py	2019-05-28 23:48:46 +0000
> @@ -397,6 +397,17 @@
>              are listed in `live_versions` are marked as Deleted.
>          :param generalization: A `GeneralizedPublication` helper representing
>              the kind of publications these are: source or binary.
> +        :param immutable_check: If True, fail if a deletion would modify an
> +            immutable suite (e.g. the RELEASE pocket of a CURRENT series).
> +        :param superseded: If not None, append (superseded publication,
> +            dominant publication) pairs to this list when marking a
> +            publication as superseded.  Binary domination uses this to
> +            supersede other publications associated with the superseded
> +            ones.
> +        :param keep: If not None, add publications that have been confirmed
> +            as live to this set.  Binary domination uses this to ensure that
> +            these live publications are not superseded when superseding
> +            associated publications.

Any reason not to have superseded/keep as return values? It'll be slightly longer in callsites, but it's really not very obvious that they're output parameters. From the perspective of this function they're also both in the past: "superseded" and "kept".

I also wonder if at this point it becomes cleaner to have dominatePackage just return its assessment of the publications rather than actually act on them (planPackageDomination?). Three different places now invoke BPPH.supersede(), and all the information needed for the callsite to perform all actions is passed up except for deletions.

>          """
>          live_versions = frozenset(live_versions)
>  
> @@ -630,11 +653,19 @@
>                  self.logger.debug("Dominating %s" % name)
>                  assert len(pubs) > 0, "Dominating zero binaries!"
>                  live_versions = find_live_binary_versions_pass_1(pubs)
> -                self.dominatePackage(pubs, live_versions, generalization)
> +                self.dominatePackage(
> +                    pubs, live_versions, generalization,
> +                    superseded=superseded, keep=keep)
>                  if contains_arch_indep(pubs):
>                      packages_w_arch_indep.add(name)
>  
> +        self.logger.info("Dominating associated binaries...")
> +        for pub, dominant in superseded:
> +            pub.supersedeAssociated(dominant, keep=keep, logger=self.logger)

"Associated" isn't an existing term in Soyuz AFAIK. It could use a comment to explain what's going on here and why we're doing it at such a high level.

And I wonder if supersede(supersede_associated=True) even makes sense any more. Encapsulating that logic in the model made sense when it was simple and could be encapsulated there, but it can't any more. BPPH._getOtherPublications would need to be renamed, cleaned up, and made public, but that's a cleaner interface than this weird supersede_associated=True mode which isn't a sensible operation at all. _getOtherPublications can probably also lose the override matches, as that rule is only there to prevent the latest publication from accidentally superseding itself, and the whole purpose of this branch is to do that in a more robust way.

This becomes particularly clean if we pull even non-associated BPPH.supersede() calls out to here. You just loop over superseded, call _getOtherPublications, subtract keep, and invoke supersede() on the pub and the surviving others.

> +
>          packages_w_arch_indep = frozenset(packages_w_arch_indep)
> +        superseded = []
> +        keep = set()
>  
>          # The second pass attempts to supersede arch-all publications of
>          # older versions, from source package releases that no longer
> @@ -655,7 +686,13 @@
>                  assert len(pubs) > 0, "Dominating zero binaries in 2nd pass!"
>                  live_versions = find_live_binary_versions_pass_2(
>                      pubs, reprieve_cache)
> -                self.dominatePackage(pubs, live_versions, generalization)
> +                self.dominatePackage(
> +                    pubs, live_versions, generalization,
> +                    superseded=superseded, keep=keep)
> +
> +        self.logger.info("Dominating associated binaries...(2nd pass)")
> +        for pub, dominant in superseded:
> +            pub.supersedeAssociated(dominant, keep=keep, logger=self.logger)

This seems like a pretty acceptable time to factor out the common bits of the two passes.

Though, in fact, can't the first pass never superseded an arch-indep pub? It finds sources that have them to seed the second pass, but find_live_binary_versions_pass_1 only includes architecture-specific.

>  
>      def _composeActiveSourcePubsCondition(self, distroseries, pocket):
>          """Compose ORM condition for restricting relevant source pubs."""


-- 
https://code.launchpad.net/~cjwatson/launchpad/multiple-override-race/+merge/368023
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.