Hi Mattia,
thank you for your review and for wishing to sponsor this package.
On 19/03/2016 13:37, Mattia Rizzolo wrote:
> control: tag -1 moreinfo
> control: owner -1 !
>
> On Fri, Feb 05, 2016 at 03:04:19PM +0100, Giulio Paci wrote:
>> I am looking for a sponsor for my package "msi-keyboard":
>
> alright.
>
>> https://anonscm.debian.org/cgit/collab-maint/msi-keyboard.git
>
> cool! finally somebody coming here with a git repository!
>
> though:
> * I see no tags, I expect to see at least the upstream/* tags (also
> otherwise gbp can be noisy and annoying)
Added a tag.
> * the HEAD points to master; given that the packaging branch is
> "debian", please configure the remote repository on collab-maint to
> have HEAD point to "debian" instead, so that cloning the repository
> gives you the packaging branch even without specifying it.
Fixed.
> * as said you are using a non-standard repository layout, so I expect
> debian/gbp.conf to be explicitly configured to use
> 'debian-branch = debian' and 'upstream-branch = master'
Fixed.
> * also, Vcs-Git does not specify the branch (which is kinda optional if
> you set correctly HEAD on the remote repository, but it wouldn't harm
> to write it here too)
Fixed.
> * I'm looking partly shocked at the commit
> 6fc1eec66c259cefeeb13453c3ceeb206fb24a55 why did you *substituted* the
> pristine-tar data? You should always just add them.
This is just because upstream never tagged a version, nor released a package.
I imported one using 0.0.1 version, but later noticed that 1.0 was set in the
source and so I renamed the package.
This was before publishing the repository, while doing preliminary package work.
Then I forgot to cleanup the history before publishing it. :-(
> that's just about the git repository.
>
> review on the package itself:
> * debian/control:
> + please sort the build-deps with wrap-and-sort or with cme or
Done.
> + please bump Standards-Version to 3.9.7
Done.
> + please drop the build-dep on dpkg-dev, that version is old enough
> and dpkg-dev is in build-essential
Ok.
> + please build-dep on debhelper (>= 9), no need for that ~
Ok.
> + please drop the version constriction on qt5-qmake, that version is
> old enough
Ok.
> * debian/changelog:
> + I see there is a weird space between the month and the year, how so?
Manual editing instead of using dch.
> * debian/copyright:
> + there is a \t at line 9, please just use spaces for consistency. I
> have a feeling you editor is configured to show a tab as 8 spaces,
> but this is not everybody's configuration, in fact here the line is
> indeed in a weird way.
Fixed (and correct guess about the editor configuration).
> + please use the license names as specified by DEP-5, so name it
> "BSD-3-clause" and not lowercase
Fixed.
> * debian/msi-keyboard.{post,pre}inst:
> + what aree these? Quasi-empty files?
Dropped. I added them while trying to understand how to deal udev files... And
then forgot about them.
BTW: do you know how to make the udev file work after installation, without
rebooting the system?
> * debian/patches:
> + debian/patches/series is empty, please remove the whole directory
Ok.
> Then, here it fails to build:
>
> debian/rules override_dh_auto_build
> make[1]: Entering directory '/build/msi-keyboard-1.0'
> PATH="`pwd`":"$PATH" help2man msi-keyboard --no-info --name="MSI steelseries
> keyboard color changer" > debian/msi-keyboard.1
> help2man: can't get `--help' info from msi-keyboard
> Try `--no-discard-stderr' if option outputs to stderr
> debian/rules:12: recipe for target 'override_dh_auto_build' failed
> make[1]: *** [override_dh_auto_build] Error 2
> make[1]: Leaving directory '/build/msi-keyboard-1.0'
> debian/rules:6: recipe for target 'build' failed
> make: *** [build] Error 2
> dpkg-buildpackage: error: debian/rules build gave error exit status 2
> E: Failed autobuilding of package
Fixed. The issue was the build order vs help2man run, as Jakub said.
> I wonder why not just use `help2man ./msi-keyboard ...` instead of
> messing around with PATH (which is probably what went wrong here).
As Jakub pointed out, this is needed to get proper command name in the man page.
