Re: Bug#1004498: RFP: toml11 -- C++11 (or later) header-only toml parser/encoder
Hi Lance,
when re-reading Nilesh's mail I agree with all his points. I've fixed
several of them when reviewing.
Am Thu, Feb 03, 2022 at 07:51:33PM +0530 schrieb Nilesh Patra:
> On Thu, Feb 03, 2022 at 01:50:39PM +0000, Lance Lin wrote:
> > Hi Nilesh,
> >
> > On Wednesday, February 2nd, 2022 at 9:33 PM, Nilesh Patra <nilesh@debian.org> wrote:
> > Ok, I've tested the package and the only warning is an improbable bug number.
> > However, it is the correct bug number.
>
> I do not see that problem; since you see a probable bug number problem, most likely
> you are using an older version of lintian (before we had our on-million'th bug report)
> Please upgrade
+1
> > It is hosted on the debian med namespace [1].
> > Please review the work, but I think it should be complete.
>
> Okay, here's the review:
>
> 1. Naming: Since this is a header-only lib, IMO it can/should be named to libtoml11 instead
I agree. Please rename.
> 2. Branch structure is wrong
>
> + We follow dep-14 protocol which means there are three branches, master; upstream and pristine-tar. Please skim-through our
> policy[2] and dep-14 docs[3] should you find it useful
I've created those branches and like to stress that reading the
suggested links is a good advise.
> + Looks like you are not using gbp, using it can save you from these pitfalls.
+1
> @Andreas, can you link here to a recent packaging tutorial of yours, using gbp?
Lance confirmed he has seen at least parts of my DebConf workshop.
> 3. Remove debian/debhelper-build-stamp; that's the default debhelper behaviour, you don't need this
Done.
> 4. Remove debian/files; did you commit this file off after doing a dpkg-buildpackage? Are you not building in a clean chroot?
Done.
> 5. d/patches is empty, so purge this
Done.
> 6. d/rules:
>
> + "export DEB_CFLAGS_MAINT_APPEND = -Wall -pedantic" is un-needed, that'd get these by default
> + "export INSTALL_PREFIX = /usr/" -- I do not see it when I grep the code; so I think this is un-needed.
> Debhelper will take care of the prefix on its own, provided the upstream build does not override it
Fully ACK - please remove.
> 7. d/control:
>
> + Maintainer field should be 'Debian Med packaging team' along with our alioth list email; take a look at
> any package on our namespace's salsa
> + You should be mentioned in uploaders
Done.
> + Bump Standards-Version to 4.6.0
> + Add Rules-Requires-Root field
Both done
> + Binary package name is wrong, it should be libtoml11-dev; check library style packaging guide[4]
ACK. Please rename
> + Description seems a bit weird, you might want to reformat it
ACK. Also the wording is a bit monotone to read but we could possibly
live with this.
> 8. No d/upstream/metadata: github-debian-upstream script from pkg-js-tools can help create one
Done by using lintian-brush (called by routine-update)
> 9. No autopkgtests: Since you are familiar with these (you added it to a couple of packages) you might want to have
> it in here too.
ACK.
> Welcome, hope this helps. I hope Andreas can steer this discussion after this point; less time in the week :-)
Lance, it would be nice if you would work down such a list of todos as
created by Nilesh and ask for a second review afterwards. Please come
back here once you fixed the remaining items.
Please note that I bumped the upstream version to latest upstream when I
did the changes (partly by calling routine-update). Meanwhile I learned
that the package uncalled is featuring a code copy of toml11 so I see a
good reason to maintain it in Debian Med team.
Kind regards
Andreas.
> > [1] https://salsa.debian.org/med-team/toml11
> [2]: https://med-team.pages.debian.net/policy/#to-create-a-new-local-git-repository
> [3]: https://dep-team.pages.debian.net/deps/dep14/
> [4]: https://www.netfort.gr.jp/~dancer/column/libpkg-guide/libpkg-guide.html
>
> Regards,
> Nilesh
--
http://fam-tille.de
Reply to: