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

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!

Reply to: