Merge ~ubuntu-core-dev/ubuntu/+source/apport:ubuntu/devel-core-dump-handler into ~ubuntu-core-dev/ubuntu/+source/apport:ubuntu/devel

[Benjamin Drung]

[Simon Chopin (schopin)]

LGTM except for the circular deps thing, which should really not be happening.

review: Needs Fixing

diff --git a/debian/apport-core-dump-handler.install b/debian/apport-core-dump-handler.install
new file mode 100644
index 0000000..124c9fd
--- /dev/null
+++ b/debian/apport-core-dump-handler.install
@@ -0,0 +1,2 @@
+debian/tmp/etc/init.d/apport
+usr/lib/systemd/system/apport.service
diff --git a/debian/apport.init b/debian/apport.init
deleted file mode 120000
index 4c697d9..0000000
--- a/debian/apport.init
+++ /dev/null
@@ -1 +0,0 @@
-../etc/init.d/apport
\ No newline at end of file
diff --git a/debian/apport.install b/debian/apport.install
index da993f8..00e96b0 100644
--- a/debian/apport.install
+++ b/debian/apport.install
@@ -4,8 +4,9 @@ debian/apport-autoreport.path /lib/systemd/system
 debian/apport-autoreport.service /lib/systemd/system
 debian/apport-autoreport.timer /lib/systemd/system
 debian/package-hooks usr/share/apport
-debian/tmp/etc
-lib/systemd/
+debian/tmp/etc/apport
+debian/tmp/etc/cron.daily
+debian/tmp/etc/default
 lib/udev/
 usr/bin/apport-bug
 usr/bin/apport-cli
@@ -13,6 +14,7 @@ usr/bin/apport-collect
 usr/bin/apport-unpack
 usr/bin/oem-getlogs
 usr/lib/pm-utils
+usr/lib/systemd/system/apport-forward*
 usr/lib/tmpfiles.d/
 usr/share/apport/apport
 usr/share/apport/apport-checkreports
diff --git a/debian/changelog b/debian/changelog
index 8c290b6..e4e5854 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,18 @@
+apport (2.27.0-0ubuntu7) noble; urgency=medium
+
+  [ Benjamin Drung ]
+  * Rely on pybuild in dh_auto_* targets
+  * Introduce the separate apport-core-dump-handler package that registers as
+    kernel crash dump handler. This is needed for the upcoming systemd-coredump
+    support.
+  * Move systemd units from /lib to /usr/lib
+
+  [ Simon Chopin ]
+  * Rework apport-retrace to handle unbound crashid (LP: #2051512)
+  * fix: use context manager when manipulating GzipFiles (LP: #2051512)
+
+ -- Benjamin Drung <[e-mail]>  Wed, 14 Feb 2024 16:51:44 +0100
+
 apport (2.27.0-0ubuntu6) noble; urgency=medium
 
   * Move additional package hooks to debian/package-hooks
diff --git a/debian/control b/debian/control
index 630358f..d4c43fc 100644
--- a/debian/control
+++ b/debian/control
@@ -39,6 +39,7 @@ Homepage: https://wiki.ubuntu.com/Apport
 Package: apport
 Architecture: all
 Depends:
+ apport-core-dump-handler,
  gir1.2-glib-2.0 (>= 1.29.17),
  python3,
  python3-apport (>= ${source:Version}),
@@ -47,10 +48,8 @@ Depends:
  ${misc:Depends},
 Recommends: apport-symptoms, python3-systemd
 Suggests: apport-gtk | apport-kde, pkexec, polkitd
-Replaces: core-dump-handler, python-apport (<< 2.2-0ubuntu1)
+Replaces: python-apport (<< 2.2-0ubuntu1)
 Breaks: python-apport (<< 2.2-0ubuntu1)
-Conflicts: core-dump-handler
-Provides: core-dump-handler
 Pre-Depends: ${misc:Pre-Depends}
 Description: automatically generate crash reports for debugging
  apport automatically collects data from crashed processes and
@@ -96,6 +95,22 @@ Description: Python 3 library for Apport crash report handling
   * Various frontend utility functions.
   * Python hook to generate crash reports when Python scripts fail.
 
+Package: apport-core-dump-handler
+Architecture: all
+Depends: apport (>= 2.27.0-0ubuntu7~), ${misc:Depends}
+Breaks: apport (<< 2.27.0-0ubuntu7~)
+Replaces: apport (<< 2.27.0-0ubuntu7~), core-dump-handler
+Conflicts: core-dump-handler
+Provides: core-dump-handler
+Pre-Depends: ${misc:Pre-Depends}
+Description: Kernel core dump handler for Apport
+ apport automatically collects data from crashed processes and
+ compiles a problem report in /var/crash/. This utilizes the crashdump
+ helper hook provided by the Ubuntu kernel.
+ .
+ This package provides the service that configures the kernel (via
+ /proc/sys/kernel/core_pattern) to forward the crash dumps to apport.
+
 Package: apport-retrace
 Section: devel
 Architecture: all
diff --git a/debian/patches/apport-retrace-split-out-report-loading-into-its-own-func.patch b/debian/patches/apport-retrace-split-out-report-loading-into-its-own-func.patch
new file mode 100644
index 0000000..c9e8529
--- /dev/null
+++ b/debian/patches/apport-retrace-split-out-report-loading-into-its-own-func.patch
@@ -0,0 +1,132 @@
+From: Simon Chopin <[e-mail]>
+Date: Tue, 13 Feb 2024 19:23:14 +0100
+Subject: apport-retrace: split out report loading into its own function
+
+This reduces complexity in main() while also addressing the unbound crashid
+issue that showed up in Noble, albeit only partially: now it's not
+unbound, but it can be None, which is another can of worms.
+
+Baby steps!
+
+Origin: upstream, https://github.com/canonical/apport/pull/295
+---
+ bin/apport-retrace | 55 ++++++++++++++++++++++++++++++++----------------------
+ 1 file changed, 33 insertions(+), 22 deletions(-)
+
+diff --git a/bin/apport-retrace b/bin/apport-retrace
+index f8082c6..246a065 100755
+--- a/bin/apport-retrace
++++ b/bin/apport-retrace
+@@ -26,12 +26,14 @@ import tempfile
+ import termios
+ import tty
+ import zlib
++from argparse import Namespace
+ from gettext import gettext as _
+ 
+ import apport
+ import apport.fileutils
+ import apport.sandboxutils
+-from apport.crashdb import get_crashdb
++from apport import Report
++from apport.crashdb import CrashDatabase, get_crashdb
+ 
+ 
+ def parse_args():
+@@ -323,20 +325,10 @@ def print_traces(report):
+         print(report["StacktraceSource"])
+ 
+ 
+-# pylint: disable-next=missing-function-docstring
+-def main():
+-    # TODO: Split into smaller functions/methods
+-    # pylint: disable=too-many-branches,too-many-locals,too-many-nested-blocks
+-    # pylint: disable=too-many-statements
+-    apport.memdbg("start")
+-
+-    gettext.textdomain("apport")
+-
+-    options = parse_args()
+-
+-    crashdb = get_crashdb(options.auth)
+-    apport.memdbg("got crash DB")
+-
++def load_report(
++    options: Namespace, crashdb: CrashDatabase
++) -> tuple[Report, int | None]:
++    """Load the initial report for the given CLI options"""
+     # load the report
+     if os.path.exists(options.report):
+         try:
+@@ -344,13 +336,17 @@ def main():
+             with open(options.report, "rb") as f:
+                 report.load(f, binary="compressed")
+             apport.memdbg("loaded report from file")
++            return report, None
+         except (MemoryError, TypeError, ValueError, OSError, zlib.error) as error:
+             apport.fatal("Cannot open report file: %s", str(error))
+     elif options.report.isdigit():
+         # crash ID
+         try:
+-            report = crashdb.download(int(options.report))
++            crashid = int(options.report)
++            report = crashdb.download(crashid)
+             apport.memdbg("downloaded report from crash DB")
++            options.report = None
++            return report, crashid
+         except AssertionError as error:
+             if "apport format data" in str(error):
+                 apport.error("Broken report: %s", str(error))
+@@ -389,14 +385,29 @@ Thank you for your understanding, and sorry for the inconvenience!
+                 sys.exit(0)
+             else:
+                 raise
+-
+-        crashid = options.report
+-        options.report = None
+     else:
+         apport.fatal(
+             '"%s" is neither an existing report file nor a crash ID', options.report
+         )
+ 
++
++# pylint: disable-next=missing-function-docstring
++def main():
++    # TODO: Split into smaller functions/methods
++    # pylint: disable=too-many-branches,too-many-locals,too-many-nested-blocks
++    # pylint: disable=too-many-statements
++    apport.memdbg("start")
++
++    gettext.textdomain("apport")
++
++    options = parse_args()
++
++    crashdb = get_crashdb(options.auth)
++    apport.memdbg("got crash DB")
++
++    # load the report
++    report, crashid = load_report(options, crashdb)
++
+     if options.core_file:
+         report["CoreDump"] = (os.path.abspath(options.core_file),)
+     if options.executable:
+@@ -619,7 +630,7 @@ Thank you for your understanding, and sorry for the inconvenience!
+                 update_bug = True
+                 if options.duplicate_db:
+                     crashdb.init_duplicate_db(options.duplicate_db)
+-                    res = crashdb.check_duplicate(int(crashid), report)
++                    res = crashdb.check_duplicate(crashid, report)
+                     if res:
+                         if res[1] is None:
+                             version = "not fixed yet"
+@@ -639,8 +650,8 @@ Thank you for your understanding, and sorry for the inconvenience!
+                     if "Stacktrace" in report:
+                         crashdb.update_traces(crashid, report)
+                         apport.log(
+-                            "New attachments uploaded to crash database LP: #"
+-                            + crashid,
++                            f"New attachments uploaded to crash database"
++                            f" LP: #{crashid}",
+                             options.timestamps,
+                         )
+                     else:
diff --git a/debian/patches/fix-use-context-manager-when-manipulating-GzipFiles-LP-20.patch b/debian/patches/fix-use-context-manager-when-manipulating-GzipFiles-LP-20.patch
new file mode 100644
index 0000000..9945554
--- /dev/null
+++ b/debian/patches/fix-use-context-manager-when-manipulating-GzipFiles-LP-20.patch
@@ -0,0 +1,109 @@
+From: Simon Chopin <[e-mail]>
+Date: Fri, 9 Feb 2024 11:25:13 +0100
+Subject: fix: use context manager when manipulating GzipFiles (LP: #2051512)
+
+Those objects are technically file-like objects, with the associated
+semantics. In particular, starting with Python 3.12 the class using
+internal buffering, which means write() calls won't necessarily directly
+be reflected in the underlying storage object.
+
+For the sake of consistency, the context manager approach has been
+generalized to read() calls as well, except when we could get away with
+removing the use of GzipFile altogether.
+
+Origin: upstream, https://github.com/canonical/apport/pull/294
+---
+ apport/crashdb_impl/launchpad.py         |  3 ++-
+ problem_report.py                        | 32 ++++++++++++++++----------------
+ tests/integration/test_problem_report.py |  3 ++-
+ 3 files changed, 20 insertions(+), 18 deletions(-)
+
+diff --git a/apport/crashdb_impl/launchpad.py b/apport/crashdb_impl/launchpad.py
+index 6d7e8b4..9fb44fb 100644
+--- a/apport/crashdb_impl/launchpad.py
++++ b/apport/crashdb_impl/launchpad.py
+@@ -369,7 +369,8 @@ class CrashDatabase(apport.crashdb.CrashDatabase):
+                     pass
+             elif ext == ".gz":
+                 try:
+-                    report[key] = gzip.GzipFile(fileobj=attachment).read()
++                    with gzip.GzipFile(fileobj=attachment) as gz:
++                        report[key] = gz.read()
+                 except OSError as error:
+                     # some attachments are only called .gz, but are
+                     # uncompressed (LP #574360)
+diff --git a/problem_report.py b/problem_report.py
+index 9b39a3b..5f65aad 100644
+--- a/problem_report.py
++++ b/problem_report.py
+@@ -65,7 +65,8 @@ class CompressedValue:
+     def set_value(self, value):
+         """Set uncompressed value."""
+         out = io.BytesIO()
+-        gzip.GzipFile(self.name, mode="wb", fileobj=out, mtime=0).write(value)
++        with gzip.GzipFile(self.name, mode="wb", fileobj=out, mtime=0) as gz:
++            gz.write(value)
+         self.gzipvalue = out.getvalue()
+         self.legacy_zlib = False
+ 
+@@ -76,7 +77,7 @@ class CompressedValue:
+ 
+         if self.legacy_zlib:
+             return zlib.decompress(self.gzipvalue)
+-        return gzip.GzipFile(fileobj=io.BytesIO(self.gzipvalue)).read()
++        return gzip.decompress(self.gzipvalue)
+ 
+     def write(self, file):
+         """Write uncompressed value into given file-like object."""
+@@ -86,12 +87,12 @@ class CompressedValue:
+             file.write(zlib.decompress(self.gzipvalue))
+             return
+ 
+-        gz = gzip.GzipFile(fileobj=io.BytesIO(self.gzipvalue))
+-        while True:
+-            block = gz.read(1048576)
+-            if not block:
+-                break
+-            file.write(block)
++        with gzip.GzipFile(fileobj=io.BytesIO(self.gzipvalue)) as gz:
++            while True:
++                block = gz.read(1048576)
++                if not block:
++                    break
++                file.write(block)
+ 
+     def __len__(self):
+         """Return length of uncompressed value."""
+@@ -635,14 +636,13 @@ class ProblemReport(collections.UserDict):
+                     attach_value = f.read()
+                 else:
+                     out = io.BytesIO()
+-                    gf = gzip.GzipFile(k, mode="wb", fileobj=out, mtime=0)
+-                    while True:
+-                        block = f.read(1048576)
+-                        if block:
+-                            gf.write(block)
+-                        else:
+-                            gf.close()
+-                            break
++                    with gzip.GzipFile(k, mode="wb", fileobj=out, mtime=0) as gz:
++                        while True:
++                            block = f.read(1048576)
++                            if block:
++                                gz.write(block)
++                            else:
++                                break
+                     attach_value = out.getvalue()
+                 f.close()
+ 
+diff --git a/tests/integration/test_problem_report.py b/tests/integration/test_problem_report.py
+index 7f705dd..f020021 100644
+--- a/tests/integration/test_problem_report.py
++++ b/tests/integration/test_problem_report.py
+@@ -537,4 +537,5 @@ class T(unittest.TestCase):
+         with tempfile.TemporaryFile() as payload:
+             payload.write(message.get_payload(decode=True))
+             payload.seek(0)
+-            return gzip.GzipFile(mode="rb", fileobj=payload).read()
++            with gzip.GzipFile(mode="rb", fileobj=payload) as gz:
++                return gz.read()
diff --git a/debian/patches/retrace-directly-test-crashid-relevance.patch b/debian/patches/retrace-directly-test-crashid-relevance.patch
new file mode 100644
index 0000000..92df430
--- /dev/null
+++ b/debian/patches/retrace-directly-test-crashid-relevance.patch
@@ -0,0 +1,34 @@
+From: Simon Chopin <[e-mail]>
+Date: Tue, 13 Feb 2024 20:05:37 +0100
+Subject: retrace: directly test crashid relevance
+
+As it turns out testing for `options.report` being falsy meant that
+crashid was defined, but I'd rather test it directly as it's a bit
+cumbersome to trace (and it made pylint crazy).
+
+Origin: upstream, https://github.com/canonical/apport/pull/295
+---
+ bin/apport-retrace | 3 +--
+ 1 file changed, 1 insertion(+), 2 deletions(-)
+
+diff --git a/bin/apport-retrace b/bin/apport-retrace
+index 246a065..f12f8da 100755
+--- a/bin/apport-retrace
++++ b/bin/apport-retrace
+@@ -345,7 +345,6 @@ def load_report(
+             crashid = int(options.report)
+             report = crashdb.download(crashid)
+             apport.memdbg("downloaded report from crash DB")
+-            options.report = None
+             return report, crashid
+         except AssertionError as error:
+             if "apport format data" in str(error):
+@@ -619,7 +618,7 @@ Thank you for your understanding, and sorry for the inconvenience!
+             modified = True
+ 
+     if modified:
+-        if not options.report and not options.output:
++        if crashid is not None and not options.output:
+             if not options.auth:
+                 apport.fatal(
+                     "You need to specify --auth for uploading retraced results"
diff --git a/debian/patches/series b/debian/patches/series
index 1c3393f..e03ab4b 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -11,3 +11,6 @@ setup-replace-distutils.log-by-logging-library.patch
 setup-drop-DistUtilsExtra-version-check.patch
 setup-explicitly-list-packages-to-install.patch
 setup-port-build_java-to-setuptools.patch
+apport-retrace-split-out-report-loading-into-its-own-func.patch
+retrace-directly-test-crashid-relevance.patch
+fix-use-context-manager-when-manipulating-GzipFiles-LP-20.patch
diff --git a/debian/rules b/debian/rules
index 0acdb99..b38b9fd 100755
--- a/debian/rules
+++ b/debian/rules
@@ -3,21 +3,6 @@
 %:
 	dh "$@" --with python3,translations --buildsystem pybuild
 
-# needs manual commands for Python 3, see Debian #597105
-override_dh_auto_clean:
-	dh_auto_clean
-	rm -rf build
-
-override_dh_auto_build:
-	dh_auto_build
-	python3 setup.py build
-
-override_dh_auto_install:
-	dh_auto_install
-	set -ex; for python in $(shell py3versions -r); do \
-	    $$python setup.py install --root=$(CURDIR)/debian/tmp --install-layout=deb; \
-	done
-
 override_dh_auto_test:
 ifeq (, $(findstring nocheck, $(DEB_BUILD_OPTIONS)))
 	tests/run-linters --errors-only
@@ -25,7 +10,7 @@ ifeq (, $(findstring nocheck, $(DEB_BUILD_OPTIONS)))
 endif
 
 override_dh_installinit:
-	dh_installinit --error-handler=true
+	dh_installinit -papport-core-dump-handler --name=apport --error-handler=true --only-scripts
 
 override_dh_install:
 	dh_install -X.pyc -X.egg-info