Re: RFS: lbzip2
Paul Wise wrote:
> And now onto the package review:
>
> Why does your diff.gz patch the Makefile? Shouldn't you add those
> changes to the upstream Makefile?
I don't think so. As my general, hobbyist free software development
policy, I *always* and *exclusively* follow the Single Unix Specification:
- the obsolete Version 1 if I want to give the package any chance to
compile on OpenVMS,
- the still valid Version 2 if I want to use threads and compile the
package on systems certified only UNIX 98 and not UNIX 03,
- the current Version 3 if I want my head to explode because all stuff
is optional now (as you see, this hasn't happened yet).
The standard I followed in case of lbzip2 is SUSv2.
So what I put into upstream must obey SUSv2, and I'll put nearly nothing
into upstream that is not covered by SUSv2.
One such thing would be the set of paths I'd to put under an "install:"
rule. This has clearly no place in Makefile.dev which is my personal
playground, or Makefile.portable, which is what it is called. (The
default Makefile, using gcc, is a concession towards the fact that most
people have gcc without an appropriate c89 wrapper.)
I simply couldn't satisfy all conceivable users with any "install:"
target. I believe there's a world out there that doesn't conform to the
FHS, for example users with install rights only in their respective
(freely structured) home directories. Maybe someone doesn't want to
install the manual at all. I just list the files one should consider
installing, in the README.
Apropos, the manual. Lbzip2 comes with full command line help; I only
created lbzip2.1 because Aníbal Monsalve Salazar told me to do so, and I
figured, first, it might not hurt, second, adding a manual as a Debian
patch would be gross; even though the man(7) groff macro package
lbzip2.1 is written with is nonportable (well, [tg]?roff itself is
nonportable). An "install:" target is different; first, it *might* hurt,
second, it's trivial to add by way of patch.
(I was forced to work around some Solaris and Tru64 bugs in 0.13, but
that concerned the *source*, and I could do those while staying
compatible with SUSv2.)
Installation is system-specific which I cannot handle, generally. I can
make an exception for Debian because I *use* Debian. However, this
exception doesn't end up in upstream (not considering some hints in the
README and said manual), it goes into .diff.gz. I have to think of the
BSD people, the proprietary unix users, etc. The more GNU and/or Debian
specific layers I put into upstream, the more they'll be annoyed. I put
Debian specific stuff into .diff.gz.
> About the manual page patch, would it be possible to make the upstream
> build process configurable so that you don't need to patch in the
> path?
As said above, I feel strongly that putting any paths into upstream is
not right. I could run sed and co. on lbzip2.1 from the Makefile, but
the strings I'd substitute in would be different for upstream and
Debian. (And not very beautiful strings.)
Why do you ask? Is it that the fixed paths in the patch should be
configurable? If .diff.gz patched upstream lbzip2.1 to include some
magic words, would dh_installman substitute those words?
> Shouldn't the upstream Makefile also install the manual page?
In my opinion, no; see above. If you're a mere user, you won't be happy
either if I create ~/man/man1 for you or if I fail because there is no
such directory. And do I have to tell you about MANPATH then? Does your
"man" know that variable?
If I let users pass the DESTDIR macro definition to make, as in
$ make DESTDIR=$HOME/installed install
somebody will get angry because he/she has a networked home and keeps
the binaries in $DESTDIR/<arch>/bin and the manuals in
$DESTDIR/share/man, and I couldn't prepare myself for his/her <arch>
subdir between $DESTDIR and bin.
Upstream is no place for pathnames, not even relative ones. Upstream
users are power users (they should be, at least). I'll happily obey your
instructions in composing .diff.
Please don't suggest autoconf & co as the next step! :)
> You are missing a Homepage field.
Added.
> You need ${misc:Depends} in your Depends line, in case one of the
> debhelper scripts you use starts adding something to it and you don't
> notice.
Added. I seem to remember that I manually removed it in the beginning
for some reason.
> noopt should add -O0 to the CFLAGS and nostrip is handled by dh_strip
> so you don't need to do it as long as you always make gcc put
> debugging symbols in.
It seems awkward to me to generate debugging symbols and then strip
them. Is there a reason to include debugging symbols per default? It can
eat up a lot of disk space (and thus buffer cache) for huge projects.
Also, -O0 is gcc's default, AFAIK.
Can you please explain the reason for these rules? (I'll accept "that's
the way we do it in Debian" too.) I created the flags stuff in
debian/rules so that the behavior matches the Policy Manual, "4.9.1
debian/rules and DEB_BUILD_OPTIONS"; all eight variations of
{ noopt, "" } x { nostrip, "" } x { parallel=k, "" }
worked when I last tested the DEB_BUILD_OPTIONS stuff.
The current state mirrors the fact that while the nostrip and noopt
DEB_BUILD_OPTIONS are orthogonal, the gcc options they result in are *not*:
- noopt, nostrip: -g3
- noopt : -s
- nostrip: -g3 -O2
- : -s -O3
Presence of nostrip means -g3, absence of nostrip means -s. Presence of
noopt means "", absence of noopt means -O2 in presence of nostrip and
-O3 otherwise. (I think I read somewhere that -O2 doesn't make symbolic
debugging impossible, but -O3 does.)
Anyway, now I add -O0 if noopt is passed.
> Shouldn't LIBS be something the upstream Makefile sets?
Yes, the upstream makefiles set up all necessary macros. But once I
patch the Makefile for Debian, I want to keep all macro definitions in
one place.
> Why do you install all the files in /usr/share/lbzip2 ?
You mean:
install -t $(DESTDIR)/usr/share/lbzip2 -m 644 -p corr-perf.sh \
malloc_trace.pl test.sh lfs.sh
1. As described in the README, "malloc_trace.pl" can be used to check
the memory allocation trace lbzip2 writes to stderr when invoked with
the "-t" option. (This is something a user might want to do, or
something I might ask a user to do.)
2. "test.sh" is the portable test script. A user might want to run it,
especially if I ask him to run it.
3. "lfs.sh" is misleadingly named after the "Large File Support"
acronym. In reality it selects a programming environment I deem useful
for lbzip2 at build time. Now, if you decide to test with "test.sh"
*and* have perl installed, "test.sh" will ask lbzip2 to write memory
allocation traces in the correctness tests phase and will check those
traces with "malloc_trace.pl". Now please consider the following:
- malloc(0) is a valid call, and is allowed by SUSv2 to return a null
pointer.
- free(0) is a valid call.
- realloc(0, x) is equivalent to malloc(x).
- The debugging allocation functions in lbzip2 use the %p printf format
specifier to print pointers.
Although I never call malloc(0), free(0), or realloc() (at all) in
lbzip2, "malloc_trace.pl" is a generic script of mine. It has to handle
such entries in the trace. So it needs to know how the %p printf format
specifier formats (void*)0. For this reason, "test.sh" compiles a small
temporary program just to get this string representation, and passes it
to "malloc_trace.pl". I need to compile this temporary program in the
same programming environment lbzip2 itself was compiled in, so I need to
call "lfs.sh" from test.sh.
The reason for the last sentence of the previous paragraph: suppose your
default programming environment is XBS5_LP64_OFF64 for whatever reason,
but your system also support XBS5_ILP32_OFFBIG, and thus lbzip2 will be
compiled in the latter (because I measured it to be faster, at least on
a Sparc). There is nothing to keep a libc from printing (void *)0 as
0x00000000 in XBS5_ILP32_OFFBIG and as 0x0000000000000000 in
XBS5_LP64_OFF64. If lbzip2 (or any traced program) prints it as
0x00000000, then "malloc_trace.pl" must recognize it the same way, and
the only way to achieve this is to pass the same options to the compiler
compiling the format-querying small program, and "lfs.sh" is the one
getting those compiler options from "getconf".
4. This leaves us with "corr-perf.sh". This script was my initial test
script. It is not portable, but after some hacking, a determined user
can run it if he wishes. Furthermore, the README contains a section
(from the time of lbzip2-0.06) that is based on the output of this script:
Performance (on "opteron", as of lbzip2-0.06)
---------------------------------------------
I don't consider either the README section or the "corr-perf.sh" script
useless as documentation.
> I'm not comfortable uploading this without a test suite being run
> during the package build process.
You're right. But.
If I add small test files (perhaps the ones from the upstream
bzip2-1.0.5 distribution), I'll only test a very small part of lbzip2
(basically no lock contention etc). If such a test passes, it proves
nothing more that lbzip2 is not fatally miscompiled and that I managed
to call the libbz2 API correctly a few times.
Large files I obviously cannot add. The user is encouraged to test with
"test.sh" on his/her own (hopefully huge) test file. "test.sh" starts
with a correctness phase. I don't want to create a false sense of
security. "corr-perf.sh" even contains ugly hacks for input draining /
output blocking (in order to give an idea, as documentation, at least),
three tiny files test almost nothing in comparison.
Please reaffirm that you want me to add test files. Then I'll put the
ones present in the upstream bzip2 package into the .diff (as base64
encoded files) and add the checks to the patched-in "install:" target.
> lintian -I -E --pedantic --show-overrides:
The lintian in etch doesn't know -E nor --pedantic. Even
mentors.debian.net called the .deb "lintian clean". Whatever,
> P: lbzip2 source: direct-changes-in-diff-but-no-patch-system Makefile and 1 more
I looked this up in "lintian-2.2.5/checks/patch-systems.desc". How do I
"keep the changes as separate patches under the control of a patch system"?
> P: lbzip2: copyright-refers-to-symlink-license usr/share/common-licenses/GPL
I (hope to have) fixed this by referring to
/usr/share/common-licenses/GPL-2, as lbzip2 is released under the GPLv2+.
> I: lbzip2: copyright-with-old-dh-make-debian-copyright
I fixed this by adding the Copyright word to the Debian packaging
copyright notice. I also changed "GPL, see above" into "GPLv2+, see above".
> I: lbzip2: binary-has-unneeded-section ./usr/bin/lbzip2 .comment
Are you suggesting that "gcc -s" doesn't strip hard enough, only
dh_strip does? Okay, I inserted dh_strip between dh_installman and
dh_compress, and cleaned up the -s from LDFLAGS.
> P: lbzip2: no-homepage-field
Fixed.
I re-uploaded the package, although
dh_gencontrol
dpkg-gencontrol: warning: unknown information field `C Homepage' in
input data in general section of control info file
dpkg-gencontrol: warning: unknown substitution variable ${misc:Depends}
Thanks,
lacos
Reply to: