launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/services/config/schema-lazr.conf'
> --- lib/lp/services/config/schema-lazr.conf	2015-11-17 01:39:17 +0000
> +++ lib/lp/services/config/schema-lazr.conf	2015-11-20 18:05:58 +0000
> @@ -541,6 +541,10 @@
>  # The maximum number of lines to format using the format_diff tal formatter.
>  max_format_lines: 5000
>  
> +# The timeout in seconds for a debdiff between two packages.
> +# datatype: integer
> +debdiff_time_limit: 600

I'd put this in a new package_diff section.

> +
>  
>  [distributionmirrorprober]
>  # The database user which will be used by this process.
> 
> === modified file 'lib/lp/soyuz/model/sourcepackagerelease.py'
> --- lib/lp/soyuz/model/sourcepackagerelease.py	2015-07-08 16:05:11 +0000
> +++ lib/lp/soyuz/model/sourcepackagerelease.py	2015-11-20 18:05:58 +0000
> @@ -395,21 +394,12 @@
>              raise PackageDiffAlreadyRequested(
>                  "%s has already been requested" % candidate.title)
>  
> -        if self.sourcepackagename.name == 'udev':
> -            # XXX 2009-11-23 Julian bug=314436
> -            # Currently diff output for udev will fill disks.  It's
> -            # disabled until diffutils is fixed in that bug.
> -            status = PackageDiffStatus.FAILED
> -        else:
> -            status = PackageDiffStatus.PENDING

It'll still block the queue for ten minutes. We might want to continue avoiding that.

> -
>          Store.of(to_sourcepackagerelease).flush()
>          del get_property_cache(to_sourcepackagerelease).package_diffs
>          packagediff = PackageDiff(
>              from_source=self, to_source=to_sourcepackagerelease,
> -            requester=requester, status=status)
> -        if status == PackageDiffStatus.PENDING:
> -            getUtility(IPackageDiffJobSource).create(packagediff)
> +            requester=requester)
> +        getUtility(IPackageDiffJobSource).create(packagediff)
>          return packagediff
>  
>      def aggregate_changelog(self, since_version):
> 
> === modified file 'lib/lp/soyuz/tests/test_packagediff.py'
> --- lib/lp/soyuz/tests/test_packagediff.py	2013-07-31 00:37:32 +0000
> +++ lib/lp/soyuz/tests/test_packagediff.py	2015-11-20 18:05:58 +0000
> @@ -156,3 +159,18 @@
>          [job] = IStore(Job).find(
>              Job, Job.base_job_type == JobType.GENERATE_PACKAGE_DIFF)
>          self.assertIsNot(None, job)
> +
> +    def test_packagediff_time_limit(self):
> +        # debdiff is killed after the time limit expires.
> +        self.pushConfig("diff", debdiff_time_limit=1)
> +        temp_dir = self.makeTemporaryDirectory()
> +        mock_debdiff_path = os.path.join(temp_dir, "debdiff")
> +        with open(mock_debdiff_path, "w") as mock_debdiff:
> +            print("#! /bin/sh", file=mock_debdiff)
> +            print("sleep 5", file=mock_debdiff)
> +        os.chmod(mock_debdiff_path, 0o755)
> +        mock_path = "%s:%s" % (temp_dir, os.environ["PATH"])
> +        diff = create_proper_job(self.factory)
> +        with EnvironmentVariableFixture("PATH", mock_path):
> +            diff.performDiff()
> +        self.assertEqual(PackageDiffStatus.FAILED, diff.status)

Can you check that the process is dead and the temporary directory gone?



-- 
https://code.launchpad.net/~cjwatson/launchpad/limit-debdiff/+merge/278187
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.