[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

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

> > 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.


>   @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

> 4. Remove debian/files; did you commit this file off after doing a dpkg-buildpackage? Are you not building in a clean chroot?


> 5. d/patches is empty, so purge this

> 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


>    + 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.

> 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

> > [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


Reply to: