launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #22110
[Merge] lp:~cjwatson/launchpad/refactor-run-parts-subprocess into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/refactor-run-parts-subprocess into lp:launchpad.
Commit message:
Refactor publish-ftpmaster to use subprocess.call rather than os.system.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/refactor-run-parts-subprocess/+merge/336298
As usual, avoiding unnecessary use of the shell makes things safer and simpler.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/refactor-run-parts-subprocess into lp:launchpad.
=== modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
--- lib/lp/archivepublisher/scripts/publish_ftpmaster.py 2016-12-05 22:16:25 +0000
+++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py 2018-01-18 13:58:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2011-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Master distro publishing script."""
@@ -11,7 +11,12 @@
from datetime import datetime
import math
import os
+try:
+ from shlex import quote as shell_quote
+except ImportError:
+ from pipes import quote as shell_quote
import shutil
+import subprocess
from pytz import utc
from zope.component import getUtility
@@ -61,43 +66,6 @@
if archive.purpose in ARCHIVES_TO_PUBLISH]
-def compose_shell_boolean(boolean_value):
- """Represent a boolean value as "yes" or "no"."""
- boolean_text = {
- True: "yes",
- False: "no",
- }
- return boolean_text[boolean_value]
-
-
-def shell_quote(literal):
- """Escape `literal` for use in a double-quoted shell string.
-
- This is a pretty naive substitution: it doesn't deal well with
- non-ASCII characters or special characters.
- """
- # Characters that need backslash-escaping. Do the backslash itself
- # first; any escapes we introduced *before* the backslash would have
- # their own backslashes escaped afterwards when we got to the
- # backslash itself.
- special_characters = '\\"$`\n'
- for escapee in special_characters:
- literal = literal.replace(escapee, '\\' + escapee)
- return '"%s"' % literal
-
-
-def compose_env_string(*env_dicts):
- """Turn dict(s) into a series of shell parameter assignments.
-
- Values in later dicts override any values with the same respective
- keys in earlier dicts.
- """
- env = {}
- for env_dict in env_dicts:
- env.update(env_dict)
- return ' '.join(['='.join(pair) for pair in env.iteritems()])
-
-
def get_backup_dists(archive_config):
"""Return the path of an archive's backup dists directory."""
return os.path.join(archive_config.archiveroot + "-distscopy", "dists")
@@ -122,31 +90,78 @@
return get_dists(archive_config) + ".in-progress"
-def extend_PATH():
- """Produce env dict for extending $PATH.
-
- Adds the Launchpad source tree's cronscripts/publishing to the
- existing $PATH.
-
- :return: a dict suitable for passing to compose_env_string.
- """
- scripts_dir = os.path.join(config.root, "cronscripts", "publishing")
- return {"PATH": '"$PATH":%s' % shell_quote(scripts_dir)}
-
-
-def find_run_parts_dir(distro, parts):
+def find_run_parts_dir(distribution_name, parts):
"""Find the requested run-parts directory, if it exists."""
run_parts_location = config.archivepublisher.run_parts_location
if not run_parts_location:
return
- parts_dir = os.path.join(run_parts_location, distro.name, parts)
+ parts_dir = os.path.join(run_parts_location, distribution_name, parts)
if file_exists(parts_dir):
return parts_dir
else:
return None
+def extend_PATH(env):
+ """Extend $PATH in an environment dictionary.
+
+ Adds the Launchpad source tree's cronscripts/publishing to the
+ existing $PATH.
+ """
+ scripts_dir = os.path.join(config.root, "cronscripts", "publishing")
+ path_elements = env.get("PATH", "").split(os.pathsep)
+ path_elements.append(scripts_dir)
+ env["PATH"] = os.pathsep.join(path_elements)
+
+
+def execute_subprocess(args, log=None, failure=None, **kwargs):
+ """Run `args`, handling logging and failures.
+
+ :param args: Program argument array.
+ :param log: An optional logger.
+ :param failure: Raise `failure` as an exception if the command returns a
+ nonzero value. If omitted, nonzero return values are ignored.
+ :param kwargs: Other keyword arguments passed on to `subprocess.call`.
+ """
+ if log is not None:
+ log.debug("Executing: %s", " ".join(shell_quote(arg) for arg in args))
+ retval = subprocess.call(args, **kwargs)
+ if retval != 0:
+ if log is not None:
+ log.debug("Command returned %d.", retval)
+ if failure is not None:
+ if log is not None:
+ log.debug("Command failed: %s", failure)
+ raise failure
+
+
+def run_parts(distribution_name, parts, log=None, env=None):
+ """Execute run-parts.
+
+ :param distribution_name: The name of the distribution to execute
+ run-parts scripts for.
+ :param parts: The run-parts directory to execute:
+ "publish-distro.d" or "finalize.d".
+ :param log: An optional logger.
+ :param env: A dict of additional environment variables to pass to the
+ scripts in the run-parts directory, or None.
+ """
+ parts_dir = find_run_parts_dir(distribution_name, parts)
+ if parts_dir is None:
+ if log is not None:
+ log.debug("Skipping run-parts %s: not configured.", parts)
+ return
+ cmd = ["run-parts", "--", parts_dir]
+ failure = LaunchpadScriptFailure(
+ "Failure while executing run-parts %s." % parts_dir)
+ full_env = dict(os.environ)
+ if env is not None:
+ full_env.update(env)
+ extend_PATH(full_env)
+ execute_subprocess(cmd, log=None, failure=failure, env=full_env)
+
+
def map_distro_pubconfigs(distro):
"""Return dict mapping archive purpose for distro's pub configs.
@@ -238,26 +253,6 @@
"Distribution %s not found." % self.options.distribution)
self.distributions = [distro]
- def executeShell(self, command_line, failure=None):
- """Run `command_line` through a shell.
-
- This won't just load an external program and run it; the command
- line goes through the full shell treatment including variable
- substitutions, output redirections, and so on.
-
- :param command_line: Shell command.
- :param failure: Raise `failure` as an exception if the shell
- command returns a nonzero value. If omitted, nonzero return
- values are ignored.
- """
- self.logger.debug("Executing: %s" % command_line)
- retval = os.system(command_line)
- if retval != 0:
- self.logger.debug("Command returned %d.", retval)
- if failure is not None:
- self.logger.debug("Command failed: %s", failure)
- raise failure
-
def getConfigs(self):
"""Set up configuration objects for archives to be published.
@@ -370,8 +365,9 @@
for purpose, archive_config in self.configs[distribution].iteritems():
dists = get_dists(archive_config)
backup_dists = get_backup_dists(archive_config)
- self.executeShell(
- "rsync -aH --delete '%s/' '%s'" % (dists, backup_dists),
+ execute_subprocess(
+ ["rsync", "-aH", "--delete", "%s/" % dists, backup_dists],
+ log=self.logger,
failure=LaunchpadScriptFailure(
"Failed to rsync new dists for %s." % purpose.title))
@@ -469,12 +465,13 @@
"""Execute the publish-distro hooks."""
archive_config = self.configs[distribution][archive.purpose]
env = {
- 'ARCHIVEROOT': shell_quote(archive_config.archiveroot),
- 'DISTSROOT': shell_quote(get_backup_dists(archive_config)),
+ 'ARCHIVEROOT': archive_config.archiveroot,
+ 'DISTSROOT': get_backup_dists(archive_config),
}
if archive_config.overrideroot is not None:
- env["OVERRIDEROOT"] = shell_quote(archive_config.overrideroot)
- self.runParts(distribution, 'publish-distro.d', env)
+ env["OVERRIDEROOT"] = archive_config.overrideroot
+ run_parts(
+ distribution.name, 'publish-distro.d', log=self.logger, env=env)
def installDists(self, distribution):
"""Put the new dists into place, as near-atomically as possible.
@@ -499,40 +496,22 @@
def clearEmptyDirs(self, distribution):
"""Clear out any redundant empty directories."""
for archive_config in self.configs[distribution].itervalues():
- self.executeShell(
- "find '%s' -type d -empty | xargs -r rmdir"
- % archive_config.archiveroot)
-
- def runParts(self, distribution, parts, env):
- """Execute run-parts.
-
- :param distribution: `Distribution` to execute run-parts scripts for.
- :param parts: The run-parts directory to execute:
- "publish-distro.d" or "finalize.d".
- :param env: A dict of environment variables to pass to the
- scripts in the run-parts directory.
- """
- parts_dir = find_run_parts_dir(distribution, parts)
- if parts_dir is None:
- self.logger.debug("Skipping run-parts %s: not configured.", parts)
- return
- env_string = compose_env_string(env, extend_PATH())
- self.executeShell(
- "env %s run-parts -- '%s'" % (env_string, parts_dir),
- failure=LaunchpadScriptFailure(
- "Failure while executing run-parts %s." % parts_dir))
+ execute_subprocess(
+ ["find", archive_config.archiveroot, "-type", "d", "-empty",
+ "-delete"],
+ log=self.logger)
def runFinalizeParts(self, distribution, security_only=False):
"""Run the finalize.d parts to finalize publication."""
- archive_roots = shell_quote(' '.join([
+ archive_roots = ' '.join([
archive_config.archiveroot
- for archive_config in self.configs[distribution].itervalues()]))
+ for archive_config in self.configs[distribution].itervalues()])
env = {
- 'SECURITY_UPLOAD_ONLY': compose_shell_boolean(security_only),
+ 'SECURITY_UPLOAD_ONLY': 'yes' if security_only else 'no',
'ARCHIVEROOTS': archive_roots,
}
- self.runParts(distribution, 'finalize.d', env)
+ run_parts(distribution.name, 'finalize.d', log=self.logger, env=env)
def publishSecurityUploads(self, distribution):
"""Quickly process just the pending security uploads.
=== modified file 'lib/lp/archivepublisher/tests/test_publish_ftpmaster.py'
--- lib/lp/archivepublisher/tests/test_publish_ftpmaster.py 2016-12-05 22:16:25 +0000
+++ lib/lp/archivepublisher/tests/test_publish_ftpmaster.py 2018-01-18 13:58:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2011-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Test publish-ftpmaster cron script."""
@@ -13,6 +13,8 @@
from apt_pkg import TagFile
from fixtures import MonkeyPatch
from testtools.matchers import (
+ ContainsDict,
+ Equals,
MatchesException,
MatchesStructure,
Not,
@@ -26,13 +28,12 @@
from lp.archivepublisher.config import getPubConfig
from lp.archivepublisher.interfaces.publisherconfig import IPublisherConfigSet
from lp.archivepublisher.scripts.publish_ftpmaster import (
- compose_env_string,
- compose_shell_boolean,
+ execute_subprocess,
find_run_parts_dir,
get_working_dists,
newer_mtime,
PublishFTPMaster,
- shell_quote,
+ run_parts,
)
from lp.registry.interfaces.pocket import (
PackagePublishingPocket,
@@ -60,10 +61,7 @@
TestCaseWithFactory,
)
from lp.testing.fakemethod import FakeMethod
-from lp.testing.layers import (
- LaunchpadZopelessLayer,
- ZopelessDatabaseLayer,
- )
+from lp.testing.layers import LaunchpadZopelessLayer
def path_exists(*path_components):
@@ -178,42 +176,49 @@
self.makeTemporaryDirectory())
-class TestPublishFTPMasterHelpers(TestCase):
-
- def test_compose_env_string_iterates_env_dict(self):
- env = {
- "A": "1",
- "B": "2",
- }
- env_string = compose_env_string(env)
- self.assertIn(env_string, ["A=1 B=2", "B=2 A=1"])
-
- def test_compose_env_string_combines_env_dicts(self):
- env1 = {"A": "1"}
- env2 = {"B": "2"}
- env_string = compose_env_string(env1, env2)
- self.assertIn(env_string, ["A=1 B=2", "B=2 A=1"])
-
- def test_compose_env_string_overrides_repeated_keys(self):
- self.assertEqual("A=2", compose_env_string({"A": "1"}, {"A": "2"}))
-
- def test_shell_quote_quotes_string(self):
- self.assertEqual('"x"', shell_quote("x"))
-
- def test_shell_quote_escapes_string(self):
- self.assertEqual('"\\\\"', shell_quote("\\"))
-
- def test_shell_quote_does_not_escape_its_own_escapes(self):
- self.assertEqual('"\\$"', shell_quote("$"))
-
- def test_shell_quote_escapes_entire_string(self):
- self.assertEqual('"\\$\\$\\$"', shell_quote("$$$"))
-
- def test_compose_shell_boolean_shows_True_as_yes(self):
- self.assertEqual("yes", compose_shell_boolean(True))
-
- def test_compose_shell_boolean_shows_False_as_no(self):
- self.assertEqual("no", compose_shell_boolean(False))
+class TestPublishFTPMasterHelpers(TestCase, HelpersMixin):
+
+ def test_execute_subprocess_executes_shell_command(self):
+ marker = os.path.join(self.makeTemporaryDirectory(), "marker")
+ execute_subprocess(["touch", marker])
+ self.assertTrue(file_exists(marker))
+
+ def test_execute_subprocess_reports_failure_if_requested(self):
+ class ArbitraryFailure(Exception):
+ """Some exception that's not likely to come from elsewhere."""
+
+ self.assertRaises(
+ ArbitraryFailure,
+ execute_subprocess, ["/bin/false"], failure=ArbitraryFailure())
+
+ def test_execute_subprocess_does_not_report_failure_if_not_requested(self):
+ # The test is that this does not fail:
+ execute_subprocess(["/bin/false"])
+
+ def test_run_parts_runs_parts(self):
+ self.enableRunParts()
+ execute_subprocess_fixture = self.useFixture(MonkeyPatch(
+ "lp.archivepublisher.scripts.publish_ftpmaster.execute_subprocess",
+ FakeMethod()))
+ run_parts("ubuntu", "finalize.d", log=DevNullLogger(), env={})
+ self.assertEqual(1, execute_subprocess_fixture.new_value.call_count)
+ args, kwargs = execute_subprocess_fixture.new_value.calls[-1]
+ self.assertEqual(
+ (["run-parts", "--",
+ os.path.join(self.parts_directory, "ubuntu/finalize.d")],),
+ args)
+
+ def test_run_parts_passes_parameters(self):
+ self.enableRunParts()
+ execute_subprocess_fixture = self.useFixture(MonkeyPatch(
+ "lp.archivepublisher.scripts.publish_ftpmaster.execute_subprocess",
+ FakeMethod()))
+ key = self.factory.getUniqueString()
+ value = self.factory.getUniqueString()
+ run_parts(
+ "ubuntu", "finalize.d", log=DevNullLogger(), env={key: value})
+ args, kwargs = execute_subprocess_fixture.new_value.calls[-1]
+ self.assertThat(kwargs["env"], ContainsDict({key: Equals(value)}))
class TestNewerMtime(TestCase):
@@ -256,31 +261,26 @@
self.assertTrue(newer_mtime(self.a, self.b))
-class TestFindRunPartsDir(TestCaseWithFactory, HelpersMixin):
- layer = ZopelessDatabaseLayer
+class TestFindRunPartsDir(TestCase, HelpersMixin):
def test_find_run_parts_dir_finds_runparts_directory(self):
self.enableRunParts()
- ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
self.assertEqual(
os.path.join(
config.root, self.parts_directory, "ubuntu", "finalize.d"),
- find_run_parts_dir(ubuntu, "finalize.d"))
+ find_run_parts_dir("ubuntu", "finalize.d"))
def test_find_run_parts_dir_ignores_blank_config(self):
self.enableRunParts("")
- ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
- self.assertIs(None, find_run_parts_dir(ubuntu, "finalize.d"))
+ self.assertIs(None, find_run_parts_dir("ubuntu", "finalize.d"))
def test_find_run_parts_dir_ignores_none_config(self):
self.enableRunParts("none")
- ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
- self.assertIs(None, find_run_parts_dir(ubuntu, "finalize.d"))
+ self.assertIs(None, find_run_parts_dir("ubuntu", "finalize.d"))
def test_find_run_parts_dir_ignores_nonexistent_directory(self):
self.enableRunParts()
- distro = self.factory.makeDistribution()
- self.assertIs(None, find_run_parts_dir(distro, "finalize.d"))
+ self.assertIs(None, find_run_parts_dir("nonexistent", "finalize.d"))
class TestPublishFTPMasterScript(TestCaseWithFactory, HelpersMixin):
@@ -552,12 +552,14 @@
script = self.makeScript(distro)
script.setUp()
script.setUpDirs()
- script.runParts = FakeMethod()
+ run_parts_fixture = self.useFixture(MonkeyPatch(
+ "lp.archivepublisher.scripts.publish_ftpmaster.run_parts",
+ FakeMethod()))
script.publishDistroArchive(distro, distro.main_archive)
- self.assertEqual(1, script.runParts.call_count)
- args, kwargs = script.runParts.calls[0]
- run_distro, parts_dir, env = args
- self.assertEqual(distro, run_distro)
+ self.assertEqual(1, run_parts_fixture.new_value.call_count)
+ args, _ = run_parts_fixture.new_value.calls[0]
+ run_distro_name, parts_dir = args
+ self.assertEqual(distro.name, run_distro_name)
self.assertEqual("publish-distro.d", parts_dir)
def test_runPublishDistroParts_passes_parameters(self):
@@ -565,14 +567,19 @@
script = self.makeScript(distro)
script.setUp()
script.setUpDirs()
- script.runParts = FakeMethod()
+ run_parts_fixture = self.useFixture(MonkeyPatch(
+ "lp.archivepublisher.scripts.publish_ftpmaster.run_parts",
+ FakeMethod()))
script.runPublishDistroParts(distro, distro.main_archive)
- args, kwargs = script.runParts.calls[0]
- run_distro, parts_dir, env = args
- required_parameters = set([
- "ARCHIVEROOT", "DISTSROOT", "OVERRIDEROOT"])
- missing_parameters = required_parameters.difference(set(env.keys()))
- self.assertEqual(set(), missing_parameters)
+ _, kwargs = run_parts_fixture.new_value.calls[0]
+ distro_config = get_pub_config(distro)
+ self.assertThat(kwargs["env"], ContainsDict({
+ "ARCHIVEROOT": Equals(get_archive_root(distro_config)),
+ "DISTSROOT": Equals(
+ os.path.join(get_distscopy_root(distro_config), "dists")),
+ "OVERRIDEROOT": Equals(
+ get_archive_root(distro_config) + "-overrides"),
+ }))
def test_clearEmptyDirs_cleans_up_empty_directories(self):
distro = self.makeDistroWithPublishDirectory()
@@ -621,67 +628,16 @@
script.options.distribution = self.factory.getUniqueString()
self.assertRaises(LaunchpadScriptFailure, script.processOptions)
- def test_runParts_runs_parts(self):
- self.enableRunParts()
- script = self.makeScript(self.prepareUbuntu())
- script.setUp()
- distro = script.distributions[0]
- script.executeShell = FakeMethod()
- script.runParts(distro, "finalize.d", {})
- self.assertEqual(1, script.executeShell.call_count)
- args, kwargs = script.executeShell.calls[-1]
- command_line, = args
- self.assertIn("run-parts", command_line)
- self.assertIn(
- os.path.join(self.parts_directory, "ubuntu/finalize.d"),
- command_line)
-
- def test_runParts_passes_parameters(self):
- self.enableRunParts()
- script = self.makeScript(self.prepareUbuntu())
- script.setUp()
- distro = script.distributions[0]
- script.executeShell = FakeMethod()
- key = self.factory.getUniqueString()
- value = self.factory.getUniqueString()
- script.runParts(distro, "finalize.d", {key: value})
- args, kwargs = script.executeShell.calls[-1]
- command_line, = args
- self.assertIn("%s=%s" % (key, value), command_line)
-
- def test_executeShell_executes_shell_command(self):
- distro = self.makeDistroWithPublishDirectory()
- script = self.makeScript(distro)
- marker = os.path.join(
- get_pub_config(distro).root_dir, "marker")
- script.executeShell("touch %s" % marker)
- self.assertTrue(file_exists(marker))
-
- def test_executeShell_reports_failure_if_requested(self):
- distro = self.makeDistroWithPublishDirectory()
- script = self.makeScript(distro)
-
- class ArbitraryFailure(Exception):
- """Some exception that's not likely to come from elsewhere."""
-
- self.assertRaises(
- ArbitraryFailure,
- script.executeShell, "/bin/false", failure=ArbitraryFailure())
-
- def test_executeShell_does_not_report_failure_if_not_requested(self):
- distro = self.makeDistroWithPublishDirectory()
- script = self.makeScript(distro)
- # The test is that this does not fail:
- script.executeShell("/bin/false")
-
def test_runFinalizeParts_passes_parameters(self):
script = self.makeScript(self.prepareUbuntu())
script.setUp()
distro = script.distributions[0]
- script.runParts = FakeMethod()
+ run_parts_fixture = self.useFixture(MonkeyPatch(
+ "lp.archivepublisher.scripts.publish_ftpmaster.run_parts",
+ FakeMethod()))
script.runFinalizeParts(distro)
- args, kwargs = script.runParts.calls[0]
- run_distro, parts_dir, env = args
+ _, kwargs = run_parts_fixture.new_value.calls[0]
+ env = kwargs["env"]
required_parameters = set(["ARCHIVEROOTS", "SECURITY_UPLOAD_ONLY"])
missing_parameters = required_parameters.difference(set(env.keys()))
self.assertEqual(set(), missing_parameters)
@@ -798,15 +754,11 @@
self.assertContentEqual(
[distro.main_archive, partner_archive], published_archives)
- def test_runFinalizeParts_quotes_archiveroots(self):
- # Passing ARCHIVEROOTS to the finalize.d scripts is a bit
- # difficult because the variable holds multiple values in a
- # single, double-quoted string. Escaping and quoting a sequence
- # of escaped and quoted items won't work.
- # This test establishes how a script can sanely deal with the
- # list. It'll probably go wrong if the configured archive root
- # contains spaces and such, but should work with Unix-sensible
- # paths.
+ def test_runFinalizeParts_passes_archiveroots_correctly(self):
+ # The ARCHIVEROOTS environment variable may contain spaces, and
+ # these are passed through correctly. It'll go wrong if the
+ # configured archive root contains whitespace, but works with
+ # Unix-sensible paths.
distro = self.makeDistroWithPublishDirectory()
self.factory.makeArchive(
distribution=distro, purpose=ArchivePurpose.PARTNER)
Follow ups
