launchpad-reviewers team mailing list archive

[Colin Watson <mp+448906@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:ci-upload-by-release into launchpad:master with ~cjwatson/launchpad:ci-build-track-report-das as a prerequisite.

Commit message:
Restrict CIBuildUploadJob to binaries built for the target series

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/448906

The `CIBuildUploadJob` mechanism (used by `Archive.uploadCIBuild`) presumes that it makes sense to upload all the binaries produced by a given CI build to the same context.  However, it seems that for the project where we're using this most heavily at the moment, the security team would strongly prefer to build both focal and jammy binaries from the same CI build (using `series: focal` and `series: jammy` in individual CI jobs), reducing the number of git branches they need to maintain.  For this to work properly, we need to filter the uploads: it's unlikely to make sense to publish Python 3.8-based wheels to jammy, or Python 3.10-based wheels to focal.

Making this work requires tracking the series and architecture for each "revision status report" (corresponding to individual CI jobs, or to rows displayed in the web UI under each revision).  With that in place, we can skip binaries built for the wrong series.

There's an additional wrinkle: we currently rely on being able to build on (e.g.) Ubuntu focal and upload the results to (e.g.) <private distribution> focal.  These series unfortunately don't have anything formally linking them other than the name, so right now the only option seems to be to compare series names.  This is an unpleasant hack, but fortunately `CIBuildUploadJob` isn't particularly deep in the data model so it should be viable to fix this later.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:ci-upload-by-release into launchpad:master.

diff --git a/lib/lp/soyuz/model/archivejob.py b/lib/lp/soyuz/model/archivejob.py
index 64df214..8c9ff5a 100644
--- a/lib/lp/soyuz/model/archivejob.py
+++ b/lib/lp/soyuz/model/archivejob.py
@@ -795,7 +795,30 @@ class CIBuildUploadJob(ArchiveJobDerived):
             metadata = scanned_artifact.metadata
             if not isinstance(metadata, BinaryArtifactMetadata):
                 continue
-            library_file = scanned_artifact.artifact.library_file
+            artifact = scanned_artifact.artifact
+            library_file = artifact.library_file
+            # XXX cjwatson 2023-08-09: Comparing distroseries names here is
+            # a pretty unpleasant hack, but it's necessary because in
+            # practice what we're doing is taking the output of build jobs
+            # that were run on (e.g.) Ubuntu focal and uploading them to
+            # (e.g.) <private distribution> focal.  Unfortunately the
+            # private series doesn't have its parent series set to the
+            # corresponding Ubuntu series, so we have no way to make the
+            # connection other than comparing names.  We need to figure out
+            # something better here, but at least this hack isn't too deep
+            # in the core of the system.
+            if (
+                artifact.report.distro_arch_series is not None
+                and artifact.report.distro_arch_series.distroseries.name
+                != self.target_distroseries.name
+            ):
+                logger.info(
+                    "Skipping %s (built for %s, not %s)",
+                    library_file.filename,
+                    artifact.report.distro_arch_series.distroseries.name,
+                    self.target_distroseries.name,
+                )
+                continue
             logger.info(
                 "Uploading %s to %s %s (%s)",
                 library_file.filename,
diff --git a/lib/lp/soyuz/tests/test_archivejob.py b/lib/lp/soyuz/tests/test_archivejob.py
index a29fc89..4592b7a 100644
--- a/lib/lp/soyuz/tests/test_archivejob.py
+++ b/lib/lp/soyuz/tests/test_archivejob.py
@@ -1654,6 +1654,109 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
 
         self.assertEqual(JobStatus.COMPLETED, job.job.status)
 
+    def test_skips_binaries_for_wrong_series(self):
+        # A CI job might build binaries on multiple series.  When uploading
+        # the results to an archive, only the binaries for the corresponding
+        # series are selected.
+        #
+        # The build distribution is always Ubuntu for now, but the target
+        # distribution may differ.  Unfortunately, this currently requires
+        # matching on series names.
+        logger = self.useFixture(FakeLogger())
+        target_distribution = self.factory.makeDistribution()
+        archive = self.factory.makeArchive(
+            distribution=target_distribution,
+            repository_format=ArchiveRepositoryFormat.PYTHON,
+        )
+        ubuntu_distroserieses = [
+            self.factory.makeUbuntuDistroSeries() for _ in range(2)
+        ]
+        target_distroserieses = [
+            self.factory.makeDistroSeries(
+                distribution=target_distribution,
+                name=ubuntu_distroseries.name,
+                version=ubuntu_distroseries.version,
+            )
+            for ubuntu_distroseries in ubuntu_distroserieses
+        ]
+        processor = self.factory.makeProcessor()
+        ubuntu_dases = [
+            self.factory.makeDistroArchSeries(
+                distroseries=ubuntu_distroseries,
+                architecturetag=processor.name,
+                processor=processor,
+            )
+            for ubuntu_distroseries in ubuntu_distroserieses
+        ]
+        target_dases = [
+            self.factory.makeDistroArchSeries(
+                distroseries=target_distroseries,
+                architecturetag=processor.name,
+                processor=processor,
+            )
+            for target_distroseries in target_distroserieses
+        ]
+        build = self.makeCIBuild(
+            target_distribution, distro_arch_series=ubuntu_dases[0]
+        )
+        reports = [
+            build.getOrCreateRevisionStatusReport(
+                "build-wheel:%d" % i, distro_arch_series=ubuntu_das
+            )
+            for i, ubuntu_das in enumerate(ubuntu_dases)
+        ]
+        # We wouldn't normally expect to see only an
+        # architecture-independent package in one series and only an
+        # architecture-dependent package in another, but these test files
+        # are handy.
+        paths = [
+            "wheel-indep/dist/wheel_indep-0.0.1-py3-none-any.whl",
+            "wheel-arch/dist/wheel_arch-0.0.1-cp310-cp310-linux_x86_64.whl",
+        ]
+        for report, path in zip(reports, paths):
+            with open(datadir(path), mode="rb") as f:
+                report.attach(name=os.path.basename(path), data=f.read())
+        job = CIBuildUploadJob.create(
+            build,
+            build.git_repository.owner,
+            archive,
+            target_distroserieses[0],
+            PackagePublishingPocket.RELEASE,
+            target_channel="edge",
+        )
+        transaction.commit()
+
+        with dbuser(job.config.dbuser):
+            JobRunner([job]).runAll()
+
+        self.assertThat(
+            archive.getAllPublishedBinaries(),
+            MatchesSetwise(
+                MatchesStructure(
+                    binarypackagename=MatchesStructure.byEquality(
+                        name="wheel-indep"
+                    ),
+                    binarypackagerelease=MatchesStructure(
+                        ci_build=Equals(build),
+                        binarypackagename=MatchesStructure.byEquality(
+                            name="wheel-indep"
+                        ),
+                        version=Equals("0.0.1"),
+                        binpackageformat=Equals(BinaryPackageFormat.WHL),
+                    ),
+                    binarypackageformat=Equals(BinaryPackageFormat.WHL),
+                    distroarchseries=Equals(target_dases[0]),
+                )
+            ),
+        )
+        self.assertEqual(JobStatus.COMPLETED, job.job.status)
+        self.assertIn(
+            "Skipping wheel_arch-0.0.1-cp310-cp310-linux_x86_64.whl (built "
+            "for %s, not %s)"
+            % (ubuntu_distroserieses[1].name, target_distroserieses[0].name),
+            logger.output.splitlines(),
+        )
+
     def test_run_failed(self):
         # A failed run sets the job status to FAILED and notifies the
         # requester.