Re: Bug#994594: ITP: time-decode -- timestamp decoder and converter
Hello Samuel,
On 10/3/21 8:51 PM, Samuel Henrique wrote:
> 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].
Thank you for creating the repository!
>
> 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).
Totally understandable, I activated the CI-pipeline, with the already
present yml-file.
>
> 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.
Remove those swap files, my bad, sorry!
>
> 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.
Added .B markers. Looks far nicer now.
>
> 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.
Merged. Originally I wanted to keep those two changelog entries, since I
created several patches for version 3.1.1, which were incorporated by
upstream, but this makes no sense, when the repo's content moves to the
team-managed repository, I guess.
>
> 3b) Your name in the last entry is not matching what's in d/control,
> so lintian thinks it's an NMU.
I overlooked this slight deviation, thanks for pointing this out.
>
> 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].
I removed those detailed bulletpoints.
>
> 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.
I thought that uploader is the actual sponsor of the package, therefore
I added myself as maintainer.
>
> 4b) Standards-Version can be bumped to 4.6.0
Both issues corrected.
>
>
> 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
I read the discussion and decided to add `:any`, so that the
lintian-tests pass. If you want me to remove it, please let me know.
> 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.
Since the proposed approach is far cleaner, I remove the explicit file
list and changed it to "Files: *"
>
>> 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.
After incorporating the changes requested by you, I think the package is
in a quite good shape already, don't you think so?
>
> 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
I added the gbp.conf with the content of other team's packages.
Manual package building is obviously far nicer now!
> 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.
>
I just copied the last build command from my shell history and didn't
consider, that it might be distracting/disturbing somehow. But I see
your point of course and verbalizing this is helpful as well.
> I forgot one small thing
>
> 7) d/source/local-options:
> This file contains two commented out lines, I suggest removing them if
> you're not gonna need it. Keeping it is also fine as long as it's a
> conscious choice done by you (not accidentally left there), let me
> know if that's the case.
>
> Thanks,
>
Removed the unnecessary file, which probably a left over of debmake.
> 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.
Great, I migrated the content from my original repository to
https://salsa.debian.org/pkg-security-team/time-decode
CI is up and running.
>
> Thank you for working on this package :)
Thank you for taking your time to review my packaging and provide in
depth feedback and explanations!
Best regards,
Jan
Reply to: