launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

I think I've sorted all this out.  Please re-review.

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.

I did indeed end up turning this into planPackageDomination.  At that point I felt it made more sense to have the returned plan be in the present tense (well, imperative really), since it hasn't been executed yet.

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

I've done all of this apart from removing the override matches from getOtherPublications, mainly because working out whether that was sound required a good deal more thinking and it doesn't seem immediately necessary.

> +
>          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)

I've done some of this factoring out of common code, although there's some more left.  That doesn't seem particularly urgent and could be done some other time.

It's not completely clear to me whether the first pass can supersede an arch-indep pub; it could happen if the first in the sorted list were arch-indep, I think.  But in any case the way I've done this refactoring means that it doesn't really matter.

>  
>      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.