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

Proposed by Łukasz Zemczak
Status: Merged
Merged at revision: 1106
Proposed branch: lp:~sil2100/ubuntu-archive-tools/kernel-sru-review-cache
Merge into: lp:ubuntu-archive-tools
Diff against target: 169 lines (+88/-6)
1 file modified
kernel-sru-review (+88/-6)
To merge this branch: bzr merge lp:~sil2100/ubuntu-archive-tools/kernel-sru-review-cache
Reviewer Review Type Date Requested Status
Steve Langasek Approve
Adam Conrad Pending
Review via email: mp+322468@code.launchpad.net

Commit message

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.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Steve Langasek (vorlon) wrote :

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
1092. By Łukasz Zemczak

Fixes as per Steve's review.

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

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.

1093. By Łukasz Zemczak

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

1094. By Łukasz Zemczak

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

By accident commited a debug-only change.

1096. By Łukasz Zemczak

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

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

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?

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

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.

Revision history for this message
Steve Langasek (vorlon) wrote :

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

review: Needs Fixing
1097. By Łukasz Zemczak

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

1098. By Łukasz Zemczak

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

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

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.

Revision history for this message
Steve Langasek (vorlon) wrote :

I like this very much.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'kernel-sru-review'
2--- kernel-sru-review 2017-03-17 12:17:00 +0000
3+++ kernel-sru-review 2017-07-11 09:42:52 +0000
4@@ -28,6 +28,7 @@
5 kernel-sru-review [-s precise] linux
6 """
7
8+import glob
9 import datetime
10 import os
11 import pytz
12@@ -59,13 +60,58 @@
13 for source in full_packages:
14 process_source_package(
15 source, release, context['archive'], context['ppa'],
16- context['ubuntu'])
17+ context['ubuntu'], context['startdir'], context['workdir'],
18+ context['tardir'], context['tarcache'])
19 tasks['proposed'].status = 'Fix Committed'
20 tasks['proposed'].assignee = lp.me
21 tasks['proposed'].lp_save()
22
23
24-def process_source_package(source, release, archive, ppa, ubuntu):
25+def fetch_tarball_from_cache(directory, tardir, source, version, cwd):
26+ actual_tardir = None
27+ tarballs = []
28+ # if a tarball directory is given, try to find the tarball there
29+ if tardir:
30+ glob_pattern = '%s_%s.orig.tar.*' % (source, version)
31+ # first we look in the current working directory where the command was
32+ # called from
33+ actual_tardir = cwd
34+ tarballs = glob.glob(os.path.join(cwd, glob_pattern))
35+ if not tarballs:
36+ actual_tardir = tardir
37+ tarballs = glob.glob(os.path.join(tardir, glob_pattern))
38+ if tarballs:
39+ target = os.path.join(directory, os.path.basename(tarballs[0]))
40+ try:
41+ os.link(tarballs[0], target)
42+ except FileExistsError:
43+ pass
44+ except:
45+ # if the hard linking fails, do a copy operation
46+ shutil.copy(tarballs[0], target)
47+ else:
48+ actual_tardir = None
49+ return actual_tardir
50+
51+
52+def save_tarball_to_cache(directory, tardir, source, version):
53+ # if we use a tarball directory and downloaded a new tarball, copy it over
54+ if tardir:
55+ glob_pattern = '%s_%s.orig.tar.*' % (source, version)
56+ to_copy = glob.glob(os.path.join(directory, glob_pattern))
57+ for tarball in to_copy:
58+ target = os.path.join(tardir, os.path.basename(tarball))
59+ try:
60+ os.link(tarball, target)
61+ except FileExistsError:
62+ pass
63+ except:
64+ # if the hard linking fails, do a copy operation
65+ shutil.copy(tarball, target)
66+
67+
68+def process_source_package(source, release, archive, ppa, ubuntu,
69+ start_dir, work_dir, tardir=None, tar_cache=False):
70 series = ubuntu.getSeries(name_or_version=release)
71
72 ppa_src = ppa.getPublishedSources(order_by_date=True,
73@@ -103,6 +149,9 @@
74 old_majorabi = re.sub(r"\.[^.]+$", '', old_fullabi)
75 old_upstream = old_fullabi.split('-')[0]
76
77+ real_tardir = fetch_tarball_from_cache(
78+ work_dir, tardir, source, old_upstream, start_dir)
79+
80 # grab the old source first
81 pull_cmd = ['pull-lp-source', source, source_ver]
82 try:
83@@ -125,9 +174,15 @@
84 (source, source_ver))
85 raise e
86
87+ if not real_tardir and tar_cache:
88+ save_tarball_to_cache(work_dir, tardir, source, old_upstream)
89+
90 # move the source dir aside so that it doesn't clobber.
91 os.rename(source + '-' + old_upstream, source + '-' + old_upstream + '.old')
92
93+ real_tardir = fetch_tarball_from_cache(
94+ work_dir, tardir, source, new_upstream, start_dir)
95+
96 # grab the new source
97 dget_cmd = ['dget', '-u', ppa_dsc]
98 try:
99@@ -137,6 +192,9 @@
100 (source, ppa_ver))
101 raise e
102
103+ if not real_tardir and tar_cache:
104+ save_tarball_to_cache(work_dir, tardir, source, new_upstream)
105+
106 # look at the diff
107 diff_cmd = ('diff -uNr "{}-{}.old" "{}-{}" | filterdiff -x \'**/abi/**\''
108 + ' | sensible-pager').format(
109@@ -186,16 +244,37 @@
110 parser = OptionParser(
111 usage="Usage: %prog [options] bug [bug ...]")
112
113+ cachedir = os.path.expanduser(
114+ '~/.cache/ubuntu-archive-tools/kernel-tarballs')
115+
116 parser.add_option(
117 "-l", "--launchpad", dest="launchpad_instance", default="production")
118 parser.add_option(
119 "-k", "--keep-files", dest="keep_files", action="store_true")
120+ parser.add_option(
121+ "-C", "--cache-tarballs", dest="caching", action="store_true")
122+ parser.add_option(
123+ "-t", "--tarball-directory", dest="tardir", default=cachedir)
124
125 opts, bugs = parser.parse_args()
126
127 if len(bugs) < 1:
128 parser.error('Need to specify at least one bug number')
129
130+ if opts.tardir:
131+ tardir = os.path.abspath(opts.tardir)
132+ else:
133+ tardir = None
134+
135+ if opts.caching:
136+ # if we enabled tarball caching, make sure the tarball directory exists
137+ if tardir and not os.path.isdir(tardir):
138+ try:
139+ os.makedirs(tardir)
140+ except:
141+ parser.error(
142+ 'Invalid tarball directory specified (%s)' % tardir)
143+
144 launchpad = Launchpad.login_with(
145 'ubuntu-archive-tools', opts.launchpad_instance, version='devel')
146
147@@ -204,15 +283,18 @@
148 ppa = launchpad.people['canonical-kernel-team'].getPPAByName(
149 distribution=ubuntu, name='ppa')
150
151- context = {'archive': archive, 'ppa': ppa, 'ubuntu': ubuntu}
152- main_dir = os.getcwd()
153+ start_dir = os.getcwd()
154+ context = {
155+ 'archive': archive, 'ppa': ppa, 'ubuntu': ubuntu,
156+ 'tardir': tardir, 'tarcache': opts.caching, 'startdir': start_dir }
157 for bugnum in bugs:
158 with ExitStack() as resources:
159- cwd = mkdtemp(prefix='kernel-sru-{}-'.format(bugnum), dir=main_dir)
160+ cwd = mkdtemp(prefix='kernel-sru-%s-' % bugnum, dir=start_dir)
161 if not opts.keep_files:
162 resources.callback(shutil.rmtree, cwd)
163 os.chdir(cwd)
164+ context['workdir'] = cwd
165 process_sru_bug(
166 launchpad, bugnum, review_task_callback,
167 review_source_callback, context)
168- os.chdir(main_dir)
169+ os.chdir(start_dir)

Subscribers

People subscribed via source and target branches