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

> 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`).