Bug#668966: Re: RFS: dparser-1.16-1 [ITP] -- a scannerless GLR parser generator
X-Debbugs-Cc: jwilk@debian.org
X-Debbugs-Cc: debian-python@lists.debian.org
Hello Jakub,
On 04/16/2012 11:35 PM, Jakub Wilk wrote:
> I don't intend to sponsor this package, but here's my review:
Thanks again for the review. I've adjusted 1.26-1 and uploaded a
corrected variant to mentors.debian.org. All changes are documented
inline below.
> base-makefile-fixes.patch removes this line: LIBS += -lm But this is
> explained neither in the patch description nor in the changelog.
Hm.. I removed it to avoid a warning about a useless dependency.
Arguably, I'm not sure this works on all platforms, but a quick check at
least revealed that math.h isn't included anywhere (from the source
tarball).
> The fix-python-makefile patch will break if Python version is longer
> than 3 characters. (I know, unlikely, but it still bothers me. ;P)
> You could query distutils directly for the build directory using the
> following code:
>
> python -c 'from distutils.command.build import build; from
> distutils.core import Distribution; b = build(Distribution());
> b.finalize_options(); print b.build_platlib'
Thanks, that looks much less prone to error, yes. Replaced.
> More importantly, the fix-python-makefile patch violates Policy
> §4.6.
Agreed. I think I fixed that now: all unit tests are concatenated with
'&&' and a proper result line gives a status hint.
Additionally, the Makefile now checks if the above python code returned
an existing directory and emits a more helpful error before even trying
to run the unit tests (instead of just failing on them).
> Oh, and please don't add commented-out code, thanks.
That slipped my attention, sorry. Cleaned up.
> Have you forwarded the manpage-hyphen-correction patch upstream?
Yeah, I forwarded all of the patches and a notification. Unfortunately,
I didn't hear back from the upstream author, yet. I'll ping him, again.
> Why priority extra? I'd use optional.
Agreed. "Extra" sounded optional enough to me, so I didn't bother
checking. Did some RTFM and degraded to "optional".
> I'd rather not use ${python:Provides}. See:
> http://lists.debian.org/20110324164804.GA5919@jwilk.net
Okay, sounds reasonable, removed. (I have to admit I didn't gain a
thorough understanding of this issue, yet).
> In debian/copyright, you need to either add newlines (escaped by
> dots) between list items or indent the whole list by an extra space.
> (License uses the same rules as Description in debian/control; see
> Policy §5.6.13 for details.)
Oh, I see, this gets word-wrapped differently, if I don't add newlines
between the points. Fixed.
> Please honour DEB_BUILD_OPTIONS=nocheck.
>
> Please honour DEB_BUILD_OPTIONS=noopt.
I naively expected debhelper 3 to take care of these things. Both should
be respected, now. The later by using dpkg-buildflags, which also helps
with hardening options.
(I tried, but couldn't figure out how to enable parallel builds, yet.
But that looks less important that the above two).
> This part of upstream makefile:
>
> ifeq ($(ARCH),x86_64) CFLAGS += -fPIC endif
>
> smells like a violation of Policy §10.2.
Yeah, dropped that in favor of always adding -fPIC. I didn't have a
chance to test on anything other than x86_64, though.
> The package fails to build in a minimal environment:
>
> python2.6 setup.py build make[2]: python2.6: Command not found
> make[2]: *** [all] Error 127
I started using pbuilder, which allowed me to reproduce that, yes.
First of all, I had the value "python-all-dev" under "Section" instead
of "Build-Depends" (?!?). I corrected that and put python-dparser in
section "python" and made it build-depend on python-all-dev. Running
this under pbuilder now looks fine to me.
> I see lots of make[3]: svnversion: Command not found in the build
> log. Is that intentional?
Well, the upstream makefile is trying to determine the SCM version the
build is originating from. I now replaced that with simply reading the
revision from file BUILD_VERSION, which is the provided fall-back. To
me, that seems saner for a Debian package.
> What is debian/dparser-doc.install for?
No idea where I got that from; dropped.
> Version declared in setup.py is 1.9. Shouldn't that be 1.26?
Good catch. Corrected (in yet another patch).
Jakub explicitly stated he doesn't intend to sponsor this package, so
I'm still looking for one.
Regards
Markus Wanner
Reply to: