Bug#822722: RFS: tldp/0.7.7 [ITP #822181]
Hello again Gianfranco (and lots of cc:),
>Hi Martin, lets pick up this package!
Excellent! Thank you very much for the help.
>First a general note: I'm truly impressed about your work and your learning curve!
Merci beaucoup! Danke! Grazie!
>Don't be sad for my review, the package works, so I guess the
>review is about minor issues and nitpicks!
Yes, completely understood.
>http://mentors.debian.net
OK, I created an identity there.
>usually you can do this way:
>
>maintain an upstream "master" branch without debian directory
>
>maintain a debian branch with the debian packaging and the upstream one
>
>git merge master when a new release is out in debian branch, and
>dpkg-buildpackage it (using the upstream tarball to be sure there
>aren't unwanted changes).
>
>or gbp buildpackage -S -sa --git-debian-branch=debian
>
>this will avoid having debian stuff in the upstream tarball, and
>letting you develop debian/specific fixes without having to release
>a new tarball
OK, I'll give that a stab. Do you have any example packages you
know that use this technique already? I could then use a working
one to ensure that I'm using a good model.
>and now the rest of the story
>(I'm not sure, probably in the last release the native/quilt stuff is fixed)
>1) debian/control:
>Depends: ${misc:Depends},
>${python3:Depends},
>python3,
>python3-networkx
>I would drop python3 and python3-networkx, because they should be
>picked up at build time by the python3:Depends variable (being
>listed in install_requires).
>
>can you please remove them and try to see in the built .deb file if
>they are picked at runtime correctly? (just open the deb file and
>look debian/CONTROL file or whatever is called nowadays)
Removed. Dependency added correctly by tools. Great! I probably
added the python3{-network,} dependency before learning about the
automated dependency detection.
>- please consider adding the Python2 or a tldp binary package
>
>2) debian/rules
>-b man -D today="$(BUILD_DATE)" \
>
>
>"BUILD_DATE" if it works wrt reproducible builds, then thanks for teaching me something new!
>I used to dpkg-parsechangelog to get a timestamp date
Uh, well, maybe I did not do that correctly, I do not know. Please
don't trust that I know how that interacts with the rest of the
system. I used an example from the tox package so that the manpage
was generated during build, rather than using the checked-in
ldptool.1 file.
>"rm -rf -- ./$(PYBUILD_NAME).egg-info"
>
>I think this should be done automatically by the clean target.
>
>I would just remove the _build and nothing more.
I added this .egg-info removal line because the directory did remain
even after 'debclean', That meant that a second build from the same
directory would fail.
>3) lintian override: you can keep it if you want, as you wish
If it doesn't matter, then I'd just as soon remove the extra file.
So, I have removed the lintian.overrides.
>4) copyright:
>
>this is the tricky part as usual :)
>
>please use the Debian packages instead of embedding external libraries
>(I didn't check the below)
>collateindex.pl
>docbook-dsssl: /usr/bin/collateindex.pl
>docbook-dsssl: /usr/share/man/man1/collateindex.pl.1.gz
>
>apt-file search ldp.dsl
>ldp-docbook-dsssl: /usr/share/sgml/docbook/stylesheet/dsssl/ldp/ldp.dsl
>
>apt-file search tldp-one-page.xsl (and the others)
>ldp-docbook-xsl: /usr/share/xml/docbook/stylesheet/ldp/html/tldp-one-page.xsl
>
>can't you just use them from the system?
Not really. Not all systems have these files. And, some that have
been distributed are different or broken. But!
The short version is that all of these files are used only for the
testing suite and are, therefore, included in the release tarball,
but are not shipped with the software.
In the installed package, yes, the 'ldptool' software would rely on:
ldp-docbook-dsssl
ldp-docbook-xsl
docbook-dsssl
Oh, and yes, that means I should add all of the other dependencies
so that 'ldptool' works. I forgot about that step.
>In case they differ, how about proposing patches to the actual
>Debian tools?
As soon as I figure out how to get one package accepted, I plan on
overhauling the ldp-docbook-{dsssl,xsl} packages for Debian.
>embedded stuff needs to be avoided as much as possible.
Is it considered embedded if it is in the source tarball, but not in
the binary package?
># Files: extras/dsssl/* extras/css/*
># Copyright: 2000-2003 - Greg Ferguson (gferg@metalab.unc.edu)
># License: GPL-2.0+
>#
>Files: extras/dsssl/* extras/xsl/* extras/css/*
>Copyright: 2000-2002 - David Horton (dhorton@speakeasy.net)
>License: GFDL-1.2
>
>this is conflicting even if commented (but using debian stuff, and
>removing them from the tarball will fix automagically the issue)
>
>also commented stuff in copyright should be avoided to me
>sample-documents: aren't on Debian? what about packaging them?
This is utterly fabricated test data to exercise the test suite of
the software.
Perhaps I can simply remove all of the commented out bits of the
debian/copyright file, if you can confirm that the copyright
information is only required for the binary package.
I assumed at first that this was required for every single file in
the source package. I think I have inferred in the last few days
(osmotically) that this is only necessary for the files in the
binary package. Is that true?
>why the license for them is commented?
>https://codesearch.debian.net/results/License:%20GFDL-1.2/page_0
>please try to add a copy of the license
>
>the other stuff looks good, let me know how you intend to proceed
>with the above issues, and I'll happily to another review
Thank you.
>automatic checks (check-all-the-things package)
>$ codespell --quiet-level=3
>./extras/collateindex.pl:643: thier ==> their
>
>$ fdupes -q -r . | grep -vE '/(\.(git|svn|bzr|hg|sgdrawer)|_(darcs|FOSSIL_)|CVS)(/|$)' | cat -s
>
>$ grep -Er '/(home|srv|opt)(\W|$)' .
>
>$ pep8 --ignore W191 .
>
>$ perlcritic -1 . 2>&1 | grep -vF 'No perl files were found.'
>
>$ pyflakes .
>
>$ pyflakes3 .
>
>$ grep -riE 'fixme|todo|hack|xxx' .
>
>$ grep -r '/tmp/' .
>
>$ Please add some upstream metadata: https://wiki.debian.org/UpstreamMetadata
>
>$ find -type f -iname '*.xml' -exec xmllint --noout --nonet {} +
>note: some of them are just nitpicks, and some others might be nice to fix, I leave checks to you
>cheers,
I found 'check-all-the-things' in experimental.
I ran pyflakes and fixed a few minor things that I missed on my last
minor code update.
Then, I ran check-all-the-things and ...well, there's a bit to read
there. So, I'll have a look at what it is suggesting.
-Martin
--
Martin A. Brown
http://linux-ip.net/
Reply to: