← Back to team overview

apport-hackers team mailing list archive

Re: [Merge] lp:~brian-murray/apport/speedier-sandboxes into lp:apport


Review: Needs Information

With this implementation, if apport-retrace is called with --gdb-sandbox, no dbg packages are installed for the extra packages (because extra_packages==gdb); but if called with --gdb-sandbox --extra-packages=foo, both foo-dbg and gdb-dbg will be installed.  This seems inconsistent, but because we're using the install_dbg argument to apport.packaging.install_packages(), we're all or nothing.

I guess what I would ask here is, do we have reason to consider it correct to automatically install the dbg packages along with extra packages?  There may or may not be existing users who rely on this behavior, but *should* they?  Should they not instead provide a complete list of the extra packages that they want installed, including any debug packages?

I would suggest one of two options here:

- if there's no requirement to automatically install dbg packages with extra-packages, just unconditionally pass install_dbg=False.
- if there is a requirement to automatically install dbg packages for extra-packages, just call install_packages() separately for gdb vs. the user-specified extra_packages.  (We already call install_packages() twice currently, a third call should hopefully be ok?)

Diff comments:

> === modified file 'backends/packaging-apt-dpkg.py'
> --- backends/packaging-apt-dpkg.py	2017-08-25 18:59:03 +0000
> +++ backends/packaging-apt-dpkg.py	2018-01-04 00:41:07 +0000
> @@ -884,12 +884,20 @@
>                      conflicts += apt.apt_pkg.parse_depends(candidate.record['Conflicts'])
>                  if 'Replaces' in candidate.record:
>                      conflicts += apt.apt_pkg.parse_depends(candidate.record['Replaces'])
> +
>                  for conflict in conflicts:
> +                    if conflict[0][0] == candidate.package.name:
> +                        continue

plain text: "if we are trying to install a package and it conflicts with itself, ignore the conflict".
So... how did this occur?  That seems to me like a broken package, and not something apport should need to work around.

>                      # apt_pkg.parse_depends needs to handle the or operator,
>                      # but as policy states it is invalid to use that in
>                      # Replaces/Depends, we can safely choose the first value
>                      # here.
>                      conflict = conflict[0]
> +                    # if the conflicting package isn't installed in the
> +                    # sandbox or in the list of packages to install its not a
> +                    # concern.
> +                    if conflict[0] not in packages and conflict[0] not in pkg_versions:
> +                        continue

This seems at odds with the immediately following line: if the conflicted-with name is a virtual package rather than a real package, it will definitely not appear in either packages or pkg_versions, but we should still need to handle it.

Without a log to look at, I don't understand the intent here.

>                      if cache.is_virtual_package(conflict[0]):
>                          try:
>                              providers = virtual_mapping[conflict[0]]

Your team Apport upstream developers is requested to review the proposed merge of lp:~brian-murray/apport/speedier-sandboxes into lp:apport.

Follow ups