launchpad-reviewers team mailing list archive

[Colin Watson <mp+416223@xxxxxxxxxxxxxxxxxx>]

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

We could also use a `@staticmethod`, but a file-level function would probably make most sense.

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

I'd at least think about making it a method on `GitRepository`, but if it feels too special-purpose for that then a file-level function would be fine too.

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

I don't have a problem with try/except blocks in the middle of methods any more than in the middle of any other functions; they're often useful for one reason or another.

Factoring out a helper method/function is fine if it helps, though your proposed `filter_valid_commits` function would need to parse the configuration out of the commit dict and return it as well.  Maybe `parse_configuration_blobs`, taking a mapping of commit IDs to blobs, returning a mapping of the parseable ones, and logging any that aren't parseable?

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

This would have to be a method (since it needs to call `self.requestBuild`), so `self._tryToRequestBuild` or something, but otherwise that seems like a reasonable place to split this up a bit.

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

As above, making it a function would be fine.  That said, I think there can be a place for testing private methods, especially when the point of decomposing a method into multiple pieces is to improve its testability: we shouldn't feel under pressure to make a method public (and hence kosher to use from other parts of the application) when we really only want to call it internally and from tests.

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

To some extent I think it looks artificially long due to specifying too many details in factory calls and then having very vertically-sparse argument formatting in the way that `black` tends to do.  It could be more compact and thus easier to take in all at once if you let the factory do a bit more of the work, for instance:

    series = self.factory.makeDistroSeries(distribution=ubuntu)
    das = self.makeBuildableDistroArchSeries(distroseries=series)

Then substitute the series name and the architecture tag into the configuration, drop `ref_paths`, replace the ref creation with `[ref] = self.factory.makeGitRefs(repository=repository)`, and then use `[ref.path]` rather than `ref_paths`.  And I think the "set up git repository + git commit" comment is probably not very necessary.  Maybe you still want to break it up more, but I think all that would help.

> +        # 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",

Using `ref.commit_sha1` here might be a little less artificial.

> +            "blobs": {
> +                ".launchpad.yaml": configuration
> +                },
> +            }
> +        hosting_fixture = self.useFixture(
> +            GitHostingFixture(commits=[encoded_commit_json])
> +        )
> +
> +        getUtility(ICIBuildSet).requestBuildsForRefs(
> +            repository, ref_paths,
> +        )
> +
> +        self.assertEqual(
> +            [
> +                (("1", ["972c6d2dc6dd5efdad1377c0d224e03eb8f276f7"]),

It's only difficult because you've discarded the return value of `makeGitRefs`.  If you do `[ref] = self.factory.makeGitRefs(repository=repository)` as I suggested above, then you could use:

    (repository.getInternalPath(), [ref.commit_sha1])

> +                 {"filter_paths": [".launchpad.yaml"]})
> +            ],
> +            hosting_fixture.getCommits.calls
> +        )
> +
> +        store = IStore(CIBuild)
> +        store.flush()

This flush seems likely to be unnecessary.  It's normally only needed when we specifically need the previous queued query to be flushed to the database before we can issue the next one (a common reason is when we need to get hold of a row's ID, which is typically a sequence number assigned by the database); and in any case the process of requesting builds will already have flushed the store via `CIBuildSet.new` just before this.

> +        build = store.find(CIBuild).one()

While I hope nobody will ever have cause to add `CIBuild` rows to sampledata in future, it nevertheless isn't good practice to assume in tests that no other rows are present in a given table.  This should instead be something like:

    build = getUtility(ICIBuildSet).findByGitRepository(repository).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.