Code review comment for ~enr0n/+git/systemd-hwe:main

Revision history for this message
Lukas Märdian (slyon) wrote (last edit ):

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.

« Back to merge proposal