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