Merge lp:~sil2100/ubuntu-archive-tools/kernel-sru-review-cache into lp:ubuntu-archive-tools

[Łukasz Zemczak]

Add an option to provide a kernel tarball cache directory to kernel-sru-review from which the orig tarballs will be used and stored for later script re-runs.

Add an option to provide a kernel tarball cache directory to kernel-sru-review from which the orig tarballs will be used and stored for later script re-runs.

Rationale: currently every run of kernel-sru-review pulls the orig tarball for the kernel as we work in a temporary directory that gets cleaned up after termination. Considering how infrequently the upstream versions are changed this way we can save a lot of time.

[Steve Langasek (vorlon)]

LGTM with a few changes.

I would also like to get Adam's review, since I know he has some sort of local mirroring setup he references, to know if this would meet his needs or if something different is needed for his use case.

review: Needs Fixing

[lp:~sil2100/ubuntu-archive-tools/kernel-sru-review-cache]

1092. By Łukasz Zemczak on 2017-04-18

Fixes as per Steve's review.

[Łukasz Zemczak (sil2100)]

Is there anything else I could do to get this merged? I would opt to just get it in and modify later on if Adam has some additional requirements.

[lp:~sil2100/ubuntu-archive-tools/kernel-sru-review-cache]

1093. By Łukasz Zemczak on 2017-06-08

Attempt to hard link before doing a copy of the tarballs.

1094. By Łukasz Zemczak on 2017-06-08

As per Adam's suggestions, add a default cache tarball directory and make sure we look at the cwd for the tarballs first.

1095. By Łukasz Zemczak on 2017-06-12

By accident commited a debug-only change.

1096. By Łukasz Zemczak on 2017-06-12

Another change - make sure that we don't copy when link already exists. Don't do anything unnecessary.

[Łukasz Zemczak (sil2100)]

The latest changes have been tested in production - I used those for recent kernel reviews. All cases have been verified: setting an explicit tarball directory, using the default tarball directory and using the current working directory. Everything seemed to work as expected.

Could we get a final review on this one?

[Łukasz Zemczak (sil2100)]

Any movement on this one? I have addressed Adam's concerns with the last batch of changes. Since I have been using this since 3 or 4 kernel releases already, I would really like the permission to get this merged. It should also fit more Adam's workflow as well as he can just execute it from the tarball cache directory and all works like a charm.

[Steve Langasek (vorlon)]

Sorry for the delays in review... a few more changes requested :(

review: Needs Fixing

[lp:~sil2100/ubuntu-archive-tools/kernel-sru-review-cache]

1097. By Łukasz Zemczak on 2017-07-11

Do not cache tarballs by default, but still use tarballs from the tardir if available.

1098. By Łukasz Zemczak on 2017-07-11

Slightly rename some variables + also pass the work directory as context.

[Łukasz Zemczak (sil2100)]

The reason why I was passing only the 'main directory' in the context field was that I knew that I could always get the work directory through getcwd() inside any function, not having to pass it through the context. Since for tarball cache handling I need to know both directories - the work directory (i.e. the temporary dir that we create) and the start directory. The work one for known reasons - we need to know where we're copying the tarballs to. The start directory, on the other hand, is also needed as per Adam's request we are supposed to look for tarballs also in the directory where the script was executed in.

Passing only the start directory allowed me to not skip passing one directory. But yeah, there's no harm in passing it I guess, so I did that now.

After the latest changes, all concerns should have been addressed. Currently tarball caching is not enabled by default but we do try to fetch tarballs from cache directories (or the cwd) if available always. This way we should also satisfy Adam's needs.

[Steve Langasek (vorlon)]

I like this very much.

review: Approve

=== modified file 'kernel-sru-review'
--- kernel-sru-review	2017-03-17 12:17:00 +0000
+++ kernel-sru-review	2017-07-11 09:42:52 +0000
@@ -28,6 +28,7 @@
     kernel-sru-review [-s precise] linux
 """
 
+import glob
 import datetime
 import os
 import pytz
@@ -59,13 +60,58 @@
     for source in full_packages:
         process_source_package(
             source, release, context['archive'], context['ppa'],
-            context['ubuntu'])
+            context['ubuntu'], context['startdir'], context['workdir'],
+            context['tardir'], context['tarcache'])
     tasks['proposed'].status = 'Fix Committed'
     tasks['proposed'].assignee = lp.me
     tasks['proposed'].lp_save()
 
 
-def process_source_package(source, release, archive, ppa, ubuntu):
+def fetch_tarball_from_cache(directory, tardir, source, version, cwd):
+    actual_tardir = None
+    tarballs = []
+    # if a tarball directory is given, try to find the tarball there
+    if tardir:
+        glob_pattern = '%s_%s.orig.tar.*' % (source, version)
+        # first we look in the current working directory where the command was
+        # called from
+        actual_tardir = cwd
+        tarballs = glob.glob(os.path.join(cwd, glob_pattern))
+        if not tarballs:
+            actual_tardir = tardir
+            tarballs = glob.glob(os.path.join(tardir, glob_pattern))
+        if tarballs:
+            target = os.path.join(directory, os.path.basename(tarballs[0]))
+            try:
+                os.link(tarballs[0], target)
+            except FileExistsError:
+                pass
+            except:
+                # if the hard linking fails, do a copy operation
+                shutil.copy(tarballs[0], target)
+        else:
+            actual_tardir = None
+    return actual_tardir
+
+
+def save_tarball_to_cache(directory, tardir, source, version):
+    # if we use a tarball directory and downloaded a new tarball, copy it over
+    if tardir:
+        glob_pattern = '%s_%s.orig.tar.*' % (source, version)
+        to_copy = glob.glob(os.path.join(directory, glob_pattern))
+        for tarball in to_copy:
+            target = os.path.join(tardir, os.path.basename(tarball))
+            try:
+                os.link(tarball, target)
+            except FileExistsError:
+                pass
+            except:
+                # if the hard linking fails, do a copy operation
+                shutil.copy(tarball, target)
+
+
+def process_source_package(source, release, archive, ppa, ubuntu,
+                           start_dir, work_dir, tardir=None, tar_cache=False):
     series = ubuntu.getSeries(name_or_version=release)
 
     ppa_src = ppa.getPublishedSources(order_by_date=True,
@@ -103,6 +149,9 @@
     old_majorabi = re.sub(r"\.[^.]+$", '', old_fullabi)
     old_upstream = old_fullabi.split('-')[0]
 
+    real_tardir = fetch_tarball_from_cache(
+        work_dir, tardir, source, old_upstream, start_dir)
+
     # grab the old source first
     pull_cmd = ['pull-lp-source', source, source_ver]
     try:
@@ -125,9 +174,15 @@
               (source, source_ver))
         raise e
 
+    if not real_tardir and tar_cache:
+        save_tarball_to_cache(work_dir, tardir, source, old_upstream)
+
     # move the source dir aside so that it doesn't clobber.
     os.rename(source + '-' + old_upstream, source + '-' + old_upstream + '.old')
 
+    real_tardir = fetch_tarball_from_cache(
+        work_dir, tardir, source, new_upstream, start_dir)
+
     # grab the new source
     dget_cmd = ['dget', '-u', ppa_dsc]
     try:
@@ -137,6 +192,9 @@
               (source, ppa_ver))
         raise e
 
+    if not real_tardir and tar_cache:
+        save_tarball_to_cache(work_dir, tardir, source, new_upstream)
+
     # look at the diff
     diff_cmd = ('diff -uNr "{}-{}.old" "{}-{}" | filterdiff -x \'**/abi/**\''
                 + ' | sensible-pager').format(
@@ -186,16 +244,37 @@
     parser = OptionParser(
         usage="Usage: %prog [options] bug [bug ...]")
 
+    cachedir = os.path.expanduser(
+        '~/.cache/ubuntu-archive-tools/kernel-tarballs')
+
     parser.add_option(
         "-l", "--launchpad", dest="launchpad_instance", default="production")
     parser.add_option(
         "-k", "--keep-files", dest="keep_files", action="store_true")
+    parser.add_option(
+        "-C", "--cache-tarballs", dest="caching", action="store_true")
+    parser.add_option(
+        "-t", "--tarball-directory", dest="tardir", default=cachedir)
 
     opts, bugs = parser.parse_args()
 
     if len(bugs) < 1:
         parser.error('Need to specify at least one bug number')
 
+    if opts.tardir:
+        tardir = os.path.abspath(opts.tardir)
+    else:
+        tardir = None
+
+    if opts.caching:
+        # if we enabled tarball caching, make sure the tarball directory exists
+        if tardir and not os.path.isdir(tardir):
+            try:
+                os.makedirs(tardir)
+            except:
+                parser.error(
+                    'Invalid tarball directory specified (%s)' % tardir)
+
     launchpad = Launchpad.login_with(
         'ubuntu-archive-tools', opts.launchpad_instance, version='devel')
 
@@ -204,15 +283,18 @@
     ppa = launchpad.people['canonical-kernel-team'].getPPAByName(
                 distribution=ubuntu, name='ppa')
 
-    context = {'archive': archive, 'ppa': ppa, 'ubuntu': ubuntu}
-    main_dir = os.getcwd()
+    start_dir = os.getcwd()
+    context = {
+      'archive': archive, 'ppa': ppa, 'ubuntu': ubuntu,
+      'tardir': tardir, 'tarcache': opts.caching, 'startdir': start_dir }
     for bugnum in bugs:
         with ExitStack() as resources:
-            cwd = mkdtemp(prefix='kernel-sru-{}-'.format(bugnum), dir=main_dir)
+            cwd = mkdtemp(prefix='kernel-sru-%s-' % bugnum, dir=start_dir)
             if not opts.keep_files:
                 resources.callback(shutil.rmtree, cwd)
             os.chdir(cwd)
+            context['workdir'] = cwd
             process_sru_bug(
                 launchpad, bugnum, review_task_callback,
                 review_source_callback, context)
-            os.chdir(main_dir)
+            os.chdir(start_dir)