Merge lp:~wgrant/launchpad/bug-680889-arch-wildcards into lp:launchpad

[William Grant]

[r=henninge][ui=none][bug=680889] Complex architecture hint strings involving architecture wildcards (e.g. "all linux-any") are now handled correctly.

As reported in bug #680889, lp.soyuz.pas.determineArchitecturesToBuild fails to properly handle valid architecture hint strings such as "all linux-any", creating no builds at all. This branch adjusts the "all", "any", and "linux-any" special cases to match even when they are not the sole component of the string.

It's a pretty simple change, except that I had to move the "all" check to the end, so hint strings like "all amd64" create an amd64 build, not an i386 arch-indep one.

I extended the tests to cover the specific problem reported and a few other edge cases.

[Henning Eggers (henninge)]

Hi William,
thanks for this fix. The code looks good to me with my limited domain knowledge. ;-) I am worried about the doc test growing out of proportion, though, as it covers more corner cases. I would think these should be tested in a unit test while the doctest just shows the general operation of the code. Could you do something along those lines, please?
Cheers,
Henning

review: Needs Fixing (code)

[Henning Eggers (henninge)]

Thanks, great work, enormous improvement!

review: Approve (code)

=== modified file 'lib/lp/soyuz/doc/package-arch-specific.txt'
--- lib/lp/soyuz/doc/package-arch-specific.txt	2010-10-14 18:42:19 +0000
+++ lib/lp/soyuz/doc/package-arch-specific.txt	2010-12-08 13:37:07 +0000
@@ -26,43 +26,10 @@
     >>> ignore = test_publisher.setUpDefaultDistroSeries(hoary)
     >>> test_publisher.addFakeChroots()
 
-Create an architecture-independent source.
-
-    >>> pub_all = test_publisher.getPubSource(
-    ...     sourcename='test-buildd', version='667')
-
-Create an architecture-specific source.
-
-    >>> pub_any = test_publisher.getPubSource(
-    ...     sourcename='test-buildd-2', version='668',
-    ...     architecturehintlist="any")
-
-Create a source with a specific list of architectures to be built.
-
     >>> pub_three = test_publisher.getPubSource(
     ...     sourcename='test-buildd-3', version='669',
     ...     architecturehintlist="i386 hppa amd64")
 
-Create a source with one single specific architecture to be built.
-
-    >>> pub_one = test_publisher.getPubSource(
-    ...     sourcename='test-buildd-4', version='670',
-    ...     architecturehintlist="hppa")
-
-Create sources with 'any-$(ARCH)', 'linux-$(ARCH)' and 'linux-any' notation.
-
-    >>> pub_any_foo = test_publisher.getPubSource(
-    ...     sourcename='test-buildd-6', version='672',
-    ...     architecturehintlist="any-hppa")
-
-    >>> pub_linux_foo = test_publisher.getPubSource(
-    ...     sourcename='test-buildd-7', version='673',
-    ...     architecturehintlist="linux-i386")
-
-    >>> pub_linux_any = test_publisher.getPubSource(
-    ...     sourcename='test-buildd-7', version='674',
-    ...     architecturehintlist="linux-any")
-
 Create a PPA source publication.
 
     >>> from lp.registry.interfaces.person import IPersonSet
@@ -107,89 +74,6 @@
     ...         pub, legal_archs, hoary, pas_verify)
     ...     print_architectures(allowed_architectures)
 
-    >>> print_build_architectures(pub_all)
-    i386
-
-    >>> print_build_architectures(pub_any)
-    hppa
-    i386
-
-    >>> print_build_architectures(pub_three)
-    hppa
-    i386
-
-    >>> print_build_architectures(pub_one)
-    hppa
-
-If an architecture is disabled for some reason, then the results from
-determineArchitecturesToBuild() will not include it.
-
-    >>> hoary['hppa'].enabled = False
-
-    >>> print_build_architectures(pub_three)
-    i386
-
-Re-enable it before continuing:
-    >>> hoary['hppa'].enabled = True
-
-
-== Check support for kernel notation in architecture hint list ==
-
-dpkg supports the use of the '<kernel-><arch>' notation in the
-'Architecture' field, where 'kernel-' is one of the kernel platform
-available in debian systems: ['linux-', 'kfreebsd', 'hurd-', 'any-']
-
-See bug 73761 and http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=268709
-for further information about this dpkg feature.
-
-Soyuz will consider only 'any-' and 'linux-' elements as the
-corresponding architecture, since only linux is supported in Ubuntu.
-
-    >>> print_build_architectures(pub_any_foo)
-    hppa
-
-    >>> print_build_architectures(pub_linux_foo)
-    i386
-
-    >>> print_build_architectures(pub_linux_any)
-    hppa
-    i386
-
-
-== PPA architectures ==
-
-For virtualized archives only architectures with established support for
-PPA will be considered when creating PPA builds. See distroarchseries.txt
-for further information.
-
-    >>> ppa_architectures = [
-    ...     arch for arch in hoary.architectures if arch.supports_virtualized]
-    >>> print_architectures(ppa_architectures)
-    i386
-
-    >>> print_build_architectures(pub_ppa)
-    i386
-
-Conversely, for non-virtual archives, build records are created for all
-architectures supported for the distroseries.
-
-    >>> LaunchpadZopelessLayer.switchDbUser("launchpad")
-    >>> pub_ppa.archive.require_virtualized = False
-    >>> commit()
-    >>> LaunchpadZopelessLayer.switchDbUser(test_dbuser)
-
-    >>> print_build_architectures(pub_ppa)
-    hppa
-    i386
-
-Revert the PPA changes.
-
-    >>> LaunchpadZopelessLayer.switchDbUser("launchpad")
-    >>> pub_ppa.archive.require_virtualized = True
-    >>> commit()
-    >>> LaunchpadZopelessLayer.switchDbUser(test_dbuser)
-
-
 === Source PAS ===
 
 Source PAS lines are the ones starting with '%' followed by the source

=== modified file 'lib/lp/soyuz/pas.py'
--- lib/lp/soyuz/pas.py	2010-10-06 11:46:51 +0000
+++ lib/lp/soyuz/pas.py	2010-12-08 13:37:07 +0000
@@ -167,23 +167,30 @@
     legal_arch_tags = set(
         arch.architecturetag for arch in legal_archseries if arch.enabled)
 
-    # We need to support arch tags like any-foo and linux-foo, so remove
-    # supported kernel prefixes, Also allow linux-any but not any-any.
-    # See bugs #73761 and #605002.
-    if hint_string in ['any', 'linux-any']:
+    hint_archs = set(hint_string.split())
+
+    # If a *-any architecture wildcard is present, build for everything
+    # we can. We only support Linux-based architectures at the moment,
+    # and any-any isn't a valid wildcard. See bug #605002.
+    if hint_archs.intersection(('any', 'linux-any')):
         package_tags = legal_arch_tags
-    elif hint_string == 'all':
-        nominated_arch = distroseries.nominatedarchindep
-        legal_archseries_ids = [arch.id for arch in legal_archseries]
-        assert nominated_arch.id in legal_archseries_ids, (
-            'nominatedarchindep is not present in legal_archseries: %s' %
-            ' '.join(legal_arch_tags))
-        package_tags = set([nominated_arch.architecturetag])
     else:
-        my_archs = hint_string.split()
-        for kernel in ['linux', 'any']:
-            my_archs = [arch.replace("%s-" % kernel, "") for arch in my_archs]
-        package_tags = set(my_archs).intersection(legal_arch_tags)
+        # We need to support arch tags like any-foo and linux-foo, so remove
+        # supported kernel prefixes. See bug #73761.
+        stripped_archs = hint_archs
+        for kernel in ('linux', 'any'):
+            stripped_archs = set(
+                arch.replace("%s-" % kernel, "") for arch in stripped_archs)
+        package_tags = stripped_archs.intersection(legal_arch_tags)
+
+        # 'all' is only used as a last resort, to create an arch-indep
+        # build where no builds would otherwise exist.
+        if len(package_tags) == 0 and 'all' in hint_archs:
+            nominated_arch = distroseries.nominatedarchindep
+            assert nominated_arch in legal_archseries, (
+                'nominatedarchindep is not present in legal_archseries: %s' %
+                ' '.join(legal_arch_tags))
+            package_tags = set([nominated_arch.architecturetag])
 
     if pas_verify:
         build_tags = set()

=== added file 'lib/lp/soyuz/tests/test_pas.py'
--- lib/lp/soyuz/tests/test_pas.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/tests/test_pas.py	2010-12-08 13:37:07 +0000
@@ -0,0 +1,99 @@
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from canonical.testing.layers import LaunchpadZopelessLayer
+from lp.soyuz.pas import determineArchitecturesToBuild
+from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
+from lp.testing import TestCaseWithFactory
+
+
+class TestDetermineArchitecturesToBuild(TestCaseWithFactory):
+    """Test that determineArchitecturesToBuild correctly interprets hints."""
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestDetermineArchitecturesToBuild, self).setUp()
+        self.publisher = SoyuzTestPublisher()
+        self.publisher.prepareBreezyAutotest()
+        self.publisher.addFakeChroots()
+
+    def assertArchsForHint(self, hint_string, expected_arch_tags):
+        """Assert that the given hint resolves to the expected archtags."""
+        pub = self.publisher.getPubSource(architecturehintlist=hint_string)
+        architectures = determineArchitecturesToBuild(
+            pub, self.publisher.breezy_autotest.architectures,
+            self.publisher.breezy_autotest)
+        self.assertEqual(
+            expected_arch_tags, [a.architecturetag for a in architectures])
+
+    def test_single_architecture(self):
+        # A hint string with a single arch resolves to just that arch.
+        self.assertArchsForHint('hppa', ['hppa'])
+
+    def test_three_architectures(self):
+        # A hint string with multiple archs resolves to just those
+        # archs.
+        self.assertArchsForHint('amd64 i386 hppa', ['hppa', 'i386'])
+
+    def test_independent(self):
+        # 'all' is special, meaning just a single build. The
+        # nominatedarchindep architecture is used -- in this case i386.
+        self.assertArchsForHint('all', ['i386'])
+
+    def test_one_and_independent(self):
+        # 'all' is redundant if we have another build anyway.
+        self.assertArchsForHint('hppa all', ['hppa'])
+
+    def test_fictional_and_independent(self):
+        # But 'all' is useful if present with an arch that wouldn't
+        # generate a build.
+        self.assertArchsForHint('foo all', ['i386'])
+
+    def test_wildcard(self):
+        # 'any' is a wildcard that matches all available archs.
+        self.assertArchsForHint('any', ['hppa', 'i386'])
+
+    def test_kernel_specific_architecture(self):
+        # Since we only support Linux-based architectures, 'linux-foo'
+        # is treated the same as 'foo'.
+        self.assertArchsForHint('linux-hppa', ['hppa'])
+
+    def test_unknown_kernel_specific_architecture(self):
+        # Non-Linux architectures aren't supported.
+        self.assertArchsForHint('kfreebsd-hppa', [])
+
+    def test_kernel_wildcard_architecture(self):
+        # Wildcards work for kernels: 'any-foo' is treated like 'foo'.
+        self.assertArchsForHint('any-hppa', ['hppa'])
+
+    def test_kernel_specific_architecture_wildcard(self):
+        # Wildcards work for archs too: 'linux-any' is treated like 'any'.
+        self.assertArchsForHint('linux-any', ['hppa', 'i386'])
+
+    def test_unknown_kernel_specific_architecture_wildcard(self):
+        # But unknown kernels continue to result in nothing.
+        self.assertArchsForHint('kfreebsd-any', [])
+
+    def test_wildcard_and_independent(self):
+        # 'all' continues to be ignored alongside a valid wildcard.
+        self.assertArchsForHint('all linux-any', ['hppa', 'i386'])
+
+    def test_kernel_independent_is_invalid(self):
+        # 'linux-all' isn't supported.
+        self.assertArchsForHint('linux-all', [])
+
+    def test_double_wildcard_is_invalid(self):
+        # 'any-any' is invalid; you want 'any'.
+        self.assertArchsForHint('any-any', [])
+
+    def test_disabled_architectures_omitted(self):
+        # Disabled architectures are not buildable, so are excluded.
+        self.publisher.breezy_autotest['hppa'].enabled = False
+        self.assertArchsForHint('any', ['i386'])
+
+    def test_virtualized_archives_have_only_virtualized_archs(self):
+        # For archives which must build on virtual builders, only
+        # virtual archs are returned.
+        self.publisher.breezy_autotest.main_archive.require_virtualized = True
+        self.assertArchsForHint('any', ['i386'])