Merge ~enr0n/+git/systemd-hwe:main into ~canonical-foundations/+git/systemd-hwe:main

[Nick Rosbrook]

This adds the initial src:systemd-hwe package. With this in place, new hwdb rules can be added to the package by adding them to the appropriate hwdb.d/90-*-ubuntu.hwdb file.

[Lukas Märdian (slyon)]

Thank you Nick! I very much like what you did here.

Some comments/suggestions:

d/changelog:
* should the version number be somehow related to the upstream systemd version and/or the Ubuntu series? The package needs to be adopted for each upstream release, due to hwdb changes that we cherry-picked into systemd-hwe-hwdb might now be in conflict with upstream's version, which makes this package FTBFS via the test case provided. Maybe the version number should account for that, to make clear that e.g. 0.2.0 in Kinetic+1/LL isn't a linear extension to 0.1.0 in Kinetic/KK.
Some ideas (similar to https://launchpad.net/ubuntu/+source/ubuntu-release-upgrader):
- 22.10.1 (Ubuntu series + incremental ".X")
- 251.2.1 (corresponding systemd version + incremental ".X") <- I think this is my favorite, but please try to evaluate this a bit more, maybe ask Brian for some more input (as I'm not super sure about it)?

d/control:
* b-d udev >= 251.2-2ubuntu1~
* Lintian: E: systemd-hwe-hwdb: extended-description-is-empty

d/tests/control:
* I wonder if it'd make sense to copy/fork upstream's test/hwdb-test.sh as an additional autopkgtest?
* We could call the binary directly, avoiding the additional one-line script, e.g.:
--- a/debian/tests/control
+++ b/debian/tests/control
@@ -1,5 +1,6 @@
-Tests: test-sd-hwdb
+Test-Command: /usr/lib/systemd/tests/test-sd-hwdb
 Depends: systemd-tests,
   udev,
   systemd-hwe-hwdb,
 Restrictions: needs-root, allow-stderr
+Features: test-name=test-sd-hwdb

tests/test-hwdb-redundancy-check:
* rename file to tests/hwdb-redundancy (there's lots of tests/test/check redundancy in this path)? but MEH...
* get_hwdb_properties: we could use output.splitlines() to make it a bit more explicit
* test_no_duplicates: IIUC the test fails if we try to override a property of any pre-existing modalias. Shouldn't we only fail on a modalias setting the exact same values/properties? (i.e. a full duplicate) and maybe only print a warning for the "intersection" case, where some matches are only partially overriden? Otherwise, we wouldn't be able to change/override a (potentially wrong) property from upstream systemd via systemd-hwe-hwdb.

[Nick Rosbrook (enr0n)]

> Thank you Nick! I very much like what you did here.
>
> Some comments/suggestions:
>
> d/changelog:
> * should the version number be somehow related to the upstream systemd version
> and/or the Ubuntu series? The package needs to be adopted for each upstream
> release, due to hwdb changes that we cherry-picked into systemd-hwe-hwdb might
> now be in conflict with upstream's version, which makes this package FTBFS via
> the test case provided. Maybe the version number should account for that, to
> make clear that e.g. 0.2.0 in Kinetic+1/LL isn't a linear extension to 0.1.0
> in Kinetic/KK.
> Some ideas (similar to https://launchpad.net/ubuntu/+source/ubuntu-release-
> upgrader):
> - 22.10.1 (Ubuntu series + incremental ".X")
> - 251.2.1 (corresponding systemd version + incremental ".X") <- I think this
> is my favorite, but please try to evaluate this a bit more, maybe ask Brian
> for some more input (as I'm not super sure about it)?

Yes, I figured we might want something like this. I can ask Brian about the u-r-u convention to decide if it's a good fit. Otherwise I think basing the version number on the systemd version number would be good.

>
> d/control:
> * b-d udev >= 251.2-2ubuntu1~
> * Lintian: E: systemd-hwe-hwdb: extended-description-is-empty
>
>
> d/tests/control:
> * I wonder if it'd make sense to copy/fork upstream's test/hwdb-test.sh as an
> additional autopkgtest?

When I looked closer at it, I didn't think it provided any additional value to us. It's really intended to test the systemd-hwdb binary itself. But, maybe we could borrow the logic that checks for warnings on malformed rules, and fail the build if that happens (so I guess this would be a build time test rather than autopkgtest).

> * We could call the binary directly, avoiding the additional one-line script,
> e.g.:
> --- a/debian/tests/control
> +++ b/debian/tests/control
> @@ -1,5 +1,6 @@
> -Tests: test-sd-hwdb
> +Test-Command: /usr/lib/systemd/tests/test-sd-hwdb
> Depends: systemd-tests,
> udev,
> systemd-hwe-hwdb,
> Restrictions: needs-root, allow-stderr
> +Features: test-name=test-sd-hwdb

Nice, thanks!

> tests/test-hwdb-redundancy-check:
> * rename file to tests/hwdb-redundancy (there's lots of tests/test/check
> redundancy in this path)? but MEH...
> * get_hwdb_properties: we could use output.splitlines() to make it a bit more
> explicit
> * test_no_duplicates: IIUC the test fails if we try to override a property of
> any pre-existing modalias. Shouldn't we only fail on a modalias setting the
> exact same values/properties? (i.e. a full duplicate) and maybe only print a
> warning for the "intersection" case, where some matches are only partially
> overriden? Otherwise, we wouldn't be able to change/override a (potentially
> wrong) property from upstream systemd via systemd-hwe-hwdb.

ACK on the first two suggestions. However, your understanding on the test case is slightly off. The current implementation *does* only fail with exact duplicates, not when a property is overridden for a given modalias (the list items are of the form `KEY=VALUE`).

[Nick Rosbrook (enr0n)]

I believe I addressed all of your review comments. Of note, I went with the "follow systemd's version number" option. I think this is basically analogous to u-r-u's approach, but in that case following the Ubuntu release number makes sense.

Also, I added a check in tests/hwdb-redundancy for stderr output from `systemd-hwdb update`. IMO, this replicates the only functionality we would benefit from if we copied upstream's test/hwdb-test.sh.

[Lukas Märdian (slyon)]

Thank you very much for addressing my concerns and explaining + extending the test case.

LGTM!

I talked to Łukasz about how to best get this into Kinetic (being a new package):
Could you please create a tracking bug on Launchpad (against the "ubuntu" component, i.e.: https://bugs.launchpad.net/ubuntu/+filebug) briefly describing the rational of this new package.

This bug reference should then be closed form debian/changelog, e.g.:
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,6 +1,6 @@
-systemd-hwe (251.2.1) UNRELEASED; urgency=medium
+systemd-hwe (251.2.1) kinetic; urgency=medium

- * Initial release.
+ * Initial release. (LP: #XXX)
   * Add hwdb.d/90-hwe-ubuntu.hwdb as a placeholder, and to serve as
     documentation.
   * Add a build-time test to check for duplicate rules between

Please feel free to push that change to the git repo directly. I'm happy to sponsor this package into Kinetic's NEW queue afterwards, for any archive-admin to pick up on.

review: Approve

diff --git a/Makefile b/Makefile
 new file mode 100644
index 0000000..bb70475
--- /dev/null
+++ b/Makefile
@@ -0,0 +1,3 @@
+.PHONY: test
+test:
+	./tests/hwdb-redundancy
diff --git a/debian/changelog b/debian/changelog
 new file mode 100644
index 0000000..42a99df
--- /dev/null
+++ b/debian/changelog
@@ -0,0 +1,10 @@
+systemd-hwe (251.2.1) UNRELEASED; urgency=medium
+
+  * Initial release.
+  * Add hwdb.d/90-hwe-ubuntu.hwdb as a placeholder, and to serve as
+    documentation.
+  * Add a build-time test to check for duplicate rules between
+    systemd-hwe-hwdb and udev.
+  * Use test-sd-hwdb from system-tests as an autopkgtest.
+
+ -- Nick Rosbrook <[e-mail]>  Wed, 06 Jul 2022 13:19:33 -0400
diff --git a/debian/control b/debian/control
 new file mode 100644
index 0000000..855daa2
--- /dev/null
+++ b/debian/control
@@ -0,0 +1,18 @@
+Source: systemd-hwe
+Section: admin
+Priority: optional
+Maintainer: Ubuntu Developers <[e-mail]>
+Build-Depends: debhelper-compat (= 13),
+               python3-all,
+               udev (>= 251.2-2ubuntu1~),
+Standards-Version: 4.6.0
+Rules-Requires-Root: no
+Vcs-Git: https://git.launchpad.net/~canonical-foundations/+git/systemd-hwe -b main
+
+Package: systemd-hwe-hwdb
+Architecture: all
+Depends: ${misc:Depends},
+         udev,
+Description: udev rules for hardware enablement (HWE)
+ systemd-hwe-hwdb contains hwdb rules for HWE on Ubuntu,
+ which are not yet present in systemd.
diff --git a/debian/copyright b/debian/copyright
 new file mode 100644
index 0000000..76853e1
--- /dev/null
+++ b/debian/copyright
@@ -0,0 +1,24 @@
+Format: https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/
+Upstream-Name: Ubuntu systemd hardware enablement (HWE)
+Source: https://git.launchpad.net/~canonical-foundations/+git/systemd-hwe
+
+Files: *
+Copyright: 2022 Canonical Ltd.
+License: GPL-2.0+
+
+License: GPL-2.0+
+ This package is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 2 of the License, or
+ (at your option) any later version.
+ .
+ This package is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ GNU General Public License for more details.
+ .
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <https://www.gnu.org/licenses/>
+ .
+ On Debian systems, the complete text of the GNU General
+ Public License version 2 can be found in "/usr/share/common-licenses/GPL-2".
diff --git a/debian/rules b/debian/rules
 new file mode 100755
index 0000000..a5e0d78
--- /dev/null
+++ b/debian/rules
@@ -0,0 +1,4 @@
+#! /usr/bin/make -f
+
+%:
+	dh $@
diff --git a/debian/source/format b/debian/source/format
 new file mode 100644
index 0000000..89ae9db
--- /dev/null
+++ b/debian/source/format
@@ -0,0 +1 @@
+3.0 (native)
diff --git a/debian/systemd-hwe-hwdb.install b/debian/systemd-hwe-hwdb.install
 new file mode 100644
index 0000000..6e78ded
--- /dev/null
+++ b/debian/systemd-hwe-hwdb.install
@@ -0,0 +1 @@
+hwdb.d/* lib/udev/hwdb.d
diff --git a/debian/systemd-hwe-hwdb.postinst b/debian/systemd-hwe-hwdb.postinst
 new file mode 100644
index 0000000..e6584d5
--- /dev/null
+++ b/debian/systemd-hwe-hwdb.postinst
@@ -0,0 +1,12 @@
+#!/bin/sh
+
+set -e
+
+case "$1" in
+    configure)
+        systemd-hwdb --usr update || true
+        exit 0
+        ;;
+esac
+
+#DEBHELPER#
diff --git a/debian/systemd-hwe-hwdb.postrm b/debian/systemd-hwe-hwdb.postrm
 new file mode 100644
index 0000000..8dc5169
--- /dev/null
+++ b/debian/systemd-hwe-hwdb.postrm
@@ -0,0 +1,12 @@
+#!/bin/sh
+
+set -e
+
+case "$1" in
+    remove|purge)
+        systemd-hwdb --usr update || true
+        exit 0
+        ;;
+esac
+
+#DEBHELPER#
diff --git a/debian/tests/control b/debian/tests/control
 new file mode 100644
index 0000000..d68bdab
--- /dev/null
+++ b/debian/tests/control
@@ -0,0 +1,6 @@
+Test-Command: /usr/lib/systemd/tests/test-sd-hwdb
+Depends: systemd-tests,
+  udev,
+  systemd-hwe-hwdb,
+Restrictions: needs-root, allow-stderr
+Features: test-name=test-sd-hwdb
diff --git a/hwdb.d/90-hwe-ubuntu.hwdb b/hwdb.d/90-hwe-ubuntu.hwdb
 new file mode 100644
index 0000000..0e023ac
--- /dev/null
+++ b/hwdb.d/90-hwe-ubuntu.hwdb
@@ -0,0 +1,11 @@
+# This file intentionally contains no hwdb rules, and serves as documentation
+# for hwdb rules shipped with systemd-hwe-hwdb.
+#
+# The systemd-hwe-hwdb package contains hwdb rules for hardware enablement
+# (HWE) on Ubuntu, which are not (yet) present in systemd. The hwdb files
+# included in systemd-hwe-hwdb follow the convention of those shipped with
+# systemd. For example, HWE hwdb rules for keyboards (if any) are defined in
+# 90-keyboard-ubuntu.hwdb.
+#
+# If no additional hwdb rules are needed for a particular systemd release, this
+# may be the only file included in systemd-hwe-hwdb.
diff --git a/tests/hwdb-redundancy b/tests/hwdb-redundancy
 new file mode 100755
index 0000000..7adade1
--- /dev/null
+++ b/tests/hwdb-redundancy
@@ -0,0 +1,82 @@
+#!/usr/bin/env python3
+# Check against current udev rules for any redundant hwdb rules.
+
+import os
+import sys
+import glob
+import subprocess
+import tempfile
+import shutil
+import unittest
+
+
+def get_modalias_list(root=''):
+    modalias_list = []
+
+    for path in glob.glob('{}/**/*.hwdb'.format(root), recursive=True):
+        with open(path, 'r') as f:
+            lines = f.readlines()
+
+        for line in lines:
+            line = str(line).rstrip()
+            if line.startswith(' ') or line.startswith('#'):
+                continue
+
+            if line in ['\n', '\r\n']:
+                continue
+
+            if not line in modalias_list:
+                modalias_list.append(line)
+
+    return modalias_list
+
+def get_hwdb_properties(modalias_list, root=None):
+    properties = {}
+
+    cmd = ['systemd-hwdb', 'query']
+    if root is not None:
+        cmd += ['--root', root]
+
+    for modalias in modalias_list:
+        output = subprocess.run(cmd + [modalias],
+                       stdout=subprocess.PIPE).stdout.decode('utf-8')
+        properties[modalias] = [line for line in output.splitlines() if line]
+
+    return properties
+
+
+class RedundancyCheckTest(unittest.TestCase):
+    """ Check for hwdb rule redundancies. """
+
+    def setUp(self):
+        # Using the hwdb rules from this package, generate a hwdb.bin
+        # in a temporary directory
+        self.tmpdir = tempfile.mkdtemp(prefix='systemd-hwe-hwdb-test')
+
+        src = os.path.join(os.path.dirname(os.path.realpath(__file__)),
+                           '../hwdb.d')
+        dst = os.path.join(self.tmpdir, 'etc/udev/hwdb.d')
+        shutil.copytree(src, dst)
+        p = subprocess.run(['systemd-hwdb', 'update', '--root', self.tmpdir],
+                           stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        self.assertEqual(p.returncode, 0)
+        self.assertEqual(p.stderr, b'', 'hwdb rules produced warnings')
+
+    def tearDown(self):
+        shutil.rmtree(self.tmpdir)
+
+    def test_no_duplicates(self):
+        modalias_list = get_modalias_list(root=self.tmpdir)
+        existing_hwdb_props = get_hwdb_properties(modalias_list)
+        new_hwdb_props = get_hwdb_properties(modalias_list, root=self.tmpdir)
+
+        for modalias in modalias_list:
+            intersection = list(set(existing_hwdb_props[modalias]) &
+                                set(new_hwdb_props[modalias]))
+            self.assertEqual([], intersection,
+                             'Duplicate hwdb rules for {}'.format(modalias))
+
+
+if __name__ == '__main__':
+    unittest.main(testRunner=unittest.TextTestRunner(stream=sys.stdout,
+                                                     verbosity=2))