launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/soyuz/adapters/archivedependencies.py'
> --- lib/lp/soyuz/adapters/archivedependencies.py	2016-01-07 18:34:07 +0000
> +++ lib/lp/soyuz/adapters/archivedependencies.py	2016-03-30 21:24:29 +0000
> @@ -166,15 +165,14 @@
>          # the component where the source is published in the primary
>          # archive.
>          if archive_dependency.component is None:
> -            components = component_dependencies[primary_component]
> +            archive_component = primary_component
>          else:
> -            components = component_dependencies[
> -                archive_dependency.component.name]
> +            archive_component = archive_dependency.component
>          # Follow pocket dependencies.
>          for pocket in pocket_dependencies[archive_dependency.pocket]:
>              deps.append(
>                  (archive_dependency.dependency, distro_arch_series, pocket,
> -                 components))
> +                 get_components_for_context(archive_component, pocket)))

This behaves really strangely when ArchiveDependency.pocket == BACKPORTS and ArchiveDependency.component is not None. sources.list will contain all components for backports, but only classically relevant components for the other pockets.

It might make more sense to use archive_dependency.pocket here.

>  
>      # Consider build tools archive dependencies.
>      if tools_source is not None:
> @@ -205,11 +203,11 @@
>              dep_arch_series = dsp.parent_series.getDistroArchSeries(
>                  distro_arch_series.architecturetag)
>              dep_archive = dsp.parent_series.distribution.main_archive
> -            components = component_dependencies[dsp.component.name]
>              # Follow pocket dependencies.
>              for pocket in pocket_dependencies[dsp.pocket]:
>                  deps.append(
> -                    (dep_archive, dep_arch_series, pocket, components))
> +                    (dep_archive, dep_arch_series, pocket,
> +                     get_components_for_context(dsp.component, pocket)))

See above.

>          except NotFoundError:
>              pass
>  
> @@ -353,13 +351,11 @@
>          archive.
>      """
>      if archive.purpose in ALLOW_RELEASE_BUILDS:
> -        primary_pockets = pocket_dependencies[
> -            default_pocket_dependency]
> -        primary_components = component_dependencies[
> +        component = getUtility(IComponentSet)[
>              default_component_dependency_name]
> -    else:
> -        primary_pockets = pocket_dependencies[pocket]
> -        primary_components = get_components_for_context(component, pocket)
> +        pocket = default_pocket_dependency
> +    primary_components = get_components_for_context(component, pocket)
> +    primary_pockets = pocket_dependencies[pocket]

This already uses the behaviour I recommended above: the component set is always calculated from the weakest pocket.

>  
>      primary_dependencies = []
>      for pocket in primary_pockets:
> 
> === modified file 'lib/lp/soyuz/model/archivedependency.py'
> --- lib/lp/soyuz/model/archivedependency.py	2015-07-08 16:05:11 +0000
> +++ lib/lp/soyuz/model/archivedependency.py	2016-03-30 21:24:29 +0000
> @@ -63,6 +63,6 @@
>              return pocket_title
>  
>          component_part = ", ".join(
> -            component_dependencies[self.component.name])
> +            get_components_for_context(self.component, self.pocket))

This also already matches the weakest pocket behaviour, but differs from the existing code.

>  
>          return "%s (%s)" % (pocket_title, component_part)


-- 
https://code.launchpad.net/~cjwatson/launchpad/refactor-component-dependencies/+merge/290532
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.