launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28932
[Merge] ~cjwatson/launchpad-buildd:lpcraft-common-output-directory into launchpad-buildd:master
Colin Watson has proposed merging ~cjwatson/launchpad-buildd:lpcraft-common-output-directory into launchpad-buildd:master.
Commit message:
Use a common output directory for all lpcraft jobs
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/+git/launchpad-buildd/+merge/427734
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad-buildd:lpcraft-common-output-directory into launchpad-buildd:master.
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 <cjwatson@xxxxxxxxxx> 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:pass@xxxxxxxxxxxxxxxxxxxxx/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()
