launchpad-reviewers team mailing list archive

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

Thank you for the context - I think this should not be lost here as a comment, but needs to go into the code as a comment and in the change log.

A couple of things on top of the inline comments.

> Test here is testing a build with a component and checks if the component is added correctly to the waiting files.

Could you add this to the test as docstring? And maybe even expand what "waiting files" are? I am not familiar with that terminology.

Could you please replace the link in the commit message with a reference to the spec as you did in the comment? Links might change, but SD149 is here to stay.



Diff comments:

> diff --git a/debian/changelog b/debian/changelog
> index 4849b34..e8bf844 100644
> --- a/debian/changelog
> +++ b/debian/changelog
> @@ -9,6 +10,9 @@ launchpad-buildd (236) UNRELEASED; urgency=medium
>      separately.
>    * Document deployment to qastaging in place of dogfood.
>  
> +  [Simone Pelosi]
> +  * Add support for snap components. 

Could you expand the change log in your own words like you did in the comment above. That is valuable information.

> So I noticed that when we are gathering the results from the build and we are adding them to the file cache we are filtering the results, for this reason I added the new suffix.

I would also like to read something what the consequence of that is... that he build artifact gets rendered in the Launchpad UI, is it? And possible also via API?

> +
>   -- Colin Watson <cjwatson@xxxxxxxxxx>  Thu, 16 Nov 2023 16:06:07 +0000
>  
>  launchpad-buildd (235) focal; urgency=medium
> diff --git a/lpbuildd/snap.py b/lpbuildd/snap.py
> index d922242..496c7c3 100644
> --- a/lpbuildd/snap.py
> +++ b/lpbuildd/snap.py
> @@ -138,8 +138,11 @@ class SnapBuildManager(BuildManagerProxyMixin, DebianBuildManager):
>                  path = os.path.join(output_path, entry)
>                  if self.backend.islink(path):
>                      continue
> +                # `.comp` files must be added to file cache.
> +                # since we are supporting Snap Components
> +                # reference: spec SD149

What about...?

`.comp` files are the binary result of building snap components, see spec SD149.

The file cache thing is already mentioned in the function docstring.

>                  if entry.endswith(
> -                    (".snap", ".manifest", ".debug", ".dpkg.yaml")
> +                    (".snap", ".manifest", ".debug", ".dpkg.yaml", ".comp")
>                  ):
>                      self.addWaitingFileFromBackend(path)
>          if self.build_source_tarball:
> diff --git a/lpbuildd/tests/test_snap.py b/lpbuildd/tests/test_snap.py
> index 0e44093..59cc595 100644
> --- a/lpbuildd/tests/test_snap.py
> +++ b/lpbuildd/tests/test_snap.py
> @@ -255,6 +255,78 @@ class TestSnapBuildManagerIteration(TestCase):
>          self.assertFalse(self.builder.wasCalled("buildFail"))
>  
>      @defer.inlineCallbacks
> +    def test_iterate_with_components(self):

Please add a comment here what you described above what this test is actually doing, which is currently neither obvious from the test name, nor the description.

> +        # The build manager iterates a build that uploads components from
> +        # start to finish.
> +        args = {
> +            "git_repository": "https://git.launchpad.dev/~example/+git/snap";,
> +            "git_path": "master",
> +        }
> +        expected_options = [
> +            "--git-repository",
> +            "https://git.launchpad.dev/~example/+git/snap";,
> +            "--git-path",
> +            "master",
> +        ]
> +        yield self.startBuild(args, expected_options)
> +
> +        log_path = os.path.join(self.buildmanager._cachepath, "buildlog")
> +        with open(log_path, "w") as log:
> +            log.write("I am a build log.")
> +
> +        self.buildmanager.backend.add_file(
> +            "/build/test-snap/test-snap_0_all.snap", b"I am a snap package."
> +        )
> +        self.buildmanager.backend.add_file(
> +            "/build/test-snap/test-snap+somecomponent_0.comp", b"I am a component."
> +        )
> +
> +        # After building the package, reap processes.
> +        yield self.buildmanager.iterate(0)
> +        expected_command = [
> +            "sharepath/bin/in-target",
> +            "in-target",
> +            "scan-for-processes",
> +            "--backend=lxd",
> +            "--series=xenial",
> +            "--arch=i386",
> +            self.buildid,
> +        ]
> +        self.assertEqual(SnapBuildState.BUILD_SNAP, self.getState())
> +        self.assertEqual(expected_command, self.buildmanager.commands[-1])
> +        self.assertNotEqual(
> +            self.buildmanager.iterate, self.buildmanager.iterators[-1]
> +        )
> +        self.assertFalse(self.builder.wasCalled("buildFail"))
> +        self.assertThat(
> +            self.builder,
> +            HasWaitingFiles.byEquality(
> +                {
> +                    "test-snap+somecomponent_0.comp": b"I am a component.",
> +                    "test-snap_0_all.snap": b"I am a snap package.",
> +                }
> +            ),
> +        )
> +
> +        # Control returns to the DebianBuildManager in the UMOUNT state.
> +        self.buildmanager.iterateReap(self.getState(), 0)
> +        expected_command = [
> +            "sharepath/bin/in-target",
> +            "in-target",
> +            "umount-chroot",
> +            "--backend=lxd",
> +            "--series=xenial",
> +            "--arch=i386",
> +            self.buildid,
> +        ]
> +        self.assertEqual(SnapBuildState.UMOUNT, self.getState())
> +        self.assertEqual(expected_command, self.buildmanager.commands[-1])
> +        self.assertEqual(
> +            self.buildmanager.iterate, self.buildmanager.iterators[-1]
> +        )
> +        self.assertFalse(self.builder.wasCalled("buildFail"))
> +
> +    @defer.inlineCallbacks
>      def test_iterate_with_debug(self):
>          # The build manager iterates a build that uploads debug symbols from
>          # start to finish.


-- 
https://code.launchpad.net/~pelpsi/launchpad-buildd/+git/launchpad-buildd/+merge/459807
Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad-buildd:snap-components-support into launchpad-buildd:master.