Merge ~enr0n/+git/systemd-hwe:main into ~canonical-foundations/+git/systemd-hwe:main
- Git
- lp:~enr0n/+git/systemd-hwe
- main
- Merge into main
Status: | Merged |
---|---|
Merged at revision: | 01bb9581ff1f687b2924a8ab88888c9564f138b5 |
Proposed branch: | ~enr0n/+git/systemd-hwe:main |
Merge into: | ~canonical-foundations/+git/systemd-hwe:main |
Diff against target: |
256 lines (+184/-0) 12 files modified
Makefile (+3/-0) debian/changelog (+10/-0) debian/control (+18/-0) debian/copyright (+24/-0) debian/rules (+4/-0) debian/source/format (+1/-0) debian/systemd-hwe-hwdb.install (+1/-0) debian/systemd-hwe-hwdb.postinst (+12/-0) debian/systemd-hwe-hwdb.postrm (+12/-0) debian/tests/control (+6/-0) hwdb.d/90-hwe-ubuntu.hwdb (+11/-0) tests/hwdb-redundancy (+82/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Lukas Märdian | Approve | ||
Review via email: mp+426491@code.launchpad.net |
Commit message
Description of the change
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/
Lukas Märdian (slyon) wrote (last edit ): | # |
Nick Rosbrook (enr0n) wrote : | # |
> Thank you Nick! I very much like what you did here.
>
> Some comments/
>
> 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:/
> 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-
>
>
> 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/
> +++ b/debian/
> @@ -1,5 +1,6 @@
> -Tests: test-sd-hwdb
> +Test-Command: /usr/lib/
> Depends: systemd-tests,
> udev,
> systemd-hwe-hwdb,
> Restrictions: needs-root, allow-stderr
> +Features: test-name=
Nice, thanks!
> tests/test-
> * rename file to tests/hwdb-
> redundancy in this path)? but MEH...
> * get_hwdb_
> 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) wrote : | # |
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-
Lukas Märdian (slyon) wrote : | # |
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:/
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/
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.
Preview Diff
1 | diff --git a/Makefile b/Makefile |
2 | new file mode 100644 |
3 | index 0000000..bb70475 |
4 | --- /dev/null |
5 | +++ b/Makefile |
6 | @@ -0,0 +1,3 @@ |
7 | +.PHONY: test |
8 | +test: |
9 | + ./tests/hwdb-redundancy |
10 | diff --git a/debian/changelog b/debian/changelog |
11 | new file mode 100644 |
12 | index 0000000..42a99df |
13 | --- /dev/null |
14 | +++ b/debian/changelog |
15 | @@ -0,0 +1,10 @@ |
16 | +systemd-hwe (251.2.1) UNRELEASED; urgency=medium |
17 | + |
18 | + * Initial release. |
19 | + * Add hwdb.d/90-hwe-ubuntu.hwdb as a placeholder, and to serve as |
20 | + documentation. |
21 | + * Add a build-time test to check for duplicate rules between |
22 | + systemd-hwe-hwdb and udev. |
23 | + * Use test-sd-hwdb from system-tests as an autopkgtest. |
24 | + |
25 | + -- Nick Rosbrook <nick.rosbrook@canonical.com> Wed, 06 Jul 2022 13:19:33 -0400 |
26 | diff --git a/debian/control b/debian/control |
27 | new file mode 100644 |
28 | index 0000000..855daa2 |
29 | --- /dev/null |
30 | +++ b/debian/control |
31 | @@ -0,0 +1,18 @@ |
32 | +Source: systemd-hwe |
33 | +Section: admin |
34 | +Priority: optional |
35 | +Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com> |
36 | +Build-Depends: debhelper-compat (= 13), |
37 | + python3-all, |
38 | + udev (>= 251.2-2ubuntu1~), |
39 | +Standards-Version: 4.6.0 |
40 | +Rules-Requires-Root: no |
41 | +Vcs-Git: https://git.launchpad.net/~canonical-foundations/+git/systemd-hwe -b main |
42 | + |
43 | +Package: systemd-hwe-hwdb |
44 | +Architecture: all |
45 | +Depends: ${misc:Depends}, |
46 | + udev, |
47 | +Description: udev rules for hardware enablement (HWE) |
48 | + systemd-hwe-hwdb contains hwdb rules for HWE on Ubuntu, |
49 | + which are not yet present in systemd. |
50 | diff --git a/debian/copyright b/debian/copyright |
51 | new file mode 100644 |
52 | index 0000000..76853e1 |
53 | --- /dev/null |
54 | +++ b/debian/copyright |
55 | @@ -0,0 +1,24 @@ |
56 | +Format: https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/ |
57 | +Upstream-Name: Ubuntu systemd hardware enablement (HWE) |
58 | +Source: https://git.launchpad.net/~canonical-foundations/+git/systemd-hwe |
59 | + |
60 | +Files: * |
61 | +Copyright: 2022 Canonical Ltd. |
62 | +License: GPL-2.0+ |
63 | + |
64 | +License: GPL-2.0+ |
65 | + This package is free software; you can redistribute it and/or modify |
66 | + it under the terms of the GNU General Public License as published by |
67 | + the Free Software Foundation; either version 2 of the License, or |
68 | + (at your option) any later version. |
69 | + . |
70 | + This package is distributed in the hope that it will be useful, |
71 | + but WITHOUT ANY WARRANTY; without even the implied warranty of |
72 | + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
73 | + GNU General Public License for more details. |
74 | + . |
75 | + You should have received a copy of the GNU General Public License |
76 | + along with this program. If not, see <https://www.gnu.org/licenses/> |
77 | + . |
78 | + On Debian systems, the complete text of the GNU General |
79 | + Public License version 2 can be found in "/usr/share/common-licenses/GPL-2". |
80 | diff --git a/debian/rules b/debian/rules |
81 | new file mode 100755 |
82 | index 0000000..a5e0d78 |
83 | --- /dev/null |
84 | +++ b/debian/rules |
85 | @@ -0,0 +1,4 @@ |
86 | +#! /usr/bin/make -f |
87 | + |
88 | +%: |
89 | + dh $@ |
90 | diff --git a/debian/source/format b/debian/source/format |
91 | new file mode 100644 |
92 | index 0000000..89ae9db |
93 | --- /dev/null |
94 | +++ b/debian/source/format |
95 | @@ -0,0 +1 @@ |
96 | +3.0 (native) |
97 | diff --git a/debian/systemd-hwe-hwdb.install b/debian/systemd-hwe-hwdb.install |
98 | new file mode 100644 |
99 | index 0000000..6e78ded |
100 | --- /dev/null |
101 | +++ b/debian/systemd-hwe-hwdb.install |
102 | @@ -0,0 +1 @@ |
103 | +hwdb.d/* lib/udev/hwdb.d |
104 | diff --git a/debian/systemd-hwe-hwdb.postinst b/debian/systemd-hwe-hwdb.postinst |
105 | new file mode 100644 |
106 | index 0000000..e6584d5 |
107 | --- /dev/null |
108 | +++ b/debian/systemd-hwe-hwdb.postinst |
109 | @@ -0,0 +1,12 @@ |
110 | +#!/bin/sh |
111 | + |
112 | +set -e |
113 | + |
114 | +case "$1" in |
115 | + configure) |
116 | + systemd-hwdb --usr update || true |
117 | + exit 0 |
118 | + ;; |
119 | +esac |
120 | + |
121 | +#DEBHELPER# |
122 | diff --git a/debian/systemd-hwe-hwdb.postrm b/debian/systemd-hwe-hwdb.postrm |
123 | new file mode 100644 |
124 | index 0000000..8dc5169 |
125 | --- /dev/null |
126 | +++ b/debian/systemd-hwe-hwdb.postrm |
127 | @@ -0,0 +1,12 @@ |
128 | +#!/bin/sh |
129 | + |
130 | +set -e |
131 | + |
132 | +case "$1" in |
133 | + remove|purge) |
134 | + systemd-hwdb --usr update || true |
135 | + exit 0 |
136 | + ;; |
137 | +esac |
138 | + |
139 | +#DEBHELPER# |
140 | diff --git a/debian/tests/control b/debian/tests/control |
141 | new file mode 100644 |
142 | index 0000000..d68bdab |
143 | --- /dev/null |
144 | +++ b/debian/tests/control |
145 | @@ -0,0 +1,6 @@ |
146 | +Test-Command: /usr/lib/systemd/tests/test-sd-hwdb |
147 | +Depends: systemd-tests, |
148 | + udev, |
149 | + systemd-hwe-hwdb, |
150 | +Restrictions: needs-root, allow-stderr |
151 | +Features: test-name=test-sd-hwdb |
152 | diff --git a/hwdb.d/90-hwe-ubuntu.hwdb b/hwdb.d/90-hwe-ubuntu.hwdb |
153 | new file mode 100644 |
154 | index 0000000..0e023ac |
155 | --- /dev/null |
156 | +++ b/hwdb.d/90-hwe-ubuntu.hwdb |
157 | @@ -0,0 +1,11 @@ |
158 | +# This file intentionally contains no hwdb rules, and serves as documentation |
159 | +# for hwdb rules shipped with systemd-hwe-hwdb. |
160 | +# |
161 | +# The systemd-hwe-hwdb package contains hwdb rules for hardware enablement |
162 | +# (HWE) on Ubuntu, which are not (yet) present in systemd. The hwdb files |
163 | +# included in systemd-hwe-hwdb follow the convention of those shipped with |
164 | +# systemd. For example, HWE hwdb rules for keyboards (if any) are defined in |
165 | +# 90-keyboard-ubuntu.hwdb. |
166 | +# |
167 | +# If no additional hwdb rules are needed for a particular systemd release, this |
168 | +# may be the only file included in systemd-hwe-hwdb. |
169 | diff --git a/tests/hwdb-redundancy b/tests/hwdb-redundancy |
170 | new file mode 100755 |
171 | index 0000000..7adade1 |
172 | --- /dev/null |
173 | +++ b/tests/hwdb-redundancy |
174 | @@ -0,0 +1,82 @@ |
175 | +#!/usr/bin/env python3 |
176 | +# Check against current udev rules for any redundant hwdb rules. |
177 | + |
178 | +import os |
179 | +import sys |
180 | +import glob |
181 | +import subprocess |
182 | +import tempfile |
183 | +import shutil |
184 | +import unittest |
185 | + |
186 | + |
187 | +def get_modalias_list(root=''): |
188 | + modalias_list = [] |
189 | + |
190 | + for path in glob.glob('{}/**/*.hwdb'.format(root), recursive=True): |
191 | + with open(path, 'r') as f: |
192 | + lines = f.readlines() |
193 | + |
194 | + for line in lines: |
195 | + line = str(line).rstrip() |
196 | + if line.startswith(' ') or line.startswith('#'): |
197 | + continue |
198 | + |
199 | + if line in ['\n', '\r\n']: |
200 | + continue |
201 | + |
202 | + if not line in modalias_list: |
203 | + modalias_list.append(line) |
204 | + |
205 | + return modalias_list |
206 | + |
207 | +def get_hwdb_properties(modalias_list, root=None): |
208 | + properties = {} |
209 | + |
210 | + cmd = ['systemd-hwdb', 'query'] |
211 | + if root is not None: |
212 | + cmd += ['--root', root] |
213 | + |
214 | + for modalias in modalias_list: |
215 | + output = subprocess.run(cmd + [modalias], |
216 | + stdout=subprocess.PIPE).stdout.decode('utf-8') |
217 | + properties[modalias] = [line for line in output.splitlines() if line] |
218 | + |
219 | + return properties |
220 | + |
221 | + |
222 | +class RedundancyCheckTest(unittest.TestCase): |
223 | + """ Check for hwdb rule redundancies. """ |
224 | + |
225 | + def setUp(self): |
226 | + # Using the hwdb rules from this package, generate a hwdb.bin |
227 | + # in a temporary directory |
228 | + self.tmpdir = tempfile.mkdtemp(prefix='systemd-hwe-hwdb-test') |
229 | + |
230 | + src = os.path.join(os.path.dirname(os.path.realpath(__file__)), |
231 | + '../hwdb.d') |
232 | + dst = os.path.join(self.tmpdir, 'etc/udev/hwdb.d') |
233 | + shutil.copytree(src, dst) |
234 | + p = subprocess.run(['systemd-hwdb', 'update', '--root', self.tmpdir], |
235 | + stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
236 | + self.assertEqual(p.returncode, 0) |
237 | + self.assertEqual(p.stderr, b'', 'hwdb rules produced warnings') |
238 | + |
239 | + def tearDown(self): |
240 | + shutil.rmtree(self.tmpdir) |
241 | + |
242 | + def test_no_duplicates(self): |
243 | + modalias_list = get_modalias_list(root=self.tmpdir) |
244 | + existing_hwdb_props = get_hwdb_properties(modalias_list) |
245 | + new_hwdb_props = get_hwdb_properties(modalias_list, root=self.tmpdir) |
246 | + |
247 | + for modalias in modalias_list: |
248 | + intersection = list(set(existing_hwdb_props[modalias]) & |
249 | + set(new_hwdb_props[modalias])) |
250 | + self.assertEqual([], intersection, |
251 | + 'Duplicate hwdb rules for {}'.format(modalias)) |
252 | + |
253 | + |
254 | +if __name__ == '__main__': |
255 | + unittest.main(testRunner=unittest.TextTestRunner(stream=sys.stdout, |
256 | + verbosity=2)) |
Thank you Nick! I very much like what you did here.
Some comments/ suggestions:
d/changelog: /launchpad. net/ubuntu/ +source/ ubuntu- release- upgrader):
* 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:/
- 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: description- is-empty
* b-d udev >= 251.2-2ubuntu1~
* Lintian: E: systemd-hwe-hwdb: extended-
d/tests/control: tests/control tests/control systemd/ tests/test- sd-hwdb hwe-hwdb, test-sd- hwdb
* 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/
+++ b/debian/
@@ -1,5 +1,6 @@
-Tests: test-sd-hwdb
+Test-Command: /usr/lib/
Depends: systemd-tests,
udev,
systemd-
Restrictions: needs-root, allow-stderr
+Features: test-name=
tests/test- hwdb-redundancy -check: redundancy (there's lots of tests/test/check redundancy in this path)? but MEH... properties: we could use output.splitlines() to make it a bit more explicit
* rename file to tests/hwdb-
* get_hwdb_
* 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.