launchpad-reviewers team mailing list archive

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

Thanks for the feedback! I have applied most suggestions, and now I am ready to implement the missing test cases.

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

Moved it to a module level function.

When it comes to `@staticmethod` I go with Guido v.R.:

> Honestly, staticmethod was something of a mistake -- I was trying to
do something like Java class methods but once it was released I found
what was really needed was classmethod. But it was too late to get rid
of staticmethod.

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

Sounds good, had similar ideas.

> +        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 tried some refactorings, but it did not become clearer or more readable. Let's keep it for now.

> +            # XXX jugmac00 2022-03-01:
> +            # - extract function
> +            # - name: try_to_schedule_builds(commit, configuration, logger)
> +            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):

When a private method is pretty complex, often it should be extract as a class - with a then public method to use in the previous context. I guess it depends :-)

> +        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 have seen this pattern in the test for `determine_DASes_to_build`, and I would prefer not to follow it.

I had quite a hard time to get a grip on the test - and that is not ideal, especially as I see tests as a documentation for the requirements and I also want to quickly figure out what is wrong when the test fails.

e.g. when a test fails in the following line, I possibly need to read a lot of preceeding code and need to use a debugger to figure out that is going wrong.

```
self.assertContentEqual(dases[2:], dases_to_build)
```

On the other hand, for the following line it is pretty clear:

```
self.assertEqual("focal", build.distro_arch_series.distroseries.name)
```

Apart from readability, I do not like to compare two computed values - they both could be wrong in a way that the assertion succeeds anyway.

The `black` style formatting is something we have opted into, so this will stay. I have noticed I have overdone it somewhat, so I'll reformat the method to make it more terse and I think more readable.

> And I think the "set up git repository + git commit" comment is probably not very necessary.

I usually create intermediate comments within functions/methods, which I later use as a method/function/fixture name when I extract that code part. Also, this is a helper for me while learning a new domain. I'll remove the comments before finishing the MP.

Thanks for your input!

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

Thanks!

Actually, I had a look at the method, but (at 10 pm) I could not figure out what it returns - I probably missed the docstring, and I could have set a breakpoint.

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

Thanks for the explanation.

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

Good one! Thanks.

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