launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve

Just a couple of nits, but looks fine, thanks.

Diff comments:

> === modified file 'lib/lp/buildmaster/interfaces/processor.py'
> --- lib/lp/buildmaster/interfaces/processor.py	2015-05-14 08:57:55 +0000
> +++ lib/lp/buildmaster/interfaces/processor.py	2015-05-14 08:57:55 +0000
> @@ -71,11 +71,20 @@
>              "Whether this processor is enabled on archives by default.")),
>          as_of='devel', readonly=True)
>      supports_virtualized = exported(
> -        Bool(title=_("Whether this processor supports virtualized builds.")),
> +        Bool(
> +            title=_("Supports virtualized builds"),
> +            description=_(
> +                "Whether the processor has virtualized builders. If not, "
> +                "archives that require virtualized builds won't build on "
> +                "this processor.")),
>          as_of='devel', readonly=True)
>      supports_nonvirtualized = exported(
> -        Bool(title=_(
> -            "Whether this processor supports non-virtualized builds.")),
> +        Bool(
> +            title=_("Supports non-virtualized builds"),
> +            description=_(
> +                "Whether the processor has non-virtualized builders. If not, "
> +                "all builds for this processor will build on virtualized "
> +                "builders, even for non-virtualized archives.")),
>          as_of='devel', readonly=True)
>  
>  
> 
> === modified file 'lib/lp/buildmaster/model/processor.py'
> --- lib/lp/buildmaster/model/processor.py	2015-05-14 08:57:55 +0000
> +++ lib/lp/buildmaster/model/processor.py	2015-05-14 08:57:55 +0000
> @@ -28,8 +28,18 @@
>      title = StringCol(dbName='title', notNull=True)
>      description = StringCol(dbName='description', notNull=True)
>      restricted = Bool(allow_none=False, default=False)
> +
> +    # When setting this to true you may want to add missing
> +    # ArchiveArches.
>      build_by_default = Bool(allow_none=False, default=False)
> +
> +    # This controls build creation, so you may want to create or cancel
> +    # some builds after changing it on an existing processor.
>      supports_virtualized = Bool(allow_none=False, default=False)
> +
> +    # Queued and failed builds' BuildQueue.virtualized and
> +    # BinaryPackageBuild.virtualized may need tweaking if this is
> +    # changed on an existing processor.
>      supports_nonvirtualized = Bool(allow_none=False, default=True)
>  
>      def __repr__(self):
> 
> === modified file 'lib/lp/buildmaster/tests/test_buildqueue.py'
> --- lib/lp/buildmaster/tests/test_buildqueue.py	2014-06-11 08:29:40 +0000
> +++ lib/lp/buildmaster/tests/test_buildqueue.py	2015-05-14 08:57:55 +0000
> @@ -196,7 +196,7 @@
>  
>          # Make sure the 'virtualized' properties are the same.
>          self.assertEqual(
> -            bq.virtualized, build.is_virtualized,
> +            bq.virtualized, build.virtualized,
>              "The 'virtualized' property deviates.")
>  
>  
> 
> === modified file 'lib/lp/buildmaster/tests/test_queuedepth.py'
> --- lib/lp/buildmaster/tests/test_queuedepth.py	2015-04-20 09:48:57 +0000
> +++ lib/lp/buildmaster/tests/test_queuedepth.py	2015-05-14 08:57:55 +0000
> @@ -391,7 +391,7 @@
>          build = self.builds[0]
>          # The build in question is an x86/native one.
>          self.assertEqual(self.x86_proc.id, build.processor.id)
> -        self.assertEqual(False, build.is_virtualized)
> +        self.assertEqual(False, build.virtualized)
>  
>          # To test this non-interface method, we need to remove the
>          # security proxy.
> @@ -403,36 +403,36 @@
>              "The number of native x86 builders is wrong")
>          # Initially all 4 builders are free.
>          free_count = get_free_builders_count(
> -            build.processor, build.is_virtualized)
> +            build.processor, build.virtualized)
>          self.assertEqual(4, free_count)
>          # Once we assign a build to one of them we should see the free
>          # builders count drop by one.
>          assign_to_builder(self, 'postgres', 1)
>          free_count = get_free_builders_count(
> -            build.processor, build.is_virtualized)
> +            build.processor, build.virtualized)
>          self.assertEqual(3, free_count)
>          # When we assign another build to one of them we should see the free
>          # builders count drop by one again.
>          assign_to_builder(self, 'gcc', 2)
>          free_count = get_free_builders_count(
> -            build.processor, build.is_virtualized)
> +            build.processor, build.virtualized)
>          self.assertEqual(2, free_count)
>          # Let's use up another builder.
>          assign_to_builder(self, 'apg', 3)
>          free_count = get_free_builders_count(
> -            build.processor, build.is_virtualized)
> +            build.processor, build.virtualized)
>          self.assertEqual(1, free_count)
>          # And now for the last one.
>          assign_to_builder(self, 'flex', 4)
>          free_count = get_free_builders_count(
> -            build.processor, build.is_virtualized)
> +            build.processor, build.virtualized)
>          self.assertEqual(0, free_count)
>          # If we reset the 'flex' build the builder that was assigned to it
>          # will be free again.
>          build, bq = find_job(self, 'flex')
>          bq.reset()
>          free_count = get_free_builders_count(
> -            build.processor, build.is_virtualized)
> +            build.processor, build.virtualized)
>          self.assertEqual(1, free_count)
>  
>  
> 
> === modified file 'lib/lp/code/interfaces/sourcepackagerecipebuild.py'
> --- lib/lp/code/interfaces/sourcepackagerecipebuild.py	2013-11-21 03:42:38 +0000
> +++ lib/lp/code/interfaces/sourcepackagerecipebuild.py	2015-05-14 08:57:55 +0000
> @@ -67,8 +67,6 @@
>          ISourcePackageRelease, title=_("The produced source package release"),
>          readonly=True)
>  
> -    is_virtualized = Bool(title=_('If True, this build is virtualized.'))
> -
>      def getFileByName(filename):
>          """Return the file under +files with specified name."""
>  
> 
> === modified file 'lib/lp/code/model/recipebuilder.py'
> --- lib/lp/code/model/recipebuilder.py	2014-06-26 11:35:59 +0000
> +++ lib/lp/code/model/recipebuilder.py	2015-05-14 08:57:55 +0000
> @@ -117,7 +117,7 @@
>             distroseries state.
>          """
>          build = self.build
> -        assert not (not self._builder.virtualized and build.is_virtualized), (
> +        assert self._builder.virtualized, (
>              "Attempt to build virtual item on a non-virtual builder.")
>  
>          # This should already have been checked earlier, but just check again
> 
> === modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
> --- lib/lp/code/model/sourcepackagerecipebuild.py	2015-04-30 01:45:30 +0000
> +++ lib/lp/code/model/sourcepackagerecipebuild.py	2015-05-14 08:57:55 +0000
> @@ -118,8 +118,6 @@
>          """See `IPackageBuild`."""
>          return self.distroseries.distribution
>  
> -    is_virtualized = True
> -
>      recipe_id = Int(name='recipe')
>      recipe = Reference(recipe_id, 'SourcePackageRecipe.id')
>  
> 
> === modified file 'lib/lp/soyuz/interfaces/binarypackagebuild.py'
> --- lib/lp/soyuz/interfaces/binarypackagebuild.py	2015-05-12 01:13:19 +0000
> +++ lib/lp/soyuz/interfaces/binarypackagebuild.py	2015-05-14 08:57:55 +0000
> @@ -134,9 +134,6 @@
>              description=_(
>                  "Whether or not this build record can be cancelled.")))
>  
> -    is_virtualized = Attribute(
> -        "Whether or not this build requires a virtual build host or not.")
> -
>      upload_changesfile = Attribute(
>          "The `LibraryFileAlias` object containing the changes file which "
>          "was originally uploaded with the results of this build. It's "
> 
> === modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
> --- lib/lp/soyuz/model/binarypackagebuild.py	2015-05-14 08:57:55 +0000
> +++ lib/lp/soyuz/model/binarypackagebuild.py	2015-05-14 08:57:55 +0000
> @@ -297,9 +297,9 @@
>          return DecoratedResultSet(results, itemgetter(0)).one()
>  
>      @property
> -    def is_virtualized(self):
> +    def can_be_nonvirtualized(self):
>          """See `IBuild`"""
> -        return self.archive.require_virtualized
> +        return not self.archive.require_virtualized

You don't use this new method name anywhere, and it isn't in the interface, so perhaps you can delete it entirely.

>  
>      @property
>      def title(self):
> @@ -918,14 +918,17 @@
>          build_farm_job = getUtility(IBuildFarmJobSource).new(
>              BinaryPackageBuild.job_type, status, date_created, builder,
>              archive)
> +        processor = distro_arch_series.processor
> +        virtualized = (
> +            archive.require_virtualized
> +            or not processor.supports_nonvirtualized)
>          return BinaryPackageBuild(
>              build_farm_job=build_farm_job,
>              distro_arch_series=distro_arch_series,
>              source_package_release=source_package_release,
>              archive=archive, pocket=pocket, arch_indep=arch_indep,
> -            status=status, processor=distro_arch_series.processor,
> -            virtualized=archive.require_virtualized, builder=builder,
> -            is_distro_archive=archive.is_main,
> +            status=status, processor=processor, virtualized=virtualized,
> +            builder=builder, is_distro_archive=archive.is_main,
>              distribution=distro_arch_series.distroseries.distribution,
>              distro_series=distro_arch_series.distroseries,
>              source_package_name=source_package_release.sourcepackagename,
> 
> === modified file 'lib/lp/soyuz/model/binarypackagebuildbehaviour.py'
> --- lib/lp/soyuz/model/binarypackagebuildbehaviour.py	2014-11-03 07:19:10 +0000
> +++ lib/lp/soyuz/model/binarypackagebuildbehaviour.py	2015-05-14 08:57:55 +0000
> @@ -97,9 +97,9 @@
>             distroseries state.
>          """
>          build = self.build
> -        if build.is_virtualized and not self._builder.virtualized:
> +        if build.archive.require_virtualized and not self._builder.virtualized:
>              raise AssertionError(
> -                "Attempt to build virtual item on a non-virtual builder.")
> +                "Attempt to build virtual archive on a non-virtual builder.")
>  
>          # Assert that we are not silently building SECURITY jobs.
>          # See findBuildCandidates. Once we start building SECURITY
> 
> === modified file 'lib/lp/soyuz/model/livefsbuild.py'
> --- lib/lp/soyuz/model/livefsbuild.py	2014-06-17 11:01:51 +0000
> +++ lib/lp/soyuz/model/livefsbuild.py	2015-05-14 08:57:55 +0000
> @@ -349,7 +349,8 @@
>          livefsbuild = LiveFSBuild(
>              build_farm_job, requester, livefs, archive, distro_arch_series,
>              pocket, distro_arch_series.processor,
> -            livefs.require_virtualized or archive.require_virtualized,
> +            not distro_arch_series.processor.supports_nonvirtualized
> +            or livefs.require_virtualized or archive.require_virtualized,
>              unique_key, metadata_override, date_created)
>          store.add(livefsbuild)
>          return livefsbuild
> 
> === modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
> --- lib/lp/soyuz/tests/test_binarypackagebuild.py	2015-05-12 01:13:19 +0000
> +++ lib/lp/soyuz/tests/test_binarypackagebuild.py	2015-05-14 08:57:55 +0000
> @@ -78,7 +78,7 @@
>          self.assertEqual(
>              self.build.build_farm_job, removeSecurityProxy(bq)._build_farm_job)
>          self.assertEqual(self.build, bq.specific_build)
> -        self.assertEqual(self.build.is_virtualized, bq.virtualized)
> +        self.assertEqual(self.build.virtualized, bq.virtualized)
>          self.assertIsNotNone(bq.processor)
>          self.assertEqual(bq, self.build.buildqueue_record)
>  
> 
> === modified file 'lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py'
> --- lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py	2014-11-03 07:19:10 +0000
> +++ lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py	2015-05-14 08:57:55 +0000
> @@ -317,7 +317,7 @@
>          e = self.assertRaises(
>              AssertionError, behaviour.verifyBuildRequest, BufferLogger())
>          self.assertEqual(
> -            'Attempt to build virtual item on a non-virtual builder.',
> +            'Attempt to build virtual archive on a non-virtual builder.',
>              str(e))
>  
>      def test_verifyBuildRequest_no_chroot(self):
> 
> === modified file 'lib/lp/soyuz/tests/test_build.py'
> --- lib/lp/soyuz/tests/test_build.py	2015-05-14 08:57:55 +0000
> +++ lib/lp/soyuz/tests/test_build.py	2015-05-14 08:57:55 +0000
> @@ -87,7 +87,7 @@
>          self.assertEquals(self.das, build.distro_arch_series)
>          self.assertEquals(PackagePublishingPocket.RELEASE, build.pocket)
>          self.assertEquals(self.das.architecturetag, build.arch_tag)
> -        self.assertTrue(build.is_virtualized)
> +        self.assertTrue(build.virtualized)
>          self.assertEquals(
>              '%s - %s' % (spph.source_package_name,
>                  spph.source_package_version),
> 
> === modified file 'lib/lp/soyuz/tests/test_build_set.py'
> --- lib/lp/soyuz/tests/test_build_set.py	2015-05-14 08:57:55 +0000
> +++ lib/lp/soyuz/tests/test_build_set.py	2015-05-14 08:57:55 +0000
> @@ -92,6 +92,36 @@
>                      b.buildqueue_record.destroySelf()
>              self.builds += builds
>  
> +    def test_new_virtualization(self):

I dislike describing things as "new" when they're going to stay around for years and will eventually not be new.  (I once lived in a part of college called "New Court" which was built in 1825.)

> +        # Builds are virtualized unless Processor.support_nonvirtualized
> +        # and not Archive.require_virtualized.
> +
> +        def make(proc_virt, proc_nonvirt, archive_virt):
> +            proc = self.factory.makeProcessor(
> +                supports_nonvirtualized=proc_nonvirt,
> +                supports_virtualized=proc_virt)
> +            das = self.factory.makeDistroArchSeries(processor=proc)
> +            archive = self.factory.makeArchive(
> +                distribution=das.distroseries.distribution,
> +                virtualized=archive_virt)
> +            bpb = getUtility(IBinaryPackageBuildSet).new(
> +                self.factory.makeSourcePackageRelease(),
> +                archive, das, PackagePublishingPocket.RELEASE)
> +            self.assertEqual(proc, bpb.processor)
> +            return bpb
> +
> +        vvvbpb = make(proc_virt=True, proc_nonvirt=True, archive_virt=True)
> +        self.assertTrue(vvvbpb.virtualized)
> +
> +        vvnbpb = make(proc_virt=True, proc_nonvirt=True, archive_virt=False)
> +        self.assertFalse(vvnbpb.virtualized)
> +
> +        vnvbpb = make(proc_virt=True, proc_nonvirt=False, archive_virt=True)
> +        self.assertTrue(vnvbpb.virtualized)
> +
> +        vnvbpb = make(proc_virt=True, proc_nonvirt=False, archive_virt=False)
> +        self.assertTrue(vnvbpb.virtualized)
> +
>      def test_get_for_distro_distribution(self):
>          # Test fetching builds for a distro's main archives
>          self.setUpBuilds()
> 
> === modified file 'lib/lp/soyuz/tests/test_livefs.py'
> --- lib/lp/soyuz/tests/test_livefs.py	2015-05-07 10:52:10 +0000
> +++ lib/lp/soyuz/tests/test_livefs.py	2015-05-14 08:57:55 +0000
> @@ -193,15 +193,21 @@
>              PackagePublishingPocket.RELEASE)
>  
>      def test_requestBuild_virtualization(self):
> -        # New builds are virtualized if either the livefs or the archive
> -        # requires it.
> -        distroarchseries = self.factory.makeDistroArchSeries()
> -        for livefs_virt, archive_virt, build_virt in (
> -                (False, False, False),
> -                (False, True, True),
> -                (True, False, True),
> -                (True, True, True),
> +        # New builds are virtualized if any of the processor, livefs or
> +        # archive require it.
> +        for proc_nonvirt, livefs_virt, archive_virt, build_virt in (
> +                (True, False, False, False),
> +                (True, False, True, True),
> +                (True, True, False, True),
> +                (True, True, True, True),
> +                (False, False, False, True),
> +                (False, False, True, True),
> +                (False, True, False, True),
> +                (False, True, True, True),
>                  ):
> +            distroarchseries = self.factory.makeDistroArchSeries(
> +                processor=self.factory.makeProcessor(
> +                    supports_nonvirtualized=proc_nonvirt))
>              livefs = self.factory.makeLiveFS(
>                  distroseries=distroarchseries.distroseries,
>                  require_virtualized=livefs_virt)
> 


-- 
https://code.launchpad.net/~wgrant/launchpad/processor-nonvirt/+merge/259088
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.