launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/improve-dpkg-architecture-cache into lp:launchpad.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/improve-dpkg-architecture-cache/+merge/332853

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.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/improve-dpkg-architecture-cache into lp:launchpad.

=== modified file 'lib/lp/soyuz/adapters/buildarch.py'
--- lib/lp/soyuz/adapters/buildarch.py	2017-02-18 01:48:31 +0000
+++ lib/lp/soyuz/adapters/buildarch.py	2017-10-26 11:47:59 +0000
@@ -29,8 +29,9 @@
             env = dict(os.environ)
             env["DEB_HOST_ARCH"] = arch
             action = timeline.start(
-                "dpkg-architecture", "-i%s" % wildcard,
-                "DEB_HOST_ARCH=%s" % arch)
+                "dpkg-architecture",
+                "-i%s DEB_HOST_ARCH=%s" % (wildcard, arch),
+                allow_nested=True)
             try:
                 ret = (subprocess.call(command, env=env) == 0)
             finally:
@@ -39,9 +40,13 @@
         return self._matches[(arch, wildcard)]
 
     def findAllMatches(self, arches, wildcards):
-        return list(sorted(set(
-            arch for arch in arches for wildcard in wildcards
-            if self.match(arch, wildcard))))
+        matches = set()
+        for arch in arches:
+            for wildcard in wildcards:
+                if self.match(arch, wildcard):
+                    matches.add(arch)
+                    break
+        return list(sorted(matches))
 
 
 dpkg_architecture = DpkgArchitectureCache()

=== modified file 'lib/lp/soyuz/adapters/tests/test_buildarch.py'
--- lib/lp/soyuz/adapters/tests/test_buildarch.py	2015-12-30 23:36:39 +0000
+++ lib/lp/soyuz/adapters/tests/test_buildarch.py	2017-10-26 11:47:59 +0000
@@ -1,34 +1,70 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
 
+from testtools.matchers import (
+    MatchesListwise,
+    MatchesStructure,
+    )
+
 from lp.soyuz.adapters.buildarch import (
     determine_architectures_to_build,
     DpkgArchitectureCache,
     )
 from lp.testing import TestCase
+from lp.testing.fixture import CaptureTimeline
 
 
 class TestDpkgArchitectureCache(TestCase):
 
+    def setUp(self):
+        super(TestDpkgArchitectureCache, self).setUp()
+        self.timeline = self.useFixture(CaptureTimeline()).timeline
+
+    def assertTimeline(self, expected_details):
+        matchers = []
+        for expected_detail in expected_details:
+            matchers.append(MatchesStructure.byEquality(
+                category='dpkg-architecture-start', detail=expected_detail))
+            matchers.append(MatchesStructure.byEquality(
+                category='dpkg-architecture-stop', detail=expected_detail))
+        self.assertThat(self.timeline.actions, MatchesListwise(matchers))
+
     def test_multiple(self):
         self.assertContentEqual(
             ['amd64', 'armhf'],
             DpkgArchitectureCache().findAllMatches(
                 ['amd64', 'i386', 'armhf'], ['amd64', 'armhf']))
+        self.assertTimeline([
+            '-iamd64 DEB_HOST_ARCH=amd64',
+            '-iamd64 DEB_HOST_ARCH=i386',
+            '-iarmhf DEB_HOST_ARCH=i386',
+            '-iamd64 DEB_HOST_ARCH=armhf',
+            '-iarmhf DEB_HOST_ARCH=armhf',
+            ])
 
     def test_any(self):
         self.assertContentEqual(
             ['amd64', 'i386', 'kfreebsd-amd64'],
             DpkgArchitectureCache().findAllMatches(
                 ['amd64', 'i386', 'kfreebsd-amd64'], ['any']))
+        self.assertTimeline([
+            '-iany DEB_HOST_ARCH=amd64',
+            '-iany DEB_HOST_ARCH=i386',
+            '-iany DEB_HOST_ARCH=kfreebsd-amd64',
+            ])
 
     def test_all(self):
         self.assertContentEqual(
             [],
             DpkgArchitectureCache().findAllMatches(
                 ['amd64', 'i386', 'kfreebsd-amd64'], ['all']))
+        self.assertTimeline([
+            '-iall DEB_HOST_ARCH=amd64',
+            '-iall DEB_HOST_ARCH=i386',
+            '-iall DEB_HOST_ARCH=kfreebsd-amd64',
+            ])
 
     def test_partial_wildcards(self):
         self.assertContentEqual(
@@ -36,6 +72,14 @@
             DpkgArchitectureCache().findAllMatches(
                 ['amd64', 'i386', 'kfreebsd-amd64', 'kfreebsd-i386'],
                 ['linux-any', 'any-amd64']))
+        self.assertTimeline([
+            '-ilinux-any DEB_HOST_ARCH=amd64',
+            '-ilinux-any DEB_HOST_ARCH=i386',
+            '-ilinux-any DEB_HOST_ARCH=kfreebsd-amd64',
+            '-iany-amd64 DEB_HOST_ARCH=kfreebsd-amd64',
+            '-ilinux-any DEB_HOST_ARCH=kfreebsd-i386',
+            '-iany-amd64 DEB_HOST_ARCH=kfreebsd-i386',
+            ])
 
 
 class TestDetermineArchitecturesToBuild(TestCase):