launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #27837
[Merge] ~cjwatson/launchpad-buildd:refactor-status into launchpad-buildd:master
Colin Watson has proposed merging ~cjwatson/launchpad-buildd:refactor-status into launchpad-buildd:master.
Commit message:
Refactor extra status handling to be common to all build types
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/+git/launchpad-buildd/+merge/413131
Checking for the JSON status file on each status call is fairly cheap, and it's convenient to have this in common code rather than having to implement it separately for each build type that needs it. I also made the operation-side code a little more general so that it's easy to add arbitrary key-value pairs to the current status, which will be useful shortly.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad-buildd:refactor-status into launchpad-buildd:master.
diff --git a/debian/changelog b/debian/changelog
index a10074a..7a5dcb1 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,6 +1,7 @@
launchpad-buildd (206) UNRELEASED; urgency=medium
* Fix flake8 violations.
+ * Refactor extra status handling to be common to all build types.
-- Colin Watson <cjwatson@xxxxxxxxxx> Wed, 08 Dec 2021 15:42:26 +0000
diff --git a/lpbuildd/builder.py b/lpbuildd/builder.py
index ad08639..194f749 100644
--- a/lpbuildd/builder.py
+++ b/lpbuildd/builder.py
@@ -6,13 +6,17 @@
# The basic builder implementation.
+from __future__ import print_function
+
__metaclass__ = type
from functools import partial
import hashlib
+import json
import os
import re
import shutil
+import sys
import tempfile
import apt
@@ -110,6 +114,17 @@ class RunCapture(protocol.ProcessProtocol):
self.notify(statusobject.value.exitCode)
+def get_build_path(home, build_id, *extra):
+ """Generate a path within the build directory.
+
+ :param home: the user's home directory.
+ :param build_id: the build id to use.
+ :param extra: the extra path segments within the build directory.
+ :return: the generated path.
+ """
+ return os.path.join(home, "build-" + build_id, *extra)
+
+
class BuildManager(object):
"""Build manager abstract parent."""
@@ -231,10 +246,10 @@ class BuildManager(object):
if 'build_url' in extra_args:
self._builder.log("%s\n" % extra_args['build_url'])
- os.mkdir("%s/build-%s" % (self.home, self._buildid))
+ os.mkdir(get_build_path(self.home, self._buildid))
for f in files:
os.symlink(self._builder.cachePath(files[f]),
- "%s/build-%s/%s" % (self.home, self._buildid, f))
+ get_build_path(self.home, self._buildid, f))
self._chroottarfile = self._builder.cachePath(chroot)
self.image_type = extra_args.get('image_type', 'chroot')
@@ -261,6 +276,16 @@ class BuildManager(object):
This may be used to return manager-specific information from the
XML-RPC status call.
"""
+ status_path = get_build_path(self.home, self._buildid, "status")
+ try:
+ with open(status_path) as status_file:
+ return json.load(status_file)
+ except IOError:
+ pass
+ except Exception as e:
+ print(
+ "Error deserialising extra status file: %s" % e,
+ file=sys.stderr)
return {}
def iterate(self, success, quiet=False):
diff --git a/lpbuildd/debian.py b/lpbuildd/debian.py
index 3473b89..a3ac2ef 100644
--- a/lpbuildd/debian.py
+++ b/lpbuildd/debian.py
@@ -22,7 +22,10 @@ from twisted.internet import (
)
from twisted.python import log
-from lpbuildd.builder import BuildManager
+from lpbuildd.builder import (
+ BuildManager,
+ get_build_path,
+ )
class DebianBuildState:
@@ -343,14 +346,3 @@ class DebianBuildManager(BuildManager):
if self._iterator is not None:
self._iterator.cancel()
self._iterator = None
-
-
-def get_build_path(home, build_id, *extra):
- """Generate a path within the build directory.
-
- :param home: the user's home directory.
- :param build_id: the build id to use.
- :param extra: the extra path segments within the build directory.
- :return: the generated path.
- """
- return os.path.join(home, "build-" + build_id, *extra)
diff --git a/lpbuildd/snap.py b/lpbuildd/snap.py
index 5afead1..0c42f12 100644
--- a/lpbuildd/snap.py
+++ b/lpbuildd/snap.py
@@ -5,9 +5,7 @@ from __future__ import print_function
__metaclass__ = type
-import json
import os
-import sys
from six.moves.configparser import (
NoOptionError,
@@ -17,7 +15,6 @@ from six.moves.configparser import (
from lpbuildd.debian import (
DebianBuildManager,
DebianBuildState,
- get_build_path,
)
from lpbuildd.proxy import BuildManagerProxyMixin
@@ -61,19 +58,6 @@ class SnapBuildManager(BuildManagerProxyMixin, DebianBuildManager):
super(SnapBuildManager, self).initiate(files, chroot, extra_args)
- def status(self):
- status_path = get_build_path(self.home, self._buildid, "status")
- try:
- with open(status_path) as status_file:
- return json.load(status_file)
- except IOError:
- pass
- except Exception as e:
- print(
- "Error deserialising status from buildsnap: %s" % e,
- file=sys.stderr)
- return {}
-
def doRunBuild(self):
"""Run the process to build the snap."""
args = []
diff --git a/lpbuildd/sourcepackagerecipe.py b/lpbuildd/sourcepackagerecipe.py
index 4e237a6..af818b0 100644
--- a/lpbuildd/sourcepackagerecipe.py
+++ b/lpbuildd/sourcepackagerecipe.py
@@ -7,10 +7,10 @@
import os
import re
+from lpbuildd.builder import get_build_path
from lpbuildd.debian import (
DebianBuildManager,
DebianBuildState,
- get_build_path,
)
diff --git a/lpbuildd/target/build_charm.py b/lpbuildd/target/build_charm.py
index 5d0b3ab..c55e9dd 100644
--- a/lpbuildd/target/build_charm.py
+++ b/lpbuildd/target/build_charm.py
@@ -110,7 +110,7 @@ class BuildCharm(BuilderProxyOperationMixin, VCSOperationMixin,
logger.info("Running repo phase...")
env = self.build_proxy_environment(proxy_url=self.args.proxy_url)
self.vcs_fetch(self.args.name, cwd="/home/buildd", env=env)
- self.save_status(self.buildd_path)
+ self.vcs_update_status(self.buildd_path)
def build(self):
logger.info("Running build phase...")
diff --git a/lpbuildd/target/build_snap.py b/lpbuildd/target/build_snap.py
index 7ec133b..e06a40f 100644
--- a/lpbuildd/target/build_snap.py
+++ b/lpbuildd/target/build_snap.py
@@ -162,7 +162,7 @@ class BuildSnap(BuilderProxyOperationMixin, VCSOperationMixin,
logger.info("Running repo phase...")
env = self.build_proxy_environment(proxy_url=self.args.proxy_url)
self.vcs_fetch(self.args.name, cwd="/build", env=env)
- self.save_status(os.path.join("/build", self.args.name))
+ self.vcs_update_status(os.path.join("/build", self.args.name))
@property
def image_info(self):
diff --git a/lpbuildd/target/status.py b/lpbuildd/target/status.py
new file mode 100644
index 0000000..59611a0
--- /dev/null
+++ b/lpbuildd/target/status.py
@@ -0,0 +1,33 @@
+# Copyright 2018-2021 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import print_function
+
+__metaclass__ = type
+
+import json
+import os
+
+
+class StatusOperationMixin:
+ """Methods supporting operations that save extra status information.
+
+ Extra status information will be picked up by the build manager and
+ included in XML-RPC status responses.
+ """
+
+ def __init__(self, args, parser):
+ super(StatusOperationMixin, self).__init__(args, parser)
+ self._status = {}
+
+ def get_status(self):
+ """Return a copy of this operation's extra status."""
+ return dict(self._status)
+
+ def update_status(self, **status):
+ """Update this operation's status with key/value pairs."""
+ self._status.update(status)
+ status_path = os.path.join(self.backend.build_path, "status")
+ with open("%s.tmp" % status_path, "w") as status_file:
+ json.dump(self._status, status_file)
+ os.rename("%s.tmp" % status_path, status_path)
diff --git a/lpbuildd/target/vcs.py b/lpbuildd/target/vcs.py
index f02e540..164fd34 100644
--- a/lpbuildd/target/vcs.py
+++ b/lpbuildd/target/vcs.py
@@ -6,16 +6,17 @@ from __future__ import print_function
__metaclass__ = type
from collections import OrderedDict
-import json
import logging
import os.path
import subprocess
+from lpbuildd.target.status import StatusOperationMixin
+
logger = logging.getLogger(__name__)
-class VCSOperationMixin:
+class VCSOperationMixin(StatusOperationMixin):
"""Methods supporting operations that check out a branch from a VCS."""
@classmethod
@@ -106,15 +107,10 @@ class VCSOperationMixin:
"'git submodule update --init --recursive failed with "
"exit code %s (build may fail later)" % e.returncode)
- def save_status(self, cwd):
- """Save a dictionary of status information about this build.
-
- This will be picked up by the build manager and included in XML-RPC
- status responses.
- """
- status = {}
+ def vcs_update_status(self, cwd):
+ """Update this operation's status with VCS information."""
if self.args.branch is not None:
- status["revision_id"] = self.run_build_command(
+ revision_id = self.run_build_command(
["bzr", "revno"],
cwd=cwd,
get_output=True, universal_newlines=True).rstrip("\n")
@@ -122,13 +118,10 @@ class VCSOperationMixin:
rev = (
self.args.git_path
if self.args.git_path is not None else "HEAD")
- status["revision_id"] = self.run_build_command(
+ revision_id = self.run_build_command(
# The ^{} suffix copes with tags: we want to peel them
# recursively until we get an actual commit.
["git", "rev-parse", rev + "^{}"],
cwd=cwd,
get_output=True, universal_newlines=True).rstrip("\n")
- status_path = os.path.join(self.backend.build_path, "status")
- with open("%s.tmp" % status_path, "w") as status_file:
- json.dump(status, status_file)
- os.rename("%s.tmp" % status_path, status_path)
+ self.update_status(revision_id=revision_id)
diff --git a/lpbuildd/tests/test_charm.py b/lpbuildd/tests/test_charm.py
index 97b4a15..3968cc3 100644
--- a/lpbuildd/tests/test_charm.py
+++ b/lpbuildd/tests/test_charm.py
@@ -93,6 +93,16 @@ class TestCharmBuildManagerIteration(TestCase):
self.buildmanager.iterate, self.buildmanager.iterators[-1])
self.assertFalse(self.builder.wasCalled("chrootFail"))
+ def test_status(self):
+ # The build manager returns saved status information on request.
+ self.assertEqual({}, self.buildmanager.status())
+ status_path = os.path.join(
+ self.working_dir, "home", "build-%s" % self.buildid, "status")
+ os.makedirs(os.path.dirname(status_path))
+ with open(status_path, "w") as status_file:
+ status_file.write('{"revision_id": "foo"}')
+ self.assertEqual({"revision_id": "foo"}, self.buildmanager.status())
+
@defer.inlineCallbacks
def test_iterate(self):
# The build manager iterates a normal build from start to finish.
