[Merge] lp:~jamesodhunt/ubuntu/wily/ubuntu-core-upgrader/call-upgrader-directly into lp:ubuntu/ubuntu-core-upgrader
Michael Vogt
michael.vogt at canonical.com
Fri May 29 13:08:48 UTC 2015
I put some python code comments inline, I am not sure about the approach to test the python object and the executable in two different code paths. I think it would be preferable to convert the call_upgrader_object to use the in-tree binary to avoid having two code paths. Once the code calls a external executable (the bin/ubuntu-core-upgrader from this source tree) it will be trivial to convert to call a different one once the need arises (i.e. once there is a external one that needs testing).
So instead of call_upgrader_{object,command} maybe something like:
"""
def call_upgrader(command_file, root_dir):
pyroot = os.path.normpath(os.path.join(os.path.dirname(__file__), ".."))
env = copy.copy(os.environ)
env["PYTHONPATH"] = pyroot
subprocess.check_call(
[os.path.join(pyroot, "bin", "ubuntu-core-upgrade"),
'--root-dir', root_dir,
'--debug', '1',
# don't delete the archive and command files.
# The tests clean up after themselves so they will get removed then,
# but useful to have them around to diagnose test failures.
'--leave-files',
command_file],
env=env)
"""
Diff comments:
> === modified file 'debian/changelog'
> --- debian/changelog 2015-04-23 09:01:01 +0000
> +++ debian/changelog 2015-05-19 10:30:58 +0000
> @@ -1,3 +1,16 @@
> +ubuntu-core-upgrader (0.7.12) UNRELEASED; urgency=medium
> +
> + * functional/test_upgrader.py:
This changelog describes the change in ubuntucoreupgrader/upgrader.py
> + - prepare(): Set cache dir to the root dir if '--root-dir' specified.
> + * ubuntucoreupgrader/upgrader.py:
This change describes the change in function/test_upgrader.py
> + - If the real upgrader exists, use it rather than creating an Upgrader
> + object. This will eventually allow the upgrader to be replaced (but
> + these tests to be retained, atleast for the medium term).
> + - Renamed call_upgrader() to call_upgrader_object().
> + - Removed unused update argument for call_upgrader_object().
> +
> + -- James Hunt <james.hunt at ubuntu.com> Tue, 19 May 2015 11:28:09 +0100
> +
> ubuntu-core-upgrader (0.7.11) vivid; urgency=medium
>
> * ubuntucoreupgrader/upgrader.py: Call os.sync() at end of sync_partitions()
>
> === modified file 'functional/test_upgrader.py'
> --- functional/test_upgrader.py 2015-04-08 14:54:05 +0000
> +++ functional/test_upgrader.py 2015-05-19 10:30:58 +0000
> @@ -24,6 +24,7 @@
> import sys
> import os
> import logging
> +import subprocess
> import unittest
>
> from ubuntucoreupgrader.upgrader import (
> @@ -42,16 +43,85 @@
> UbuntuCoreUpgraderTestCase,
> )
>
> +log = logging.getLogger()
If you use the python default logger, you could as well use logging.{debug,info,warning} AFAICS. Alternatively you could use log = logging.getLogger(__file__) to get different prefixes for each file.
> +
> CMD_FILE = 'ubuntu_command'
>
> -
> -def call_upgrader(command_file, root_dir, update):
> +UPGRADER = '/usr/bin/ubuntu-core-upgrade'
> +
> +
> +def call_upgrader(command_file, root_dir, upgrader=UPGRADER):
> +
> + found = False
This looks quite complex, why not just:
"""
if not os.path.exists(upgrader):
log.info("Using build-in upgrader object")
return call_upgrader_object()
log.info("Calling external upgrader command")
return call_upgrader_command()
"""
> +
> + if os.path.exists(upgrader):
> + found = True
> +
> + log.warning("Real upgrader ({}) {}found"
> + .format(upgrader, "" if found else "not"))
> +
> + if not found:
> + log.warning("Creating an Upgrader object instead")
> +
> + call_upgrader_object(command_file, root_dir)
> + return
> +
> + # Running on a system with the upgrader installed, so exercise it!
> + log.warning("Calling upgrader directly")
> +
> + options = [
These options are created again in call_upgrader_object as args. Would it make sense to have this only once?
> + # always run with full debug
> + '--debug', '2',
> + # don't delete the archive and command files.
> + # The tests clean up after themselves so they will get removed then,
> + # but useful to have them around to diagnose test failures.
> + '--leave-files',
> + ]
> +
> + if root_dir:
> + options += ['--root-dir', root_dir]
> +
> + call_upgrader_command(upgrader, options, command_file)
> +
> +
> +def call_upgrader_command(upgrader=UPGRADER, options=[],
This looks quite complext, why not simply:
"""
subprocess.check_call([upgrader]+options+[command_file])
"""
> + command_file=CMD_FILE):
> + '''
> + Invoke the upgrader binary directly.
> +
> + :param upgrader: path to the upgrader to use.
> + :param options: array of options to pass to the upgrader
> + :param command_file: commands file to drive the upgrader.
> + '''
> + args = [upgrader]
> + args.extend(options)
> +
> + args += [command_file]
> +
> + proc = subprocess.Popen(args,
> + stdout=subprocess.PIPE,
> + stderr=subprocess.PIPE,
> + universal_newlines=True)
> +
> + ret = proc.wait()
> +
> + if ret == 0:
> + return
> +
> + stdout, stderr = proc.communicate()
> + log.error("{} returned {}: stdout='{}', stderr='{}'"
> + .format(args,
> + ret,
> + stdout,
> + stderr))
> +
> +
> +def call_upgrader_object(command_file, root_dir):
> '''
> Invoke the upgrader.
>
> :param command_file: commands file to drive the upgrader.
> :param root_dir: Test directory to apply the upgrade to.
> - :param update: UpdateTree object.
> '''
>
> args = []
> @@ -207,7 +277,7 @@
> # remove 'system' suffix that upgrader will add back on
> vdir = os.path.split(self.victim_dir)[0]
>
> - call_upgrader(cmd_file, vdir, self.update)
> + call_upgrader(cmd_file, vdir)
>
> self.assertFalse(os.path.exists(file_path))
>
> @@ -232,7 +302,7 @@
> self.assertTrue(os.path.isdir(dir_path))
>
> vdir = os.path.split(self.victim_dir)[0]
> - call_upgrader(cmd_file, vdir, self.update)
> + call_upgrader(cmd_file, vdir)
>
> self.assertFalse(os.path.exists(dir_path))
>
> @@ -265,7 +335,7 @@
> self.assertTrue(os.path.islink(link_file_path))
>
> vdir = os.path.split(self.victim_dir)[0]
> - call_upgrader(cmd_file, vdir, self.update)
> + call_upgrader(cmd_file, vdir)
>
> # original file should still be there
> self.assertTrue(os.path.exists(src_file_path))
> @@ -304,7 +374,7 @@
> self.assertTrue(os.path.islink(link_file_path))
>
> vdir = os.path.split(self.victim_dir)[0]
> - call_upgrader(cmd_file, vdir, self.update)
> + call_upgrader(cmd_file, vdir)
>
> # original directory should still be there
> self.assertTrue(os.path.exists(src_dir_path))
> @@ -347,7 +417,7 @@
> self.assertTrue(src_inode == link_inode)
>
> vdir = os.path.split(self.victim_dir)[0]
> - call_upgrader(cmd_file, vdir, self.update)
> + call_upgrader(cmd_file, vdir)
>
> # original file should still be there
> self.assertTrue(os.path.exists(src_file_path))
> @@ -388,7 +458,7 @@
> self.assertTrue(os.path.isfile(file_path))
>
> vdir = os.path.split(self.victim_dir)[0]
> - call_upgrader(cmd_file, vdir, self.update)
> + call_upgrader(cmd_file, vdir)
>
> # upgrader doesn't currently remove device files
> self.assertTrue(os.path.exists(file_path))
> @@ -418,7 +488,7 @@
> file_path = os.path.join(self.victim_dir, file)
> self.assertFalse(os.path.exists(file_path))
>
> - call_upgrader(cmd_file, self.victim_dir, self.update)
> + call_upgrader(cmd_file, self.victim_dir)
>
> self.assertTrue(os.path.exists(file_path))
> self.assertTrue(os.path.isfile(file_path))
> @@ -442,7 +512,7 @@
> dir_path = os.path.join(self.victim_dir, dir)
> self.assertFalse(os.path.exists(dir_path))
>
> - call_upgrader(cmd_file, self.victim_dir, self.update)
> + call_upgrader(cmd_file, self.victim_dir)
>
> self.assertTrue(os.path.exists(dir_path))
> self.assertTrue(os.path.isdir(dir_path))
> @@ -487,7 +557,7 @@
>
> self.assertFalse(os.path.exists(victim_link_file_path))
>
> - call_upgrader(cmd_file, self.victim_dir, self.update)
> + call_upgrader(cmd_file, self.victim_dir)
>
> self.assertTrue(os.path.exists(victim_src_file_path))
> self.assertTrue(os.path.isfile(victim_src_file_path))
> @@ -538,7 +608,7 @@
>
> self.assertFalse(os.path.exists(victim_link_file_path))
>
> - call_upgrader(cmd_file, self.victim_dir, self.update)
> + call_upgrader(cmd_file, self.victim_dir)
>
> self.assertTrue(os.path.exists(victim_src_file_path))
> self.assertTrue(os.path.isfile(victim_src_file_path))
> @@ -581,7 +651,7 @@
> self.assertFalse(os.path.exists(victim_src_file_path))
> self.assertFalse(os.path.exists(victim_link_file_path))
>
> - call_upgrader(cmd_file, self.victim_dir, self.update)
> + call_upgrader(cmd_file, self.victim_dir)
>
> # source still shouldn't exist
> self.assertFalse(os.path.exists(victim_src_file_path))
> @@ -622,7 +692,7 @@
> # XXX: There is an implicit test here since if the upgrader
> # fails (as documented on LP: #1437225), this test will also
> # fail.
> - call_upgrader(cmd_file, vdir, self.update)
> + call_upgrader(cmd_file, vdir)
>
> self.assertTrue(os.path.exists(vdir))
> self.assertTrue(os.path.exists(self.victim_dir))
>
> === modified file 'ubuntucoreupgrader/upgrader.py'
> --- ubuntucoreupgrader/upgrader.py 2015-04-22 09:37:35 +0000
> +++ ubuntucoreupgrader/upgrader.py 2015-05-19 10:30:58 +0000
> @@ -553,6 +553,7 @@
>
> if self.options.root_dir != '/':
> # Don't modify root when running in test mode
> + self.cache_dir = self.options.root_dir
> return
>
> def run(self):
>
--
https://code.launchpad.net/~jamesodhunt/ubuntu/wily/ubuntu-core-upgrader/call-upgrader-directly/+merge/259481
Your team Ubuntu branches is requested to review the proposed merge of lp:~jamesodhunt/ubuntu/wily/ubuntu-core-upgrader/call-upgrader-directly into lp:ubuntu/ubuntu-core-upgrader.
More information about the Ubuntu-reviews
mailing list