Bug#1010494: Re: Bug#1010494: RFS: usbrelay/1.0-1 -- USB HID relay driver
On Wed, May 04, 2022 at 01:27:23AM +0200, Michal Sojka wrote:
> Hi Jan and Tobias,
>
> @Jan good to hear from you.
> @Tobias, thanks for useful suggestions, see my reactions below.
>
> On Tue, May 03 2022, Jan Dittberner wrote:
> > As you might have guessed I'm busier than I would like and do usually need a
> > bit of time to respond.
> >
> > It would have been a good idea to contact me directly or via a wishlist bug
> > requesting a package update. I would have answered a wishlist bug
> > requesting an usbrelay upstream version update. The RFS came a bit
> > surprisingly. A short mail before starting the work would have been nice. I
> > had contact with Darryl Bond in the past and responded to his
> > requests.
>
> I apologize for not contacting you. I'm also in contact with Darryl Bond
> and I understood that he tried to contact you recently, but without
> success. It might have been my misunderstanding.
>
> > I would be happy to have a co-maintainer for usbrelay. @Michal you may add
> > yourself to Uploaders if you are interested in taking care of the package in
> > the future.
>
> Yes, I'll add myself. I'm using this software quite extensively and will
> be happy to help with packaging.
\o/
> @Jan How shall I proceed now? I've implemented the changes suggested by
> Tobias, but I need to test the result again with the hardware, for which
> I'll need a day or two. What then? Shall I upload a new version to
> mentors.d.o or send salsa merge request or both?
I've just gave you access rights to the repo on salsa, you should be able to
commit directly.
For the review and upload, an git reference or mentors upload will works both
for me.
> > Please run lintian with the flags -i -v -E --pedantic to get the maximum
> > output. I recommend using pbuilder or sbuild to build in a clean
> > environment. I usually use sbuild in combination with git-buildpackage to do
> > my package builds. Both sbuild and pbuilder can lintian automatically after
> > a finished package build.
>
> Thanks.
>
> On Tue, May 03 2022, Tobias Frost wrote:
> > Ok, let me check the package: (I'm using the mentors version for the review)
> > - As said earlier, depending if you want a Team upload or follow the ITS, this
> > needs some entry in d/changelog, depending how you want to proceed...
> > - debian/io.github.darrylb123.usbrelay.metainfo.xml should possible brought upstream,
> > shouldn't it (no need to change for the upload, but I guess this is
> > not debian specific)
>
> I'll send that (as well as other parts) upstream. I first wanted to know
> what changes are required for Debian and then send the final version
> upstream.
>
> > - d/rules
> > - (bikeshed -- no need to change) I'd prefer to use d/clean instead of
> > overriding the clean target; overrides are harder to maintain :)
>
> I didn't notice this possibility - it's definitely nicer!
>
> > - the man page is still in the debian directory -- it can be deleted as
> > it is now part of upstream.
>
> Upstream has usbrelay.1, I've added usbrelayd.8. As mentioned above,
> I'll send it upstream later.
ok, I've missed that little detail that those are different manpages..
Sorry for the noise.
> > - same for the udev rules.
>
> Upstream rules are slightly different - looser permissions, different group.
Ok, that created my confusion... For conciseness, I'd recommend to patch the
file instead of having a copy.
> > - d/usbrelay.install has a hard-coded architecture-dependent path. That will break build on
> > archs != amd64.
>
> Good catch.
>
> > - d/postinst (and postrm):
> > The username is not correct. According to Debian Policy, 4.9.1, it must start with an "_"
> > I guess also that you don't want/need a homedirectory for that uses, so its $HOME
> > should be /nonexistent in this case. (Policy 9.2.3)
> > HOWEVER, let me suggest something else:
> > Use the DynamicUser feature from systemd:
> >
> > DynamicUser=yes
> > SupplementaryGroups=plugdev
> >
> > in the service file should make postint/postrm no longer be needed.
>
> That's definitely a good thing.
>
> > - Lintian has a few remarks: (my related remarks in brackets)
> > W: usbrelay source: no-nmu-in-changelog
> > W: usbrelay source: source-nmu-has-incorrect-version-number 1.0-1
> > (will be gone once the changelog mentions the team upload or after the salvage procedure is done)
> > I: usbrelay source: debian-rules-parses-dpkg-parsechangelog (line 20)
> > (see abovr)
> > I: usbrelay source: older-debian-watch-file-standard 3
> > (could be updated to version 4, just s/3/4/ in the header)
>
> done
>
> > I: usbrelayd: package-supports-alternative-init-but-no-init.d-script lib/systemd/system/usbrelayd.service
> > (can be ignored)
> > I: usbrelay source: patch-not-forwarded-upstream debian/patches/0002-Mention-README-in-the-man-page.patch
> > I: usbrelay source: patch-not-forwarded-upstream debian/patches/gcc9.patch
> > (see below)
> > I: usbrelayd: systemd-service-file-missing-documentation-key [lib/systemd/system/usbrelayd.service]
> > (possibly ask upstream to ammend the service file accordingly)
>
> Added, will send upstream later.
>
> > X: usbrelay source: debian-watch-does-not-check-gpg-signature [debian/watch]
> > (it would be nice if upstream could pgp-sign their tarballs, so noone can tamper with them.
> > They sign their commits already, so a key would be available. Not
> > required for this upload.)
>
> I think that tarballs are created automatically by GitHub upon tagging
> the release. I guess that for that, we would need to generate tarballs
> locally, sign them and upload to github. I think we will change the
> release procedure anyway, so we will discuss about this too.
>
> > P: usbrelay source: maintainer-manual-page debian/usbrelayd.8
> > (see above -- deletign the file will fix this.)
> > X: usbrelay source: prefer-uscan-symlink filenamemangle s/.*(\d[\d\.]*)\.tar\.gz/usbrelay-$1.tar.gz/ [debian/watch:3]
> > (ignore that)
> > P: usbrelay source: silent-on-rules-requiring-root [debian/control]
> > (Add "Rules-Requires-Root: no" to d/control ->
> > https://lintian.debian.org/tags/silent-on-rules-requiring-root)
>
> Done.
>
> > X: usbrelayd: systemd-service-file-missing-hardening-features [lib/systemd/system/usbrelayd.service]
> > (DynamicUser=yes should fix that too)
> > X: usbrelay source: upstream-metadata-file-is-missing
> > (would be nice to have, but not required: https://wiki.debian.org/UpstreamMetadata
> > example: https://salsa.debian.org/games-team/darkradiant/-/blob/master/debian/upstream/metadata
>
> Will prepare that later.
>
> > You have mentioned that you are involved with upstream: In this case, can
> > you check if the debian/patches can be brought upstream? I don't think they are
> > to be Debian specific, especially the gcc-9 patch. (no need to do that for this
> > upload, but if you forward them now, please add the dep3-Tag "Forwarded" to the
> > d/patch)
>
> As mentioned above, that's planned.
>
> > Overall, package quality is nice, only a few things to fix before this can be uploaded.
>
> I've pushed the changes to git. I don't have the hardware with me right
> now, so I cannot test whether the changes didn't break something. I'll
> test it in the upcomming days and let you know.
✔. You can remove the "moreinfo" tag on this bug to signal that it is ready.
--
tobi
Reply to: