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 <[email protected]> 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. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
