Merge ~mitchburton/ubuntu/+source/landscape-client:2027613-2040189-service-start-fix-mantic into ubuntu/+source/landscape-client:ubuntu/noble-devel

[Mitch Burton]

fix: landscape-config does not start landscape-client service (LP: #2040189)
fix: avoid stopping services on upgrade (LP: #2027613)

[Andreas Hasenack (ahasenack)]

Thanks for adding the tests!

Comments inline about packaging practices. Besides those, it would help to have separate commits. Given the changes, I would expect three commits:
- one for the d/rules fix
- one for the added patch (so d/p/0001-.... and d/p/series)
- one for the d/changelog entry

In that way, I can easily compare the commits across different ubuntu releases using git range-diff.

review: Needs Fixing

[Andreas Hasenack (ahasenack)]

I'll refrain from reviewing the other branches, since the changes would be the same. Let's get noble in good shape, apply the same review comments to the other branches, and then we can proceed.

[Mitch Burton (mitchburton)]

Thanks for the comments. I've split the commit into three, as suggested, and addressed the inline comments in this revision.

[Andreas Hasenack (ahasenack)]

To re-enable the proper test suite, this is what I had to do:
Author: Andreas Hasenack <email address hidden>
Date: Wed Nov 8 14:30:55 2023 +0000

      * Re-enable build-time test suite (LP: #XXXXXX):
        - d/rules: use correct test command
        - d/control: build-depend on python3-pycurl which is used by the
          test suite

diff --git a/debian/control b/debian/control
index ec0471c..6c42510 100644
--- a/debian/control
+++ b/debian/control
@@ -6,7 +6,8 @@ XSBC-Original-Maintainer: Landscape Team <email address hidden>
 Build-Depends: debhelper-compat (= 12), po-debconf, libdistro-info-perl,
                dh-python, python3-dev, python3-distutils-extra,
                lsb-release, gawk, net-tools,
- python3-apt, python3-twisted, python3-configobj
+ python3-apt, python3-twisted, python3-configobj,
+ python3-pycurl
 Standards-Version: 4.4.0
 Homepage: https://github.com/CanonicalLtd/landscape-client

diff --git a/debian/rules b/debian/rules
index d6a156c..f41183a 100755
--- a/debian/rules
+++ b/debian/rules
@@ -24,3 +24,6 @@ override_dh_auto_install:

 override_dh_installsystemd:
        dh_installsystemd
+
+override_dh_auto_test:
+ make check3

There is a "check" makefile target already, but it calls out to check2 and check3. check2 is for python2, and doesn't work anymore. You might want to:
- remove check2 from upstream (if my reading of this is correct)
- maybe change setup.py to run the proper test suite with trial. If this is done, then I think we don't even need to change d/rules, as the dh_auto_test debhelper will likely do the right thing

[Andreas Hasenack (ahasenack)]

Hi Mitch, did you have a chance to look at my last comment yet?

[Mitch Burton (mitchburton)]

I've updated d/rules and d/changelog to enable the test suite. Do I need to create a corresponding LP bug?

Wasn't able to change setup.py to run the proper test suite, as setuptools appears to have deprecated the `test` command - they recommend using tox.

I've also created a PR to upstream to clean up the Makefile there, and after that's merged will also contribute the patch to get the test suite running on build.

[Andreas Hasenack (ahasenack)]

> I've updated d/rules and d/changelog to enable the test suite. Do I need to create a corresponding
> LP bug?

Yes please, and then updated the d/changelog entry about this with the LP bug number.

> Wasn't able to change setup.py to run the proper test suite, as setuptools appears to have deprecated
> the `test` command - they recommend using tox.

Ok

> I've also created a PR to upstream to clean up the Makefile there, and after that's merged will also
> contribute the patch to get the test suite running on build.

Cool

[Andreas Hasenack (ahasenack)]

I had to add a few extra build-dependencies, to cope with "module not found" errors in the test suite:

python3-netifaces
python3-yaml
ubuntu-advantage-tools

But the tests are now failing:
https://launchpadlibrarian.net/698874496/buildlog_ubuntu-noble-amd64.landscape-client_23.08-0ubuntu2~ppa4_BUILDING.txt.gz

Many of the errors are happening because HOME=/sbuild-nonexistent, and that directory really doesn't exist. I checked older build logs, before we enabled the test suite, and they also had this HOME setting.

Errors like:
FileNotFoundError: [Errno 2] No such file or directory: '/sbuild-nonexistent/.cache/ubuntu-pro'

This comes from within ubuntu-advantage-client. Have you seen this before? I suspect the workaround will be to set HOME to a valid path, but was wondering if you have seen this before in other test environments.

[Mitch Burton (mitchburton)]

Re: sbuild-nonexistent. I haven't seen this before in other test environments. Generally HOME is set to an existing directory. I agree that setting HOME to a valid path should work. What's the cleanest way to do that? Something like:

override_dh_auto_test:
    HOME=$(shell pwd) make check3

[Andreas Hasenack (ahasenack)]

I just tried this:

@@ -26,4 +26,4 @@ override_dh_installsystemd:
        dh_installsystemd

 override_dh_auto_test:
- make check3
+ HOME=$(shell mktemp -d) make check3

It moved much further[1], but now is failing elsewhere:

Traceback (most recent call last):
  File "/<<PKGBUILDDIR>>/landscape/client/manager/tests/test_scriptexecution.py", line 466, in test_user
    return self._run_script(username, uid, gid, path)
  File "/<<PKGBUILDDIR>>/landscape/client/manager/tests/test_scriptexecution.py", line 433, in _run_script
    self.assertEqual(spawn[4], path)
  File "/usr/lib/python3/dist-packages/twisted/trial/_synctest.py", line 422, in assertEqual
    super().assertEqual(first, second, msg)
  File "/usr/lib/python3.11/unittest/case.py", line 873, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python3.11/unittest/case.py", line 1253, in assertMultiLineEqual
    self.fail(self._formatMessage(msg, standardMsg))
twisted.trial.unittest.FailTest: '/' != '/nonexistent'
- /
+ /nonexistent

and

Unable to list installed snaps: (7, "Failed to connect to localhost port 80 after 0 ms: Couldn't connect to server")

and one other type.

Maybe we need to export HOME.

1. https://launchpadlibrarian.net/698892672/buildlog_ubuntu-noble-amd64.landscape-client_23.08-0ubuntu2~ppa6_BUILDING.txt.gz

[Mitch Burton (mitchburton)]

Okay I see three categories of errors still:

  1. the one you mentioned above - I think exporting HOME will probably help, as this is a subprocess execution being tested
  2. communications with snapd's REST API. I take it the build environment does not have snapd? May need to have these tests conditionally skipped or mock the snapd side of things in all tests. It's mocked in one of them, which passes.
  3. locale.setlocale(locale.LC_CTYPE, ("C", "UTF-8")) is failing, probably because this locale hasn't been generated. Is it alright to locale-gen ahead of running the tests? Do we need that as a build-depends?

[Andreas Hasenack (ahasenack)]

1. Trying exporting HOME:

 override_dh_auto_test:
- make check3
+ export HOME=$(shell mktemp -d) && make check3

Log: https://launchpadlibrarian.net/698896308/buildlog_ubuntu-noble-amd64.landscape-client_23.08-0ubuntu2~ppa8_BUILDING.txt.gz

In the last run, the locale and snap errors remain, but also this one:

[FAIL]
Traceback (most recent call last):
  File "/<<PKGBUILDDIR>>/landscape/client/manager/tests/test_scriptexecution.py", line 466, in test_user
    return self._run_script(username, uid, gid, path)
  File "/<<PKGBUILDDIR>>/landscape/client/manager/tests/test_scriptexecution.py", line 433, in _run_script
    self.assertEqual(spawn[4], path)
  File "/usr/lib/python3/dist-packages/twisted/trial/_synctest.py", line 422, in assertEqual
    super().assertEqual(first, second, msg)
  File "/usr/lib/python3.11/unittest/case.py", line 873, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python3.11/unittest/case.py", line 1253, in assertMultiLineEqual
    self.fail(self._formatMessage(msg, standardMsg))
twisted.trial.unittest.FailTest: '/' != '/nonexistent'
- /
+ /nonexistent

This might still be about HOME.

2. I suspect snapd is not installed. If we need it for tests, we can build-depend on it, but to be honest I don't know if the test environment will restrict snapd somehow. We can try. Do you have to install it in the github runners?

3. I suppose it's allright to locale-gen before the test, but will that really fix it? I suspect it's another missing build-depends. Are these new tests? If not, we could go back to an older landscape-client and check what the build-depends was back then. But if this test is new, then it makes sense that a build-depends was missed.

Could you take it from here please? :) Use a PPA for these iterations, just like I was doing here.

[Mitch Burton (mitchburton)]

I'll look further into the HOME issue.

2. snapd does not have to be installed in the ubuntu github runners. It's already in there. I'd be interested in trying it as a build dependency.

3. Change and accompanying test are 3 1/2 years old: https://github.com/canonical/landscape-client/commit/25aed2015e3da424a185c1caaf38bada9705a9f4
   I can try an old revision and see.

Yup, I'll take it from here.

[Mitch Burton (mitchburton)]

Looks like for the above test, if the user's home directory (in this case /nonexistent) doesn't exist, the code falls back to /. I'll need to produce an additional patch to fix this, though it will be a very small one.

[Mitch Burton (mitchburton)]

Including locales-all as a build dependency solved some of the errors. Including snapd did not solve the snapd-related issues.

https://launchpadlibrarian.net/698912164/buildlog_ubuntu-noble-amd64.landscape-client_23.08-0ubuntu2~ppa2_BUILDING.txt.gz

So it looks like snapd interaction is a bust, generally. I will have to patch the associated tests to mock it or be skipped.

https://launchpadlibrarian.net/698915314/buildlog_ubuntu-noble-amd64.landscape-client_23.08-0ubuntu2~ppa4_BUILDING.txt.gz

error: cannot list snaps: cannot communicate with server: Get "http://localhost/v2/snaps": dial unix /run/snapd.socket: connect: no such file or directory

[Mitch Burton (mitchburton)]

Pushed version with a passing build. Let me know if the commits or patches need rearranging.

buildlog: https://launchpadlibrarian.net/699090088/buildlog_ubuntu-noble-amd64.landscape-client_23.08-0ubuntu2~ppa6_BUILDING.txt.gz

[Andreas Hasenack (ahasenack)]

Please rebase on top of https://launchpad.net/ubuntu/+source/landscape-client/23.08-0ubuntu2, which was uploaded to noble

[Mitch Burton (mitchburton)]

Rebased on latest commit to noble-devel.

[Andreas Hasenack (ahasenack)]

review: Needs Information

[Andreas Hasenack (ahasenack)]

review: Needs Fixing

[Mitch Burton (mitchburton)]

Pushed latest changes. Summary of what I've changed
- added a revert of the fix for https://bugs.launchpad.net/ubuntu/+source/landscape-client/+bug/2040924
- added a new fix for above, updating d/landscape-sysinfo.wrapper, tested it by executing it as both a regular user and root as per my previous testing for the previous fix
- Added Bug-Ubuntu header items for both patches

re: inline comment about falling back to "/" instead of HOME in patch 0002:
Internally, landscape.lib.user uses pwd (https://docs.python.org/3/library/pwd.html#module-pwd) for script execution to run a script in the run-as user's pw_dir directory, which circumvents HOME. If pw_dir doesn't exist, it defaults to "/".

I'm open to changing that default to something less bad, though. Or maybe first trying to default to HOME, then something else? I was trying to restrict my changes to just test files, though.

[Andreas Hasenack (ahasenack)]

Some comments inline, let me know what you think.,

[Andreas Hasenack (ahasenack)]

Ah, the cp will preserve the permissions if the target file (the cache in this case) already exists, but not if it doesn't, then it will use the permissions of the source file.

[Andreas Hasenack (ahasenack)]

It might be simpler to just make sure the perms are 0644 in the end.

[Mitch Burton (mitchburton)]

Sorry, Launchpad double-posted my previous comments. Also going to ensure perms are 0644.

[Mitch Burton (mitchburton)]

updated landscape-sysinfo.cache to use a shell variable instead of a temp file. This simplifies cleanup. Also changed permissions of the cache to 0644.

[Andreas Hasenack (ahasenack)]

+1

review: Approve

[Andreas Hasenack (ahasenack)]

Sponsored:
Uploading landscape-client_23.08-0ubuntu3.dsc
Uploading landscape-client_23.08-0ubuntu3.debian.tar.xz
Uploading landscape-client_23.08-0ubuntu3_source.buildinfo
Uploading landscape-client_23.08-0ubuntu3_source.changes

diff --git a/debian/changelog b/debian/changelog
index 4eaf917..1e04a38 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,19 @@
+landscape-client (23.08-0ubuntu3) noble; urgency=medium
+
+  * d/p/0001-start-service-during-config.patch: fix landscape-config does not
+    start landscape-client service (LP: #2040189)
+  * d/rules: avoid stopping services on upgrade (LP: #2027613)
+  * Re-enable build-time test suite (LP: #2044181)
+    - d/rules: enable test suite during build
+    - d/control: build-depend on python3-pycurl, python3-netifaces,
+        python3-yaml, ubuntu-advantage-tools, locales-all
+    - d/p/0002-fix-broken-build-tests.patch: add mocks to snapd-related tests;
+        use existing directory if HOME does not exist
+  * d/landscape-sysinfo.wrapper: fix handle using cache when permissions allow
+    (LP: #2040924)
+
+ -- Mitch Burton <[e-mail]>  Tue, 28 Nov 2023 11:40:17 -0800
+
 landscape-client (23.08-0ubuntu2) noble; urgency=medium
 
   * d/landscape-common.postint: ensure sysinfo cache file exists and is globally
diff --git a/debian/control b/debian/control
index ec0471c..c70ceae 100644
--- a/debian/control
+++ b/debian/control
@@ -6,7 +6,9 @@ XSBC-Original-Maintainer: Landscape Team <[e-mail]>
 Build-Depends: debhelper-compat (= 12), po-debconf, libdistro-info-perl,
                dh-python, python3-dev, python3-distutils-extra,
                lsb-release, gawk, net-tools,
-               python3-apt, python3-twisted, python3-configobj
+               python3-apt, python3-twisted, python3-configobj,
+               python3-pycurl, python3-netifaces, python3-yaml,
+               ubuntu-advantage-tools, locales-all
 Standards-Version: 4.4.0
 Homepage: https://github.com/CanonicalLtd/landscape-client
 
diff --git a/debian/landscape-client.service b/debian/landscape-client.service
index 6dc721a..aa5c026 100644
--- a/debian/landscape-client.service
+++ b/debian/landscape-client.service
@@ -11,6 +11,7 @@ WantedBy=multi-user.target
 [Service]
 Type=simple
 Group=landscape
+ExecCondition=/usr/bin/landscape-config --is-registered
 ExecStart=/usr/bin/landscape-client
 # Don't kill cgroup as child dpkg may restart the service during an upgrade.
 KillMode=process
diff --git a/debian/landscape-common.postinst b/debian/landscape-common.postinst
index 9aeb7a5..e78c193 100755
--- a/debian/landscape-common.postinst
+++ b/debian/landscape-common.postinst
@@ -90,11 +90,6 @@ case "$1" in
         WRAPPER=/usr/share/landscape/landscape-sysinfo.wrapper
         PROFILE_LOCATION=/etc/profile.d/50-landscape-sysinfo.sh
         UPDATE_MOTD_LOCATION=/etc/update-motd.d/50-landscape-sysinfo
-        STAMP_LOCATION=/var/lib/landscape/landscape-sysinfo.cache
-
-        touch $STAMP_LOCATION
-        chown landscape:root $STAMP_LOCATION
-        chmod 666 $STAMP_LOCATION
 
         if [ "$RET" = "Cache sysinfo in /etc/motd" ]; then
             rm -f $PROFILE_LOCATION 2>/dev/null || true
diff --git a/debian/landscape-sysinfo.wrapper b/debian/landscape-sysinfo.wrapper
index 54f139d..4460113 100644
--- a/debian/landscape-sysinfo.wrapper
+++ b/debian/landscape-sysinfo.wrapper
@@ -2,28 +2,46 @@
 
 # don't try refresh this more than once per minute
 # Due to cpu consumption and login delays (LP: #1893716)
-stamp="/var/lib/landscape/landscape-sysinfo.cache"
-NEED_UPDATE="FALSE"
-[ -z "$(find "$stamp" -newermt 'now-1 minutes' 2> /dev/null)" ] && NEED_UPDATE="TRUE"
+CACHE="/var/lib/landscape/landscape-sysinfo.cache"
+HAS_CACHE="FALSE"
+CACHE_NEEDS_UPDATE="FALSE"
+
+[ -r "$CACHE" ] && HAS_CACHE="TRUE"
+[ -z "$(find "$CACHE" -newermt 'now-1 minutes' 2> /dev/null)" ] && CACHE_NEEDS_UPDATE="TRUE"
+
+if [ "$HAS_CACHE" = "TRUE" ] && [ "$CACHE_NEEDS_UPDATE" = "FALSE" ]; then
+    cat "$CACHE"
+else
+    SYSINFO=""
 
-if [ "$NEED_UPDATE" = "TRUE" ]; then
     # pam_motd does not carry the environment
     [ -f /etc/default/locale ] && . /etc/default/locale
     export LANG
-    cores=$(grep -c ^processor /proc/cpuinfo 2>/dev/null)
-    [ "$cores" -eq "0" ] && cores=1
-    threshold="${cores:-1}.0"
-    if [ $(echo "`cut -f1 -d ' ' /proc/loadavg` < $threshold" | bc) -eq 1 ]; then
-        printf "\n  System information as of %s\n\n%s\n" \
+    CORES=$(grep -c ^processor /proc/cpuinfo 2>/dev/null)
+    [ "$CORES" -eq "0" ] && CORES=1
+    THRESHOLD="${cores:-1}.0"
+
+    if [ $(echo "`cut -f1 -d ' ' /proc/loadavg` < $THRESHOLD" | bc) -eq 1 ]; then
+        SYSINFO=$(printf "\n System information as of %s\n\n%s\n" \
             "$(/bin/date)" \
-            "$(/usr/bin/landscape-sysinfo)" \
-            > "$stamp"
+            "$(/usr/bin/landscape-sysinfo)")
+        echo "$SYSINFO" 2>/dev/null >"$CACHE" || true
+        chmod 0644 "$CACHE" 2>/dev/null || true
     else
-        # do not replace a formerly good result due to load
-        if ! grep -q "System information as of" $stamp 2> /dev/null; then
-            printf "\n System information disabled due to load higher than %s\n" "$threshold" > "$stamp"
+        SYSINFO=$(printf "\n System information disabled due to load higher than %s\n" "$THRESHOLD")
+
+        if [ "$HAS_CACHE" = "TRUE" ]; then
+            if ! grep -q "System information as of" $CACHE 2>/dev/null; then
+                # do not replace a formerly good result due to load
+                echo "$SYSINFO" 2>/dev/null >"$CACHE" || true
+                chmod 0644 "$CACHE" 2>/dev/null || true
+            else
+                SYSINFO=$(cat "$CACHE")
+            fi
         fi
     fi
+
+    printf "%s\n" "$SYSINFO"
 fi
 
-[ ! -r "$stamp" ] || cat "$stamp"
+exit 0
diff --git a/debian/patches/0001-start-service-during-config.patch b/debian/patches/0001-start-service-during-config.patch
new file mode 100644
index 0000000..d3fb5f9
--- /dev/null
+++ b/debian/patches/0001-start-service-during-config.patch
@@ -0,0 +1,269 @@
+Description: Allow landscape-config to start landscape-client systemd service
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/landscape-client/+bug/2040189
+Author: Mitch Burton <[e-mail]>
+Origin: upstream, https://github.com/canonical/landscape-client/commit/0da6b4b64c7ca50c109279bd42633c537458fcf4
+Reviewed-by: Kevin Nasto <[e-mail]>
+Applied-Upstream: 23.10
+Last-Update: 2023-11-07
+---
+This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
+--- a/landscape/client/configuration.py
++++ b/landscape/client/configuration.py
+@@ -624,8 +624,10 @@
+     decode_base64_ssl_public_certificate(config)
+     config.write()
+     # Restart the client to ensure that it's using the new configuration.
++
+     if not config.no_start:
+         try:
++            set_secure_id(config, "registering")
+             ServiceConfig.restart_landscape()
+         except ServiceConfigException as exc:
+             print_text(str(exc), error=True)
+@@ -796,13 +798,14 @@
+     # Results will be things like "success" or "ssl-error".
+     result = results[0]
+ 
+-    if isinstance(result, SystemExit):
+-        raise result
+-
+     # If there was an error and the caller requested that errors be reported
+     # to the on_error callable, then do so.
+     if result != "success" and on_error is not None:
+         on_error(1)
++
++    if isinstance(result, SystemExit):
++        raise result
++
+     return result
+ 
+ 
+@@ -883,6 +886,21 @@
+     return text
+ 
+ 
++def set_secure_id(config, new_id):
++    """Persists a secure id in the identity data file. This is used to indicate
++    whether we are currently in the process of registering.
++    """
++    persist = Persist(
++        filename=os.path.join(
++            config.data_path,
++            f"{BrokerService.service_name}.bpickle",
++        ),
++    )
++    identity = Identity(config, persist)
++    identity.secure_id = new_id
++    persist.save()
++
++
+ def main(args, print=print):
+     """Interact with the user and the server to set up client configuration."""
+ 
+@@ -927,7 +945,11 @@
+     # Attempt to register the client.
+     reactor = LandscapeReactor()
+     if config.silent:
+-        result = register(config, reactor)
++        result = register(
++            config,
++            reactor,
++            on_error=lambda _: set_secure_id(config, None),
++        )
+         report_registration_outcome(result, print=print)
+         sys.exit(determine_exit_code(result))
+     else:
+@@ -937,6 +959,10 @@
+             default=default_answer,
+         )
+         if answer:
+-            result = register(config, reactor)
++            result = register(
++                config,
++                reactor,
++                on_error=lambda _: set_secure_id(config, None),
++            )
+             report_registration_outcome(result, print=print)
+             sys.exit(determine_exit_code(result))
+--- a/landscape/client/tests/test_configuration.py
++++ b/landscape/client/tests/test_configuration.py
+@@ -34,6 +34,7 @@
+ from landscape.client.configuration import register
+ from landscape.client.configuration import registration_info_text
+ from landscape.client.configuration import report_registration_outcome
++from landscape.client.configuration import set_secure_id
+ from landscape.client.configuration import setup
+ from landscape.client.configuration import show_help
+ from landscape.client.configuration import store_public_key_data
+@@ -738,12 +739,17 @@
+         bootstrap_tree_patcher = mock.patch(
+             "landscape.client.configuration.bootstrap_tree",
+         )
++        set_secure_id_patch = mock.patch(
++            "landscape.client.configuration.set_secure_id",
++        )
+         self.mock_getuid = getuid_patcher.start()
+         self.mock_bootstrap_tree = bootstrap_tree_patcher.start()
++        set_secure_id_patch.start()
+ 
+         def cleanup():
+             getuid_patcher.stop()
+             bootstrap_tree_patcher.stop()
++            set_secure_id_patch.stop()
+ 
+         self.addCleanup(cleanup)
+ 
+@@ -1191,7 +1197,11 @@
+         )
+         self.assertEqual(0, exception.code)
+         mock_setup.assert_called_once_with(mock.ANY)
+-        mock_register.assert_called_once_with(mock.ANY, mock.ANY)
++        mock_register.assert_called_once_with(
++            mock.ANY,
++            mock.ANY,
++            on_error=mock.ANY,
++        )
+ 
+     @mock.patch("landscape.client.configuration.input", return_value="y")
+     @mock.patch(
+@@ -1219,7 +1229,11 @@
+         )
+         self.assertEqual(0, exception.code)
+         mock_setup.assert_called_once_with(mock.ANY)
+-        mock_register.assert_called_once_with(mock.ANY, mock.ANY)
++        mock_register.assert_called_once_with(
++            mock.ANY,
++            mock.ANY,
++            on_error=mock.ANY,
++        )
+         mock_input.assert_called_once_with(
+             "\nRequest a new registration for this computer now? [Y/n]: ",
+         )
+@@ -1256,7 +1270,11 @@
+         )
+         self.assertEqual(2, exception.code)
+         mock_setup.assert_called_once_with(mock.ANY)
+-        mock_register.assert_called_once_with(mock.ANY, mock.ANY)
++        mock_register.assert_called_once_with(
++            mock.ANY,
++            mock.ANY,
++            on_error=mock.ANY,
++        )
+         mock_input.assert_called_once_with(
+             "\nRequest a new registration for this computer now? [Y/n]: ",
+         )
+@@ -1295,7 +1313,11 @@
+         )
+         self.assertEqual(0, exception.code)
+         mock_setup.assert_called_once_with(mock.ANY)
+-        mock_register.assert_called_once_with(mock.ANY, mock.ANY)
++        mock_register.assert_called_once_with(
++            mock.ANY,
++            mock.ANY,
++            on_error=mock.ANY,
++        )
+         mock_input.assert_not_called()
+ 
+         self.assertEqual(
+@@ -1333,7 +1355,11 @@
+         )
+         self.assertEqual(2, exception.code)
+         mock_setup.assert_called_once_with(mock.ANY)
+-        mock_register.assert_called_once_with(mock.ANY, mock.ANY)
++        mock_register.assert_called_once_with(
++            mock.ANY,
++            mock.ANY,
++            on_error=mock.ANY,
++        )
+         mock_input.assert_not_called()
+         # Note that the error is output via sys.stderr.
+         self.assertEqual(
+@@ -1378,7 +1404,11 @@
+         mock_serviceconfig.set_start_on_boot.assert_called_once_with(True)
+         mock_serviceconfig.restart_landscape.assert_called_once_with()
+         mock_setup_script().run.assert_called_once_with()
+-        mock_register.assert_called_once_with(mock.ANY, mock.ANY)
++        mock_register.assert_called_once_with(
++            mock.ANY,
++            mock.ANY,
++            on_error=mock.ANY,
++        )
+         mock_input.assert_called_with(
+             "\nRequest a new registration for this computer now? [Y/n]: ",
+         )
+@@ -1457,7 +1487,11 @@
+             print=noop_print,
+         )
+         mock_setup.assert_called_once_with(mock.ANY)
+-        mock_register.assert_called_once_with(mock.ANY, mock.ANY)
++        mock_register.assert_called_once_with(
++            mock.ANY,
++            mock.ANY,
++            on_error=mock.ANY,
++        )
+         mock_input.assert_called_once_with(
+             "\nRequest a new registration for this computer now? [Y/n]: ",
+         )
+@@ -1477,7 +1511,11 @@
+             print=noop_print,
+         )
+         mock_setup.assert_called_once_with(mock.ANY)
+-        mock_register.assert_called_once_with(mock.ANY, mock.ANY)
++        mock_register.assert_called_once_with(
++            mock.ANY,
++            mock.ANY,
++            on_error=mock.ANY,
++        )
+         mock_input.assert_not_called()
+ 
+     @mock.patch("landscape.client.configuration.input")
+@@ -2290,6 +2328,27 @@
+         # We ask for retries because networks aren't reliable.
+         self.assertEqual(99, connector.max_retries)
+ 
++    def test_register_got_error(self):
++        """If there is an error from the connection, raises `SystemExit`."""
++        reactor = mock.Mock()
++        connector_factory = mock.Mock()
++        results = []
++
++        def add_error():
++            results.append(SystemExit())
++
++        reactor.run.side_effect = add_error
++
++        self.assertRaises(
++            SystemExit,
++            register,
++            self.config,
++            reactor,
++            connector_factory,
++            max_retries=2,
++            results=results,
++        )
++
+     @mock.patch("landscape.client.configuration.LandscapeReactor")
+     def test_register_without_reactor(self, mock_reactor):
+         """If no reactor is passed, a LandscapeReactor will be instantiated.
+@@ -2683,3 +2742,21 @@
+             print=noop_print,
+         )
+         self.assertEqual(EXIT_NOT_REGISTERED, exception.code)
++
++
++class SetSecureIdTest(LandscapeTest):
++    """Tests for the `set_secure_id` function."""
++
++    @mock.patch("landscape.client.configuration.Persist")
++    @mock.patch("landscape.client.configuration.Identity")
++    def test_function(self, Identity, Persist):
++        config = mock.Mock(data_path="/tmp/landscape")
++
++        set_secure_id(config, "fancysecureid")
++
++        Persist.assert_called_once_with(
++            filename="/tmp/landscape/broker.bpickle",
++        )
++        Persist().save.assert_called_once_with()
++        Identity.assert_called_once_with(config, Persist())
++        self.assertEqual(Identity().secure_id, "fancysecureid")
diff --git a/debian/patches/0002-fix-broken-build-tests.patch b/debian/patches/0002-fix-broken-build-tests.patch
new file mode 100644
index 0000000..8a118b3
--- /dev/null
+++ b/debian/patches/0002-fix-broken-build-tests.patch
@@ -0,0 +1,54 @@
+Description: Fix tests that do not pass in debian build environment
+ In environments where the run-as user's home directory does not exist, such
+ as sbuild, fall back to expecting the script path to be the root directory.
+ Mock snapd, as it does not run in an accessible way in some environments.
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/landscape-client/+bug/2044181
+Author: Mitch Burton <[e-mail]>
+Last-Update: 2023-11-22
+---
+This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
+--- a/landscape/client/manager/tests/test_scriptexecution.py
++++ b/landscape/client/manager/tests/test_scriptexecution.py
+@@ -463,6 +463,9 @@
+         gid = info.pw_gid
+         path = info.pw_dir
+ 
++        if not os.path.exists(path):
++            path = "/"
++
+         return self._run_script(username, uid, gid, path)
+ 
+     def test_user_no_home(self):
+--- a/landscape/client/monitor/tests/test_snapmonitor.py
++++ b/landscape/client/monitor/tests/test_snapmonitor.py
+@@ -16,7 +16,12 @@
+ 
+     def test_get_data(self):
+         """Tests getting installed snap data."""
++        snap_http_mock = Mock(
++            spec=SnapHttp,
++            get_snaps=Mock(return_value={"result": []}),
++        )
+         plugin = SnapMonitor()
++        plugin._snap_http = snap_http_mock
+         self.monitor.add(plugin)
+ 
+         plugin.exchange()
+--- a/landscape/client/snap/tests/test_http.py
++++ b/landscape/client/snap/tests/test_http.py
+@@ -19,6 +19,15 @@
+     def test_get_snaps(self):
+         """get_snaps() returns a dict with a list of installed snaps."""
+         http = SnapHttp()
++
++        def fill_buff(curl, buff, **kwargs):
++            buff.write(
++                b'{"result": [{"id": "foo", "name": "bar", '
++                b'"publisher": "baz"}]}'
++            )
++
++        http._perform = Mock(side_effect=fill_buff)
++
+         result = http.get_snaps()["result"]
+ 
+         self.assertTrue(isinstance(result, list))
diff --git a/debian/patches/series b/debian/patches/series
index e69de29..ee1509d 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -0,0 +1,2 @@
+0001-start-service-during-config.patch
+0002-fix-broken-build-tests.patch
diff --git a/debian/rules b/debian/rules
index ad30f5c..cc680cd 100755
--- a/debian/rules
+++ b/debian/rules
@@ -23,4 +23,7 @@ override_dh_auto_install:
 	install -D -o root -g root -m 755 apt-update/apt-update $(CURDIR)/$(root_dir)$(LIBDIR)/apt-update
 
 override_dh_installsystemd:
-	dh_installsystemd --no-enable --no-start --no-stop-on-upgrade
+	dh_installsystemd
+
+override_dh_auto_test:
+	HOME=$(shell mktemp -d) && make check3