Merge ~cjwatson/launchpad-buildd:lpcraft-common-output-directory into launchpad-buildd:master

[Colin Watson]

Use a common output directory for all lpcraft jobs

In order to implement the `input` keyword in lpcraft, launchpad-buildd needs to use the same top-level output directory for each job so that lpcraft can find artifacts from previously-executed jobs. This is problematic with the current arrangements, because launchpad-buildd has to pass different `--output-directory` options for each individual job so that it can accurately determine which output artifacts belong to which job.

However, with https://code.launchpad.net/~cjwatson/lpcraft/+git/lpcraft/+merge/427724, it becomes possible for launchpad-buildd to identify artifacts by job name and index within a single common output directory, so we can take advantage of that by looking for output files more precisely.

This also gathers the `properties` file (currently unused) in a slightly different way, preparing for Launchpad to extract those output properties and store them in the database in a way that can conveniently be used downstream.

This mustn't be merged until https://code.launchpad.net/~cjwatson/lpcraft/+git/lpcraft/+merge/427724 has been released to the `stable` channel.

[Jürgen Gmach (jugmac00)]

review: Approve

diff --git a/debian/changelog b/debian/changelog
index 810b64d..679201c 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+launchpad-buildd (218) UNRELEASED; urgency=medium
+
+  * Use a common output directory for all lpcraft jobs.
+
+ -- Colin Watson <[e-mail]>  Tue, 02 Aug 2022 15:55:19 +0100
+
 launchpad-buildd (217) focal; urgency=medium
 
   [ Colin Watson ]
diff --git a/lpbuildd/ci.py b/lpbuildd/ci.py
index ac6c585..fcfd8d4 100644
--- a/lpbuildd/ci.py
+++ b/lpbuildd/ci.py
@@ -241,16 +241,20 @@ class CIBuildManager(BuildManagerProxyMixin, DebianBuildManager):
         This is called once for each CI job in the pipeline.
         """
         job_status = {}
-        output_path = os.path.join("/build", "output", self.current_job_id)
-        log_path = "%s.log" % output_path
-        if self.backend.path_exists(log_path):
-            log_name = "%s.log" % self.current_job_id
-            self.addWaitingFileFromBackend(log_path, log_name)
-            job_status["log"] = self._builder.waitingfiles[log_name]
-        if self.backend.path_exists(output_path):
+        job_name, job_index = self.current_job
+        job_output_path = os.path.join(
+            "/build", "output", job_name, str(job_index))
+        for item_name in ("log", "properties"):
+            item_path = os.path.join(job_output_path, item_name)
+            if self.backend.path_exists(item_path):
+                item_id = f"{self.current_job_id}.{item_name}"
+                self.addWaitingFileFromBackend(item_path, name=item_id)
+                job_status[item_name] = self._builder.waitingfiles[item_id]
+        files_path = os.path.join(job_output_path, "files")
+        if self.backend.path_exists(files_path):
             for entry in sorted(self.backend.find(
-                    output_path, include_directories=False)):
-                path = os.path.join(output_path, entry)
+                    files_path, include_directories=False)):
+                path = os.path.join(files_path, entry)
                 if self.backend.islink(path):
                     continue
                 entry_base = os.path.basename(entry)
diff --git a/lpbuildd/target/run_ci.py b/lpbuildd/target/run_ci.py
index 75deaf9..7a8fa3b 100644
--- a/lpbuildd/target/run_ci.py
+++ b/lpbuildd/target/run_ci.py
@@ -127,8 +127,11 @@ class RunCI(BuilderProxyOperationMixin, Operation):
         env = self.build_proxy_environment(proxy_url=self.args.proxy_url)
         job_id = f"{self.args.job_name}:{self.args.job_index}"
         logger.info("Running %s" % job_id)
-        output_path = os.path.join("/build", "output", job_id)
-        self.backend.run(["mkdir", "-p", output_path])
+        output_path = os.path.join("/build", "output")
+        # This matches the per-job output path used by lpcraft.
+        job_output_path = os.path.join(
+            output_path, self.args.job_name, str(self.args.job_index))
+        self.backend.run(["mkdir", "-p", job_output_path])
         lpcraft_args = [
             "lpcraft",
             "-v",
@@ -161,7 +164,7 @@ class RunCI(BuilderProxyOperationMixin, Operation):
 
         escaped_lpcraft_args = (
             " ".join(shell_escape(arg) for arg in lpcraft_args))
-        tee_args = ["tee", "%s.log" % output_path]
+        tee_args = ["tee", os.path.join(job_output_path, "log")]
         escaped_tee_args = " ".join(shell_escape(arg) for arg in tee_args)
         args = [
             "/bin/bash", "-o", "pipefail", "-c",
diff --git a/lpbuildd/target/tests/test_run_ci.py b/lpbuildd/target/tests/test_run_ci.py
index 2f18bd5..ed6e0a2 100644
--- a/lpbuildd/target/tests/test_run_ci.py
+++ b/lpbuildd/target/tests/test_run_ci.py
@@ -316,11 +316,11 @@ class TestRunCI(TestCase):
         run_ci = parse_args(args=args).operation
         run_ci.run_job()
         self.assertThat(run_ci.backend.run.calls, MatchesListwise([
-            RanCommand(["mkdir", "-p", "/build/output/test:0"]),
+            RanCommand(["mkdir", "-p", "/build/output/test/0"]),
             RanBuildCommand([
                 "/bin/bash", "-o", "pipefail", "-c",
-                "lpcraft -v run-one --output-directory /build/output/test:0 test 0 2>&1 "  # noqa: E501
-                "| tee /build/output/test:0.log",
+                "lpcraft -v run-one --output-directory /build/output test 0 2>&1 "  # noqa: E501
+                "| tee /build/output/test/0/log",
                 ], cwd="/build/tree"),
             ]))
 
@@ -340,11 +340,11 @@ class TestRunCI(TestCase):
             "SNAPPY_STORE_NO_CDN": "1",
             }
         self.assertThat(run_ci.backend.run.calls, MatchesListwise([
-            RanCommand(["mkdir", "-p", "/build/output/test:0"]),
+            RanCommand(["mkdir", "-p", "/build/output/test/0"]),
             RanBuildCommand([
                 "/bin/bash", "-o", "pipefail", "-c",
-                "lpcraft -v run-one --output-directory /build/output/test:0 test 0 2>&1 "  # noqa: E501
-                "| tee /build/output/test:0.log",
+                "lpcraft -v run-one --output-directory /build/output test 0 2>&1 "  # noqa: E501
+                "| tee /build/output/test/0/log",
                 ], cwd="/build/tree", **env),
             ]))
 
@@ -359,15 +359,15 @@ class TestRunCI(TestCase):
         run_ci = parse_args(args=args).operation
         run_ci.run_job()
         self.assertThat(run_ci.backend.run.calls, MatchesListwise([
-            RanCommand(["mkdir", "-p", "/build/output/test:0"]),
+            RanCommand(["mkdir", "-p", "/build/output/test/0"]),
             RanBuildCommand([
                 "/bin/bash", "-o", "pipefail", "-c",
-                "lpcraft -v run-one --output-directory /build/output/test:0 "
+                "lpcraft -v run-one --output-directory /build/output "
                 "test 0 "
                 "--set-env PIP_INDEX_URL=http://example "
                 "--set-env SOME_PATH=/etc/some_path "
                 "2>&1 "
-                "| tee /build/output/test:0.log",
+                "| tee /build/output/test/0/log",
                 ], cwd="/build/tree"),
             ]))
 
@@ -384,15 +384,15 @@ class TestRunCI(TestCase):
         run_ci = parse_args(args=args).operation
         run_ci.run_job()
         self.assertThat(run_ci.backend.run.calls, MatchesListwise([
-            RanCommand(["mkdir", "-p", "/build/output/test:0"]),
+            RanCommand(["mkdir", "-p", "/build/output/test/0"]),
             RanBuildCommand([
                 "/bin/bash", "-o", "pipefail", "-c",
-                "lpcraft -v run-one --output-directory /build/output/test:0 "
+                "lpcraft -v run-one --output-directory /build/output "
                 "test 0 "
                 "--apt-replace-repositories 'deb http://archive.ubuntu.com/ubuntu/ focal main restricted' "  # noqa: E501
                 "--apt-replace-repositories 'deb http://archive.ubuntu.com/ubuntu/ focal universe' "  # noqa: E501
                 "2>&1 "
-                "| tee /build/output/test:0.log",
+                "| tee /build/output/test/0/log",
                 ], cwd="/build/tree"),
             ]))
 
@@ -407,15 +407,15 @@ class TestRunCI(TestCase):
         run_ci = parse_args(args=args).operation
         run_ci.run_job()
         self.assertThat(run_ci.backend.run.calls, MatchesListwise([
-            RanCommand(["mkdir", "-p", "/build/output/test:0"]),
+            RanCommand(["mkdir", "-p", "/build/output/test/0"]),
             RanBuildCommand([
                 "/bin/bash", "-o", "pipefail", "-c",
-                "lpcraft -v run-one --output-directory /build/output/test:0 "
+                "lpcraft -v run-one --output-directory /build/output "
                 "test 0 "
                 "--plugin-setting "
                 "miniconda_conda_channel=https://user:[e-mail]/artifactory/soss-conda-stable-local/ "  # noqa: E501
                 "2>&1 "
-                "| tee /build/output/test:0.log",
+                "| tee /build/output/test/0/log",
                 ], cwd="/build/tree"),
             ]))
 
@@ -429,14 +429,14 @@ class TestRunCI(TestCase):
         run_ci = parse_args(args=args).operation
         run_ci.run_job()
         self.assertThat(run_ci.backend.run.calls, MatchesListwise([
-            RanCommand(["mkdir", "-p", "/build/output/test:0"]),
+            RanCommand(["mkdir", "-p", "/build/output/test/0"]),
             RanBuildCommand([
                 "/bin/bash", "-o", "pipefail", "-c",
-                "lpcraft -v run-one --output-directory /build/output/test:0 "
+                "lpcraft -v run-one --output-directory /build/output "
                 "test 0 "
                 "--secrets /build/.launchpad-secrets.yaml "
                 "2>&1 "
-                "| tee /build/output/test:0.log",
+                "| tee /build/output/test/0/log",
                 ], cwd="/build/tree"),
             ]))
 
@@ -451,7 +451,7 @@ class TestRunCI(TestCase):
         # Just check that it did something in each step, not every detail.
         self.assertThat(
             run_ci.backend.run.calls,
-            AnyMatch(RanCommand(["mkdir", "-p", "/build/output/test:0"])))
+            AnyMatch(RanCommand(["mkdir", "-p", "/build/output/test/0"])))
 
     def test_run_install_fails(self):
         class FailInstall(FakeMethod):
diff --git a/lpbuildd/tests/test_ci.py b/lpbuildd/tests/test_ci.py
index f67300d..8fa6ee7 100644
--- a/lpbuildd/tests/test_ci.py
+++ b/lpbuildd/tests/test_ci.py
@@ -148,17 +148,19 @@ class TestCIBuildManagerIteration(TestCase):
             ]
         yield self.expectRunJob("build", "0", options=expected_job_options)
         self.buildmanager.backend.add_file(
-            "/build/output/build:0.log", b"I am a CI build job log.")
+            "/build/output/build/0/log", b"I am a CI build job log.")
         self.buildmanager.backend.add_file(
-            "/build/output/build:0/ci.whl",
+            "/build/output/build/0/files/ci.whl",
             b"I am output from a CI build job.")
+        self.buildmanager.backend.add_file(
+            "/build/output/build/0/properties", b'{"key": "value"}')
 
         # Collect the output of the first job and start running the second.
         yield self.expectRunJob("test", "0", options=expected_job_options)
         self.buildmanager.backend.add_file(
-            "/build/output/test:0.log", b"I am a CI test job log.")
+            "/build/output/test/0/log", b"I am a CI test job log.")
         self.buildmanager.backend.add_file(
-            "/build/output/test:0/ci.tar.gz",
+            "/build/output/test/0/files/ci.tar.gz",
             b"I am output from a CI test job.")
 
         # Output from the first job is visible in the status response.
@@ -167,6 +169,8 @@ class TestCIBuildManagerIteration(TestCase):
             {
                 "build:0": {
                     "log": self.builder.waitingfiles["build:0.log"],
+                    "properties": (
+                        self.builder.waitingfiles["build:0.properties"]),
                     "output": {
                         "ci.whl": self.builder.waitingfiles["build:0/ci.whl"],
                         },
@@ -188,6 +192,7 @@ class TestCIBuildManagerIteration(TestCase):
         self.assertFalse(self.builder.wasCalled("buildFail"))
         self.assertThat(self.builder, HasWaitingFiles.byEquality({
             "build:0.log": b"I am a CI build job log.",
+            "build:0.properties": b'{"key": "value"}',
             "build:0/ci.whl": b"I am output from a CI build job.",
             "test:0.log": b"I am a CI test job log.",
             "test:0/ci.tar.gz": b"I am output from a CI test job.",
@@ -199,6 +204,8 @@ class TestCIBuildManagerIteration(TestCase):
             {
                 "build:0": {
                     "log": self.builder.waitingfiles["build:0.log"],
+                    "properties": (
+                        self.builder.waitingfiles["build:0.properties"]),
                     "output": {
                         "ci.whl": self.builder.waitingfiles["build:0/ci.whl"],
                         },
@@ -283,7 +290,7 @@ class TestCIBuildManagerIteration(TestCase):
             ]
         yield self.expectRunJob("lint", "0", options=expected_job_options)
         self.buildmanager.backend.add_file(
-            "/build/output/lint:0.log", b"I am a failing CI lint job log.")
+            "/build/output/lint/0/log", b"I am a failing CI lint job log.")
 
         # Collect the output of the first job and start running the second.
         # (Note that `retcode` is the return code of the *first* job, not the
@@ -292,7 +299,7 @@ class TestCIBuildManagerIteration(TestCase):
             "build", "0", options=expected_job_options,
             retcode=RETCODE_FAILURE_BUILD)
         self.buildmanager.backend.add_file(
-            "/build/output/build:0.log", b"I am a CI build job log.")
+            "/build/output/build/0/log", b"I am a CI build job log.")
 
         # Output from the first job is visible in the status response.
         extra_status = self.buildmanager.status()