Re: request for review: lbzip2-0.16rc1-1
On Thu, 22 Oct 2009, Paul Wise wrote:
Please note that you need to remove the patches after running make
clean, since you patch the Makefile.
(1) Is it acceptable if I make the "clean" target in debian/rules depend
on the "unpatch" target, which is defined by /usr/share/quilt/quilt.make
and seems to do the right thing?
(2) How do you make pbuilder build the package twice in a row?
I get some GCC warnings when I add -Wall to the CFLAGS, like most
Debian source packages have.
I develop with Makefile.dev:
CFLAGS=$$($(SHELL) lfs.sh CFLAGS) -D _XOPEN_SOURCE=500 -pipe -ansi -pedantic \
-fno-builtin -fhosted -Wall -Wextra -Wfloat-equal -Wundef -Wshadow \
-Wlarger-than-32767 -Wpointer-arith -Wbad-function-cast -Wcast-qual \
-Wcast-align -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes \
-Wmissing-declarations -Wredundant-decls -Wnested-externs -Wformat=2 \
-Wunreachable-code -Winline -Wlong-long -g3
Whatever warnings remain do so because I know about them and deem them
unimportant. I'd suppress them if I could *locally* (or perhaps not, even
so). The warnings you mention are probably:
- Unused parameters. In allocation callbacks for lacos_rbtree and libbz2
an opaque pointer is accepted. In lbzip2 I don't use them, so I named such
parameters "ignored".
- "condition always true/false due to limited range of data type", and as
a consequence to this, code blocks that "will never be executed". These
are sanity checks that protect against integer overflows before
multiplications and additions. If, for example, size_t is 64-bit wide and
unsigned int is 32-bit wide, then sometimes such checks can be proven
unconditionally true or false at compile time, hence the warnings. (And
the dependent block will never execute.) Many of these could be solved
nicer if the preprocessor knew about (size_t)-1, or <limits.h> defined
SIZE_MAX. (SSIZE_MAX is very different.)
I left -Wall out of Makefile and Makefile.portable on purpose.
(If you ask why three standalone makefiles: I can't use "include" in the
makefiles for the rules as "include" is not SUSv2.)
I personally think you could merge the Makefile patch upstream. I've
attached what I think the Makefile and rules file would look like if
you do that.
Thank you.
Ad upstream Makefile:
* -Wall: see above
* PREFIX=/usr/local:
I never install packages I compile from source under /usr/local as they
are effectively un-removable after installation (or I have to keep the
configured source tree for an eventual "make uninstall"). I always do a
variant of /opt/vendor/package, or simply $HOME/installed/package-version,
and I either extend my PATH, MANPATH, INFOPATH etc. or create symlinks. Of
course, you can do this by overriding PREFIX, but at packages this size I
always hand-pick what I install (or if the project uses autoconf and
stuff, I fix it up after "make install").
For example, "make install" often doesn't install the upstream README, and
I like to keep them (and others don't). So I rather give a description in
the README what's what and the user can choose. I don't feel like
torturing my brain with autoconf for a project this size (even less so as
I can't see any use in it, lbzip2 being coded against SUSv2).
I accept that most users don't *want* to make choices. This is why I'd
like if Debian provided lbzip2 for them, and I do strive to package as you
say.
* sanity check: test.sh does this and more, and is available through the
Makefile, and documented in the README.
Ad debian/rules:
If you want me to, (3) I can add -Wall, and also (4) let dh_installman
find lbzip2.1 on its own.
If you were to throw autotools into the mix, you could get rid of the
first hunk of the manual page patch too. For the second hunk, manual
pages can contain a BUGS section. I understand completely if you do not
want to adopt my changes.
autotools I've covered above. For the second hunk: the sentence referring
to the README's Bugs section is already in the manual page's BUGS section;
that's why the phrase ends with "for more on this": the verbiage I've put
under README/Bugs would be a bit too much for the manual, I believe.
I respectfully ask for your help with (1)-(4) so I can re-upload the
package.
Thanks a lot!
lacos
Reply to: