launchpad-reviewers team mailing list archive

[Colin Watson <mp+427724@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/lpcraft:rearrange-output-paths into lpcraft:main.

Commit message:
Rearrange output directory structure

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/lpcraft/+git/lpcraft/+merge/427724

The previous structure of `<job_name>/<series>/<architecture>` didn't work well with matrix jobs, where there can be multiple jobs with the same series and architecture but differing by some other property.  It also made it difficult to implement the `input` keyword, which needs to be able to locate artifacts produced by previously-executed jobs in the output directory; this would currently be impossible in Launchpad builds because each individual job is run with a different `--output-directory` in order to allow launchpad-buildd to extract just the artifacts produced by that job.

Instead, mirror the structure implied by the `run-one` command, and rearrange the output directory structure to `<job_name>/<job_index>`, where `job_index` is the index of the individual job being run in the matrix-expanded list of jobs with the same name.  This is a bit simpler, and makes it possible to iterate over output from all jobs with the same name just by iterating over the `<job_name>` directory.

I've checked that launchpad-buildd won't object to this rearranged structure, although once it's in place we'll be able to change launchpad-buildd to run all individual jobs with the same `--output-directory` (because it will then have another way to identify artifacts produced by individual jobs), thus preparing the way for implementing the `input` keyword.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/lpcraft:rearrange-output-paths into lpcraft:main.

diff --git a/NEWS.rst b/NEWS.rst
index 64f09d0..0a57156 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -5,7 +5,8 @@ Version history
 0.0.23 (unreleased)
 ===================
 
-- Nothing yet.
+- Rearrange output directory structure to improve support for matrix jobs
+  and to prepare for passing input artifacts to jobs.
 
 0.0.22 (2022-08-01)
 ===================
diff --git a/lpcraft/commands/run.py b/lpcraft/commands/run.py
index c7128ae..131811f 100644
--- a/lpcraft/commands/run.py
+++ b/lpcraft/commands/run.py
@@ -339,8 +339,9 @@ def _run_instance_command(
 
 
 def _run_job(
+    config: Config,
     job_name: str,
-    job: Job,
+    job_index: int,
     provider: Provider,
     output: Optional[Path],
     apt_replacement_repositories: Optional[List[str]] = None,
@@ -352,6 +353,7 @@ def _run_job(
     """Run a single job."""
     # XXX jugmac00 2022-04-27: we should create a configuration object to be
     # passed in and not so many arguments
+    job = config.jobs[job_name][job_index]
     host_architecture = get_host_architecture()
     if host_architecture not in job.architectures:
         return
@@ -453,7 +455,7 @@ def _run_job(
                 )
 
         if job.output is not None and output is not None:
-            target_path = output / job_name / job.series / host_architecture
+            target_path = output / job_name / str(job_index)
             target_path.mkdir(parents=True, exist_ok=True)
             _copy_output_paths(job.output, remote_cwd, instance, target_path)
             _copy_output_properties(
@@ -552,13 +554,14 @@ class RunCommand(BaseCommand):
                             raise CommandError(
                                 f"No job definition for {job_name!r}"
                             )
-                        for job in jobs:
+                        for job_index, job in enumerate(jobs):
                             launched_instances.append(
                                 _get_job_instance_name(provider, job)
                             )
                             _run_job(
+                                config,
                                 job_name,
-                                job,
+                                job_index,
                                 provider,
                                 args.output_directory,
                                 apt_replacement_repositories=(
@@ -681,8 +684,9 @@ class RunOneCommand(BaseCommand):
             secrets = yaml.safe_load(content)
         try:
             _run_job(
+                config,
                 args.job,
-                job,
+                args.index,
                 provider,
                 args.output_directory,
                 apt_replacement_repositories=args.apt_replace_repositories,
diff --git a/lpcraft/commands/tests/test_run.py b/lpcraft/commands/tests/test_run.py
index 5ff89f9..3dc6bbc 100644
--- a/lpcraft/commands/tests/test_run.py
+++ b/lpcraft/commands/tests/test_run.py
@@ -912,7 +912,7 @@ class TestRun(RunBaseTestCase):
         )
 
         self.assertEqual(0, result.exit_code)
-        job_output = target_path / "build" / "focal" / "amd64"
+        job_output = target_path / "build" / "0"
         self.assertEqual(
             [
                 call(
@@ -976,7 +976,7 @@ class TestRun(RunBaseTestCase):
         )
 
         self.assertEqual(0, result.exit_code)
-        job_output = target_path / "build" / "focal" / "amd64"
+        job_output = target_path / "build" / "0"
         self.assertEqual(
             [
                 call(
@@ -1218,7 +1218,7 @@ class TestRun(RunBaseTestCase):
         )
 
         self.assertEqual(0, result.exit_code)
-        job_output = target_path / "build" / "focal" / "amd64"
+        job_output = target_path / "build" / "0"
         self.assertEqual(
             {"foo": "bar"},
             json.loads((job_output / "properties").read_text()),
@@ -1263,7 +1263,7 @@ class TestRun(RunBaseTestCase):
         )
 
         self.assertEqual(0, result.exit_code)
-        job_output = target_path / "test" / "focal" / "amd64"
+        job_output = target_path / "test" / "0"
         self.assertEqual(
             {"version": "0.1"},
             json.loads((job_output / "properties").read_text()),
@@ -1313,7 +1313,7 @@ class TestRun(RunBaseTestCase):
         )
 
         self.assertEqual(0, result.exit_code)
-        job_output = target_path / "test" / "focal" / "amd64"
+        job_output = target_path / "test" / "0"
         self.assertEqual(
             {"version": "0.2"},
             json.loads((job_output / "properties").read_text()),
@@ -2444,7 +2444,7 @@ class TestRunOne(RunBaseTestCase):
         )
 
         self.assertEqual(0, result.exit_code)
-        job_output = target_path / "build" / "focal" / "amd64"
+        job_output = target_path / "build" / "0"
         self.assertEqual(
             [
                 call(