Re: Bug#994594: ITP: time-decode -- timestamp decoder and converter
Hello Jan, removing the ITP's email address from this reply.
> Thank you for your offer to sponsor `time-decode`, Samuel. I worked on
> the package and want to ask for a review. The sources for the packaging
> process can be found here:
>
> https://salsa.debian.org/jgru/time-decode
Cool, I have created the repo under the team's org and I gave you
maintainer permission until the end of 2022 [0].
> `time-decode`' can be built successfully by running the commands listed
> in [0]. Furthermore, I created three autopackage-tests, which check the
> functionality of the resulting package. Those can be run with the
> command listed in [1].
>
> The package seems to be lintian-clean, except for some warnings about
> NMU-version numbering and a not forwarded manpage.
Allow me to suggest one thing which is usually helpful when reviewing
packages (both my own and somebody else's): You can setup salsa-ci[1]
to have a summary of some basic checks available to everyone.
I see you commited salsa-ci.yml, but maybe forgot to enable CI in your
project (or to trigger a build after doing so).
I'll enumerate the findings of my review to make it easier to discuss
them, their order doesn't mean anything:
1) d/tests/*~:
I see you accidentally committed some vim temporary/swap files there,
you can remove all of them.
2) d/manpage/time-decode.1:
In the SYNOPSIS, only the first line of parameters is being
highlighted in bold, you need to add the .B marker at the start of
each line.
3) d/changelog:
The NMU issues lintian is complaining about are related to a few
things you'll have to fix:
3a) The last changelog entry of a NEW package needs to start with "*
Initial release (Closes: #994594)", so I suggest you merge the last
two entries together.
3b) Your name in the last entry is not matching what's in d/control,
so lintian thinks it's an NMU.
3c) The changelog entry for a NEW package doesn't have to contain
anything besides the "Initial release" message, sometimes we want to
add a few more things, especially if the packaging is based on another
distro, to show what has been changed. But in the case of the entries
you have there, they can be removed[3].
4) d/control:
4a) You can add the team as the Maintainer and move yourself to
Uploader, if you'd like to keep the package under the team's umbrella.
4b) Standards-Version can be bumped to 4.6.0
5) missing-build-dependency-for-dh-addon:
There's a recent lintian version that issues an Error for the above
finding, discussion is ongoing in the lintian's bug report, but
everything in pointing in the direction of this not being a real
issue:
https://bugs.debian.org/995498
6) file-without-copyright-information:
Lintian is complaining that the .gitignore file is not matched by any
entry in d/copyright, my suggestion is to follow the practice of using
the first entry in d/copyright to match all of the files "Files: *"
and then have following entries where you target the files that you
want to exclude from the catch-all case. This will avoid leaving files
unmatched, but you're free to explicitly list all the files if you
want to.
> I would be very glad, if you or another team member could conduct a
> thourough review and provide some feedback, so that the package might
> find its way in unstable's or testing's package sources eventually in
> the future. If there are any issues, please let me know.
Great, I'm replying to this email with the review, please always feel
free to request a private review if you'd like.
It's good to have them in the team's list as the whole process is a
learning opportunity for everyone (myself included)[2], besides being
able to reuse some reviews when helping other people.
> [0] `git clone https://salsa.debian.org/jgru/time-decode.git; cd
> time-decode; gbp buildpackage --git-debian-branch=debian/master
> --git-ignore-new --git-upstream-tree="upstream";`
Just a tip here, if you clone with gbp clone, and you setup d/gbp.conf
the way we do for the team's packages, you just need a:
gbp clone https://salsa.debian.org/jgru/time-decode.git
cd time-decode && gbp buildpackage
The "--git-ignore-new" is not needed as this is a brand new clone, you
probably got used to using it when you want to test-build ignoring
uncommitted changes.
I understand you probably already know all of this, but it's worth
saying as I've seen other people getting bitten by it; I recommend you
to be on the watch for this parameter confusing you when you see that
the build fails due to some change you thought you had committed
already.
The packaging is in a good state, and the tests are much appreciated,
feel free to push your next changes directly to the team's repo.
Thank you for working on this package :)
[0] As usual, if the permission expires before you receive broader
permissions permanently, we can extend that.
[1] https://salsa.debian.org/salsa-ci-team/pipeline#basic-use
[2] And somebody else could also call out a mistake I could make in the review.
[3] That is, unless you have a preference on keeping them, let me know
if that's the case.
--
Samuel Henrique <samueloph>
Reply to: