Merge lp:~cjwatson/launchpad/improve-dpkg-architecture-cache into lp:launchpad

Proposed by Colin Watson on 2017-10-26
Status: Merged
Merged at revision: 18494
Proposed branch: lp:~cjwatson/launchpad/improve-dpkg-architecture-cache
Merge into: lp:launchpad
Diff against target: 123 lines (+55/-6)
2 files modified
lib/lp/soyuz/adapters/buildarch.py (+10/-5)
lib/lp/soyuz/adapters/tests/test_buildarch.py (+45/-1)
To merge this branch: bzr merge lp:~cjwatson/launchpad/improve-dpkg-architecture-cache
Reviewer Review Type Date Requested Status
William Grant code 2017-10-26 Approve on 2017-10-27
Review via email: mp+332853@code.launchpad.net

Commit message

Improve DpkgArchitectureCache's timeline handling, and speed it up a bit in some cases.

Description of the change

We can only really do so much to make this faster without https://bugs.debian.org/771058, but the nested list comprehension I wrote five years was very confusing for 2017-me to read, so I took the opportunity to make it short-circuit properly once any wildcard has matched for a given architecture.

To post a comment you must log in.
William Grant (wgrant) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/soyuz/adapters/buildarch.py'
2--- lib/lp/soyuz/adapters/buildarch.py 2017-02-18 01:48:31 +0000
3+++ lib/lp/soyuz/adapters/buildarch.py 2017-10-26 11:47:59 +0000
4@@ -29,8 +29,9 @@
5 env = dict(os.environ)
6 env["DEB_HOST_ARCH"] = arch
7 action = timeline.start(
8- "dpkg-architecture", "-i%s" % wildcard,
9- "DEB_HOST_ARCH=%s" % arch)
10+ "dpkg-architecture",
11+ "-i%s DEB_HOST_ARCH=%s" % (wildcard, arch),
12+ allow_nested=True)
13 try:
14 ret = (subprocess.call(command, env=env) == 0)
15 finally:
16@@ -39,9 +40,13 @@
17 return self._matches[(arch, wildcard)]
18
19 def findAllMatches(self, arches, wildcards):
20- return list(sorted(set(
21- arch for arch in arches for wildcard in wildcards
22- if self.match(arch, wildcard))))
23+ matches = set()
24+ for arch in arches:
25+ for wildcard in wildcards:
26+ if self.match(arch, wildcard):
27+ matches.add(arch)
28+ break
29+ return list(sorted(matches))
30
31
32 dpkg_architecture = DpkgArchitectureCache()
33
34=== modified file 'lib/lp/soyuz/adapters/tests/test_buildarch.py'
35--- lib/lp/soyuz/adapters/tests/test_buildarch.py 2015-12-30 23:36:39 +0000
36+++ lib/lp/soyuz/adapters/tests/test_buildarch.py 2017-10-26 11:47:59 +0000
37@@ -1,34 +1,70 @@
38-# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
39+# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
40 # GNU Affero General Public License version 3 (see the file LICENSE).
41
42 __metaclass__ = type
43
44+from testtools.matchers import (
45+ MatchesListwise,
46+ MatchesStructure,
47+ )
48+
49 from lp.soyuz.adapters.buildarch import (
50 determine_architectures_to_build,
51 DpkgArchitectureCache,
52 )
53 from lp.testing import TestCase
54+from lp.testing.fixture import CaptureTimeline
55
56
57 class TestDpkgArchitectureCache(TestCase):
58
59+ def setUp(self):
60+ super(TestDpkgArchitectureCache, self).setUp()
61+ self.timeline = self.useFixture(CaptureTimeline()).timeline
62+
63+ def assertTimeline(self, expected_details):
64+ matchers = []
65+ for expected_detail in expected_details:
66+ matchers.append(MatchesStructure.byEquality(
67+ category='dpkg-architecture-start', detail=expected_detail))
68+ matchers.append(MatchesStructure.byEquality(
69+ category='dpkg-architecture-stop', detail=expected_detail))
70+ self.assertThat(self.timeline.actions, MatchesListwise(matchers))
71+
72 def test_multiple(self):
73 self.assertContentEqual(
74 ['amd64', 'armhf'],
75 DpkgArchitectureCache().findAllMatches(
76 ['amd64', 'i386', 'armhf'], ['amd64', 'armhf']))
77+ self.assertTimeline([
78+ '-iamd64 DEB_HOST_ARCH=amd64',
79+ '-iamd64 DEB_HOST_ARCH=i386',
80+ '-iarmhf DEB_HOST_ARCH=i386',
81+ '-iamd64 DEB_HOST_ARCH=armhf',
82+ '-iarmhf DEB_HOST_ARCH=armhf',
83+ ])
84
85 def test_any(self):
86 self.assertContentEqual(
87 ['amd64', 'i386', 'kfreebsd-amd64'],
88 DpkgArchitectureCache().findAllMatches(
89 ['amd64', 'i386', 'kfreebsd-amd64'], ['any']))
90+ self.assertTimeline([
91+ '-iany DEB_HOST_ARCH=amd64',
92+ '-iany DEB_HOST_ARCH=i386',
93+ '-iany DEB_HOST_ARCH=kfreebsd-amd64',
94+ ])
95
96 def test_all(self):
97 self.assertContentEqual(
98 [],
99 DpkgArchitectureCache().findAllMatches(
100 ['amd64', 'i386', 'kfreebsd-amd64'], ['all']))
101+ self.assertTimeline([
102+ '-iall DEB_HOST_ARCH=amd64',
103+ '-iall DEB_HOST_ARCH=i386',
104+ '-iall DEB_HOST_ARCH=kfreebsd-amd64',
105+ ])
106
107 def test_partial_wildcards(self):
108 self.assertContentEqual(
109@@ -36,6 +72,14 @@
110 DpkgArchitectureCache().findAllMatches(
111 ['amd64', 'i386', 'kfreebsd-amd64', 'kfreebsd-i386'],
112 ['linux-any', 'any-amd64']))
113+ self.assertTimeline([
114+ '-ilinux-any DEB_HOST_ARCH=amd64',
115+ '-ilinux-any DEB_HOST_ARCH=i386',
116+ '-ilinux-any DEB_HOST_ARCH=kfreebsd-amd64',
117+ '-iany-amd64 DEB_HOST_ARCH=kfreebsd-amd64',
118+ '-ilinux-any DEB_HOST_ARCH=kfreebsd-i386',
119+ '-iany-amd64 DEB_HOST_ARCH=kfreebsd-i386',
120+ ])
121
122
123 class TestDetermineArchitecturesToBuild(TestCase):