Merge lp:~jamesodhunt/ubuntu/wily/ubuntu-core-upgrader/call-upgrader-directly into lp:ubuntu/wily/ubuntu-core-upgrader

[James Hunt]

[Michael Vogt (mvo)]

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)
"""

[James Hunt (jamesodhunt)]

I'm slightly confused by this - if we replace call_upgrader_object() and call_upgrader_command() with a single call_upgrader(), most of the in-line comments are no longer relevant right?

[Sebastien Bacher (seb128)]

seems like work is needed/ongoing discussion is going with mvo, changing to "work in progress" to get out of the sponsoring queue

Unmerged revisions

33. By James Hunt on 2015-05-19

* functional/test_upgrader.py:
  - prepare(): Set cache dir to the root dir if '--root-dir' specified.
* ubuntucoreupgrader/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().

=== 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:
+    - prepare(): Set cache dir to the root dir if '--root-dir' specified.
+  * ubuntucoreupgrader/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 <[e-mail]>  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()
+
 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
+
+    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 = [
+        # 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=[],
+                          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):