launchpad-reviewers team mailing list archive

[Jürgen Gmach <mp+416223@xxxxxxxxxxxxxxxxxx>]

@Colin: While the biggest part of the code is still the one you prepared, maybe you can have a look at what I added and give me feedback on my suggestions.

Diff comments:

> diff --git a/lib/lp/code/model/cibuild.py b/lib/lp/code/model/cibuild.py
> index f9c9031..e5893b3 100644
> --- a/lib/lp/code/model/cibuild.py
> +++ b/lib/lp/code/model/cibuild.py
> @@ -329,6 +337,136 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
>          store.flush()
>          return cibuild
>  
> +    def findByGitRepository(self, git_repository, commit_sha1s=None):
> +        """See `ICIBuildSet`."""
> +        clauses = [CIBuild.git_repository == git_repository]
> +        if commit_sha1s is not None:
> +            clauses.append(CIBuild.commit_sha1.is_in(commit_sha1s))
> +        return IStore(CIBuild).find(CIBuild, *clauses)
> +
> +    def _isBuildableArchitectureAllowed(self, das):
> +        """Check whether we may build for a buildable `DistroArchSeries`.
> +
> +        The caller is assumed to have already checked that a suitable chroot
> +        is available (either directly or via
> +        `DistroSeries.buildable_architectures`).
> +        """
> +        return (
> +            das.enabled
> +            # We only support builds on virtualized builders at the moment.
> +            and das.processor.supports_virtualized)
> +
> +    def _isArchitectureAllowed(self, das, pocket, snap_base=None):
> +        return (
> +            das.getChroot(pocket=pocket) is not None
> +            and self._isBuildableArchitectureAllowed(das))
> +
> +    def requestBuild(self, git_repository, commit_sha1, distro_arch_series):
> +        """See `ICIBuildSet`."""
> +        pocket = PackagePublishingPocket.UPDATES
> +        if not self._isArchitectureAllowed(distro_arch_series, pocket):
> +            raise CIBuildDisallowedArchitecture(distro_arch_series, pocket)
> +
> +        result = IStore(CIBuild).find(
> +            CIBuild,
> +            CIBuild.git_repository == git_repository,
> +            CIBuild.commit_sha1 == commit_sha1,
> +            CIBuild.distro_arch_series == distro_arch_series)
> +        if not result.is_empty():
> +            raise CIBuildAlreadyRequested
> +
> +        build = self.new(git_repository, commit_sha1, distro_arch_series)
> +        build.queueBuild()
> +        notify(ObjectCreatedEvent(build))
> +        return build
> +
> +    def _determineDASesToBuild(self, configuration, logger=None):

This currently private method does not use `self` once. Imho it should moved outside this class.

> +        """Generate distroarchseries to build for this configuration."""
> +        architectures_by_series = {}
> +        for stage in configuration.pipeline:
> +            for job_name in stage:
> +                if job_name not in configuration.jobs:
> +                    if logger is not None:
> +                        logger.error("No job definition for %r", job_name)
> +                    continue
> +                for job in configuration.jobs[job_name]:
> +                    for architecture in job["architectures"]:
> +                        architectures_by_series.setdefault(
> +                            job["series"], set()).add(architecture)
> +        # XXX cjwatson 2022-01-21: We have to hardcode Ubuntu for now, since
> +        # the .launchpad.yaml format doesn't currently support other
> +        # distributions (although nor does the Launchpad build farm).
> +        distribution = getUtility(ILaunchpadCelebrities).ubuntu
> +        for series_name, architecture_names in architectures_by_series.items():
> +            try:
> +                series = distribution[series_name]
> +            except NotFoundError:
> +                if logger is not None:
> +                    logger.error("Unknown Ubuntu series name %s" % series_name)
> +                continue
> +            architectures = {
> +                das.architecturetag: das
> +                for das in series.buildable_architectures}
> +            for architecture_name in architecture_names:
> +                try:
> +                    das = architectures[architecture_name]
> +                except KeyError:
> +                    if logger is not None:
> +                        logger.error(
> +                            "%s is not a buildable architecture name in "
> +                            "Ubuntu %s" % (architecture_name, series_name))
> +                    continue
> +                yield das
> +
> +    def requestBuildsForRefs(self, git_repository, ref_paths, logger=None):
> +        """See `ICIBuildSet`."""
> +        # XXX jugmac00 2022-03-01:
> +        # - extract function
> +        # - name: get_all_commits_for_paths(repository, paths)

What is your take on this?

> +        commit_sha1s = [
> +            ref.commit_sha1
> +            for ref in GitRef.findByReposAndPaths(
> +                [(git_repository, ref_path)
> +                 for ref_path in ref_paths]).values()]
> +        # getCommits performs a web request!
> +        commits = getUtility(IGitHostingClient).getCommits(
> +            git_repository.getInternalPath(), commit_sha1s,
> +            # XXX cjwatson 2022-01-19: We should also fetch
> +            # debian/.launchpad.yaml (or perhaps make the path a property of
> +            # the repository) once lpcraft and launchpad-buildd support
> +            # using alternative paths for builds.
> +            filter_paths=[".launchpad.yaml"])
> +        for commit in commits:
> +            try:
> +                configuration = parse_configuration(
> +                    git_repository, commit["blobs"][".launchpad.yaml"])
> +            except CannotParseConfiguration as e:
> +                if logger is not None:
> +                    logger.error(
> +                        "Cannot parse .launchpad.yaml from %s:%s: %s",
> +                        git_repository.unique_name, commit["sha1"], e)
> +                continue

Maybe we could even replace the above code with this...

```
buildable_commits = [commit, configuration for commit in filter_valid_commits(commits, logger]
```

I am not super happy with a try/except block in the middle of a method. But maybe this goes too far.

> +            # XXX jugmac00 2022-03-01:
> +            # - extract function
> +            # - name: try_to_schedule_builds(commit, configuration, logger)

What's your take on this? Or I think I would even like this better...

```
for das in self._determineDASesToBuild(configuration):
    try_to_schedule_build(commit, das)
```

> +            for das in self._determineDASesToBuild(configuration):
> +                try:
> +                    if logger is not None:
> +                        logger.info(
> +                            "Requesting CI build for %s on %s/%s",
> +                            commit["sha1"], das.distroseries.name,
> +                            das.architecturetag)
> +                    self.requestBuild(git_repository, commit["sha1"], das)
> +                except CIBuildAlreadyRequested:
> +                    pass
> +                except Exception as e:
> +                    if logger is not None:
> +                        logger.error(
> +                            "Failed to request CI build for %s on %s/%s: "
> +                            "%s",
> +                            commit["sha1"], das.distroseries.name,
> +                            das.architecturetag, e)
> +
>      def getByID(self, build_id):
>          """See `ISpecificBuildFarmJobSource`."""
>          store = IMasterStore(CIBuild)
> diff --git a/lib/lp/code/model/tests/test_cibuild.py b/lib/lp/code/model/tests/test_cibuild.py
> index 133bed2..fbd802b 100644
> --- a/lib/lp/code/model/tests/test_cibuild.py
> +++ b/lib/lp/code/model/tests/test_cibuild.py
> @@ -336,6 +354,211 @@ class TestCIBuildSet(TestCaseWithFactory):
>          self.assertContentEqual(
>              builds[2:], ci_build_set.findByGitRepository(repositories[1]))
>  
> +    def makeBuildableDistroArchSeries(self, architecturetag=None,
> +                                      processor=None,
> +                                      supports_virtualized=True,
> +                                      supports_nonvirtualized=True, **kwargs):
> +        if architecturetag is None:
> +            architecturetag = self.factory.getUniqueUnicode("arch")
> +        if processor is None:
> +            try:
> +                processor = getUtility(IProcessorSet).getByName(
> +                    architecturetag)
> +            except ProcessorNotFound:
> +                processor = self.factory.makeProcessor(
> +                    name=architecturetag,
> +                    supports_virtualized=supports_virtualized,
> +                    supports_nonvirtualized=supports_nonvirtualized)
> +        das = self.factory.makeDistroArchSeries(
> +            architecturetag=architecturetag, processor=processor, **kwargs)
> +        # Add both a chroot and a LXD image to test that
> +        # getAllowedArchitectures doesn't get confused by multiple
> +        # PocketChroot rows for a single DistroArchSeries.
> +        fake_chroot = self.factory.makeLibraryFileAlias(
> +            filename="fake_chroot.tar.gz", db_only=True)
> +        das.addOrUpdateChroot(fake_chroot)
> +        fake_lxd = self.factory.makeLibraryFileAlias(
> +            filename="fake_lxd.tar.gz", db_only=True)
> +        das.addOrUpdateChroot(fake_lxd, image_type=BuildBaseImageType.LXD)
> +        return das
> +
> +    def test_requestCIBuild(self):
> +        # requestBuild creates a new CIBuild.
> +        repository = self.factory.makeGitRepository()
> +        commit_sha1 = hashlib.sha1(self.factory.getUniqueBytes()).hexdigest()
> +        das = self.makeBuildableDistroArchSeries()
> +
> +        build = getUtility(ICIBuildSet).requestBuild(
> +            repository, commit_sha1, das)
> +
> +        self.assertTrue(ICIBuild.providedBy(build))
> +        self.assertThat(build, MatchesStructure.byEquality(
> +            git_repository=repository,
> +            commit_sha1=commit_sha1,
> +            distro_arch_series=das,
> +            status=BuildStatus.NEEDSBUILD,
> +            ))
> +        store = Store.of(build)
> +        store.flush()
> +        build_queue = store.find(
> +            BuildQueue,
> +            BuildQueue._build_farm_job_id ==
> +                removeSecurityProxy(build).build_farm_job_id).one()
> +        self.assertProvides(build_queue, IBuildQueue)
> +        self.assertTrue(build_queue.virtualized)
> +        self.assertEqual(BuildQueueStatus.WAITING, build_queue.status)
> +
> +    def test_requestBuild_score(self):
> +        # CI builds have an initial queue score of 2600.
> +        repository = self.factory.makeGitRepository()
> +        commit_sha1 = hashlib.sha1(self.factory.getUniqueBytes()).hexdigest()
> +        das = self.makeBuildableDistroArchSeries()
> +        build = getUtility(ICIBuildSet).requestBuild(
> +            repository, commit_sha1, das)
> +        queue_record = build.buildqueue_record
> +        queue_record.score()
> +        self.assertEqual(2600, queue_record.lastscore)
> +
> +    def test_requestBuild_rejects_repeats(self):
> +        # requestBuild refuses if an identical build was already requested.
> +        repository = self.factory.makeGitRepository()
> +        commit_sha1 = hashlib.sha1(self.factory.getUniqueBytes()).hexdigest()
> +        distro_series = self.factory.makeDistroSeries()
> +        arches = [
> +            self.makeBuildableDistroArchSeries(distroseries=distro_series)
> +            for _ in range(2)]
> +        old_build = getUtility(ICIBuildSet).requestBuild(
> +            repository, commit_sha1, arches[0])
> +        self.assertRaises(
> +            CIBuildAlreadyRequested, getUtility(ICIBuildSet).requestBuild,
> +            repository, commit_sha1, arches[0])
> +        # We can build for a different distroarchseries.
> +        getUtility(ICIBuildSet).requestBuild(
> +            repository, commit_sha1, arches[1])
> +        # Changing the status of the old build does not allow a new build.
> +        old_build.updateStatus(BuildStatus.BUILDING)
> +        old_build.updateStatus(BuildStatus.FULLYBUILT)
> +        self.assertRaises(
> +            CIBuildAlreadyRequested, getUtility(ICIBuildSet).requestBuild,
> +            repository, commit_sha1, arches[0])
> +
> +    def test_requestBuild_virtualization(self):
> +        # New builds are virtualized.
> +        repository = self.factory.makeGitRepository()
> +        commit_sha1 = hashlib.sha1(self.factory.getUniqueBytes()).hexdigest()
> +        distro_series = self.factory.makeDistroSeries()
> +        for proc_nonvirt in True, False:
> +            das = self.makeBuildableDistroArchSeries(
> +                distroseries=distro_series, supports_virtualized=True,
> +                supports_nonvirtualized=proc_nonvirt)
> +            build = getUtility(ICIBuildSet).requestBuild(
> +                repository, commit_sha1, das)
> +            self.assertTrue(build.virtualized)
> +
> +    def test_requestBuild_nonvirtualized(self):
> +        # A non-virtualized processor cannot run a CI build.
> +        repository = self.factory.makeGitRepository()
> +        commit_sha1 = hashlib.sha1(self.factory.getUniqueBytes()).hexdigest()
> +        distro_series = self.factory.makeDistroSeries()
> +        das = self.makeBuildableDistroArchSeries(
> +            distroseries=distro_series, supports_virtualized=False,
> +            supports_nonvirtualized=True)
> +        self.assertRaises(
> +            CIBuildDisallowedArchitecture,
> +            getUtility(ICIBuildSet).requestBuild, repository, commit_sha1, das)
> +
> +    def test__determineDASesToBuild(self):

I usually do not like to test private methods - as I mentioned above, the method never references `self`, so imho this should be no method, but rather extracted from the class.

> +        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
> +        distro_serieses = [
> +            self.factory.makeDistroSeries(ubuntu) for _ in range(2)]
> +        dases = []
> +        for distro_series in distro_serieses:
> +            for _ in range(2):
> +                dases.append(self.makeBuildableDistroArchSeries(
> +                    distroseries=distro_series))
> +        configuration = load_configuration(dedent("""\
> +            pipeline:
> +                - [build]
> +                - [test]
> +            jobs:
> +                build:
> +                    series: {distro_serieses[1].name}
> +                    architectures:
> +                        - {dases[2].architecturetag}
> +                        - {dases[3].architecturetag}
> +                test:
> +                    series: {distro_serieses[1].name}
> +                    architectures:
> +                        - {dases[2].architecturetag}
> +            """.format(distro_serieses=distro_serieses, dases=dases)))
> +        ci_build_set = removeSecurityProxy(getUtility(ICIBuildSet))
> +        logger = BufferLogger()
> +
> +        dases_to_build = list(
> +            ci_build_set._determineDASesToBuild(configuration, logger=logger))
> +
> +        self.assertContentEqual(dases[2:], dases_to_build)
> +        self.assertEqual("", logger.getLogBuffer())
> +        # XXX error cases for _determineDASesToBuild
> +
> +
> +    def test_requestBuildsForRefs(self):

I still do not like this test - it is super long, and the commits from `makeGitRefs` and `encoded_commit_json` are disconnected.

> +        # set up distroseries and buildable architectures
> +        ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
> +        series = self.factory.makeDistroSeries(
> +            distribution=ubuntu,
> +            name="focal",
> +        )
> +        self.makeBuildableDistroArchSeries(
> +            distroseries=series,
> +            architecturetag="amd64"
> +        )
> +
> +        # set up git repository + git commit
> +        configuration = dedent("""\
> +            pipeline:
> +            - test
> +
> +            jobs:
> +                test:
> +                    series: focal
> +                    architectures: amd64
> +                    run: echo hello world >output
> +            """).encode()
> +        repository = self.factory.makeGitRepository()
> +        ref_paths = ['refs/heads/master']
> +        self.factory.makeGitRefs(repository, ref_paths)
> +        encoded_commit_json = {
> +            "sha1": "0",
> +            "blobs": {
> +                ".launchpad.yaml": configuration
> +                },
> +            }
> +        hosting_fixture = self.useFixture(
> +            GitHostingFixture(commits=[encoded_commit_json])
> +        )
> +
> +        getUtility(ICIBuildSet).requestBuildsForRefs(
> +            repository, ref_paths,
> +        )
> +
> +        self.assertEqual(
> +            [
> +                (("1", ["972c6d2dc6dd5efdad1377c0d224e03eb8f276f7"]),

I don't like this magic number, but I think there is no easy way out when using `makeGitRefs`.

> +                 {"filter_paths": [".launchpad.yaml"]})
> +            ],
> +            hosting_fixture.getCommits.calls
> +        )
> +
> +        store = IStore(CIBuild)
> +        store.flush()
> +        build = store.find(CIBuild).one()
> +
> +        # check that a build was created
> +        self.assertEqual("0", build.commit_sha1)
> +        self.assertEqual("focal", build.distro_arch_series.distroseries.name)
> +        self.assertEqual("amd64", build.distro_arch_series.architecturetag)
> +
>      def test_deleteByGitRepository(self):
>          repositories = [self.factory.makeGitRepository() for _ in range(2)]
>          builds = []


-- 
https://code.launchpad.net/~jugmac00/launchpad/+git/launchpad/+merge/416223
Your team Launchpad code reviewers is requested to review the proposed merge of ~jugmac00/launchpad:create-lpcraft-jobs-on-push into launchpad:master.