[Merge] ~xnox/germinate/+git/builtusing2:master into germinate:master

[Colin Watson]

Review: Needs Fixing

This looks broadly OK, but I've commented on a few things I'd like to see fixed before we land this.

Diff comments:

> diff --git a/germinate/germinator.py b/germinate/germinator.py
> index a86c25d..8d50608 100644
> --- a/germinate/germinator.py
> +++ b/germinate/germinator.py
> @@ -86,7 +86,6 @@ class RecommendsReason(object):
>      def __str__(self):
>          return '%s (Recommends)' % self._pkg
>  
> -

Revert this removal - it makes flake8 sad.

>  class DependsReason(object):
>      def __init__(self, pkg):
>          self._pkg = pkg
> @@ -110,6 +109,14 @@ class RescueReason(object):
>      def __str__(self):
>          return 'Rescued from %s' % self._src
>  
> +class RescueBuiltUsingReason(object):
> +    def __init__(self, src, reverse_bu):
> +        self._src = src
> +        self._reverse_bu = reverse_bu
> +
> +    def __str__(self):
> +        return 'Rescued from %s (Reverse Built-Using %s)' % (
> +            self._src, self._reverse_bu)

Maybe 'Rescued from %s: %s (Built-Using)', which is a bit more in line with things like RecommendsReason.

>  
>  class SeedKernelVersions(object):
>      """A virtual seed entry representing lexically scoped Kernel-Version."""
> @@ -1667,8 +1676,13 @@ class Germinator(object):
>                      seed._build_depends.add(pkg)
>                  else:
>                      seed._depends.add(pkg)
> -                self._add_package(seed, pkg, RescueReason(src),
> -                                  build_tree=build_tree)
> +                # add pkg, which came from src
> +                # it is rescued

Could we have comments as full sentences?

> +                reason = RescueReason(src)
> +                if len(self._sources[src]["Reverse-Built-Using"]):
> +                    reverse_bu = ', '.join(self._sources[src]["Reverse-Built-Using"])

Please either sort this list so that we get deterministic output in tests and the like, or arrange for the data structure you're using to be something that maintains insertion order.  (I don't mind either way.)

I'm also a bit concerned that this could get very long and make the table formatting awful.  Because of this, Germinator generally only records one reason.  In that spirit, perhaps it would be better to record only the first item in the list here (but still after ordering, as above)?

> +                    reason = RescueBuiltUsingReason(src, reverse_bu)

It would be nice if this also prompted a debug log message like the one not far above, I think.

> +                self._add_package(seed, pkg, reason, build_tree=build_tree)
>  
>      # Accessors.
>      # ----------
> diff --git a/germinate/tests/test_germinator.py b/germinate/tests/test_germinator.py
> index 79956dc..306c9e8 100644
> --- a/germinate/tests/test_germinator.py
> +++ b/germinate/tests/test_germinator.py
> @@ -161,6 +161,7 @@ class TestGerminator(TestCase):
>              "Build-Depends-Indep": [],
>              "Build-Depends-Arch": [],
>              "Binaries": ["hello", "hello-dependency"],
> +            "Reverse-Built-Using": set(),
>              }, germinator._sources["hello"])
>          self.assertIn("hello", germinator._packages)
>          self.assertEqual({

Would an actual test be possible as well?  I know the test suite isn't great at the moment, but it's only likely to get better if we try to add something at least when we make substantial changes to the code under test.



-- 
https://code.launchpad.net/~xnox/germinate/+git/builtusing2/+merge/308743
Your team Ubuntu Package Archive Administrators is requested to review the proposed merge of ~xnox/germinate/+git/builtusing2:master into germinate:master.