request for review: lbzip2-0.16rc1-1

[ERSEK Laszlo-2]

Dear Mentors,

I released a new upstream version of lbzip2. It is a beta (or release
candidate). I refreshed the Debian package under [0], built in a sid
pbuilder; lintian produced no warnings.


  lbzip2 (0.16rc1-1) unstable; urgency=low
  .
    * New upstream release (candidate):
      - (Mostly) bzip2-compatible command-line, e.g. settable compression block
        size, file operands etc. However, lbzip2 never deletes or overwrites
        files it didn't create.
      - Merged eglibc getconf bug workaround (patches/eglibc-getconf) from
        0.15-2.
    * Changed the "bzip2 filter program" substring in the short description to
      "bzip2 utility".
    * Adapted "patches/man-inst-paths" to new upstream manual page.


I kindly request you to review it and upload it if appropriate. I'd be
grateful if valiant squeeze users would be able and willing to test the
RC, perhaps even as a bzip2 substitute.

Thank you very much,
lacos
(deb-0_16rc1-1-try-01)


[0] http://mentors.debian.net/debian/pool/main/l/lbzip2/


--
To UNSUBSCRIBE, email to debian-mentors-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Paul Wise-3]

There is one lintian warning:

I: lbzip2 source: quilt-patch-missing-description makefile

The package fails to unapply the debian patches in the clean rule. As
a result, some files get embedded in the diff.gz when built several
times in a row:

lbzip2-0.16rc1/.pc/applied-patches
lbzip2-0.16rc1/.pc/.version
lbzip2-0.16rc1/.pc/man-inst-paths/lbzip2.1
lbzip2-0.16rc1/.pc/makefile/Makefile

Please note that you need to remove the patches after running make
clean, since you patch the Makefile.

I get some GCC warnings when I add -Wall to the CFLAGS, like most
Debian source packages have.

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. The changes I've made make it feel a bit more like an
automake project. 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.

--
bye,
pabs

http://wiki.debian.org/PaulWise

[ERSEK Laszlo-2]

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


--
To UNSUBSCRIBE, email to debian-mentors-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[ERSEK Laszlo-2]

On Thu, 22 Oct 2009, ERSEK Laszlo wrote:

> (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?

Or rather, as the (new) last action of the "clean" target, invoke

         $(MAKE) unpatch

So that the inverse of "patch, then build" is correctly ordered as "clean,
then unpatch"; especially because the Makefile and its "clean" target are
patched as well.

Thanks,
lacos


--
To UNSUBSCRIBE, email to debian-mentors-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[ERSEK Laszlo-2]

Dear Mentors,

I kindly request you to review lbzip2-0.16-1:

http://mentors.debian.net/debian/pool/main/l/lbzip2

Changes:

  lbzip2 (0.16-1) unstable; urgency=low
  .
    * New upstream release:
      - handle signals and problems with input/output files more gracefully,
      - close standard output after (de)compressing to it from file operands.
    * debian/rules: unpatch the source as the last step of the "clean" target, so
      that the package can be built more than once in a row.
    * debian/changelog, debian/control, debian/copyright: refresh e-mail address.

lintian displayed one warning:

W: lbzip2: latest-debian-changelog-entry-without-new-version
N:
N:    The latest Debian changelog entry has a version number that's either the
N:    same or smaller than the version number of the entry before.
N:
N:    Severity: normal, Certainty: certain
N:

0.16rc1 precedes 0.16 chronologicallly.

Thank you,
lacos


--
To UNSUBSCRIBE, email to debian-mentors-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Ruben Molina-2]

Hi lacos,

> I kindly request you to review lbzip2-0.16-1:

Just a comment...

> W: lbzip2: latest-debian-changelog-entry-without-new-version
> 0.16rc1 precedes 0.16 chronologicallly.

You should have used something like 0.16~rc1 for the previous release.
See the --compare-versions option in dpkg for the next time.

I think you can't upload this new release just as 0.16, maybe you should
use 0.16rc2 to avoid an epoch...

Regards,
  ruben


--
To UNSUBSCRIBE, email to debian-mentors-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[ERSEK Laszlo-2]

On Sun, 25 Oct 2009, Ruben Molina wrote:

>> W: lbzip2: latest-debian-changelog-entry-without-new-version
>> 0.16rc1 precedes 0.16 chronologicallly.
>
> You should have used something like 0.16~rc1 for the previous release.
> See the --compare-versions option in dpkg for the next time.

Thanks.


> I think you can't upload this new release just as 0.16, maybe you should
> use 0.16rc2 to avoid an epoch...

I was thinking about 0.16z; I figured I could rename the source root to
"lbzip2-0.16z" (instead of "lbzip2-0.16") from the upstream "lbzip2" and
continue packaging as usual. In that case, however, "lbzip2 --version" and
the Debian package would have stated different versions.

I think I'll just change the previous changelog entry to 0.16~rc1-1, since
that package wasn't uploaded.

$ dpkg --compare-versions 0.16~rc1-1 '<' 0.16-1 && echo OK
OK

Thank you,
lacos


--
To UNSUBSCRIBE, email to debian-mentors-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[ERSEK Laszlo-2]

On Sun, 25 Oct 2009, Ruben Molina wrote:

>> W: lbzip2: latest-debian-changelog-entry-without-new-version
>> 0.16rc1 precedes 0.16 chronologicallly.
>
> You should have used something like 0.16~rc1 for the previous release.
> See the --compare-versions option in dpkg for the next time.

Fixed,

http://mentors.debian.net/debian/pool/main/l/lbzip2

    * Replace version string of previous changelog entry ("0.16rc1-1") with
      "0.16~rc1-1" for correct ordering; thanks to Ruben Molina in
      <6d57146c1ed5d6a38b1320ab63f163c7.squirrel@...>.

(As said before, 0.16rc1 was not uploaded.)

Thank you very much,
lacos
deb-0_16-1-try-02


--
To UNSUBSCRIBE, email to debian-mentors-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Paul Wise-3]

[apologies for the lateness of this reply]

On Thu, Oct 22, 2009 at 7:00 AM, ERSEK Laszlo <lacos@...> wrote:

> (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?

No, then you get unpatch running before clean and clean not removing everything.

> Or rather, as the (new) last action of the "clean" target, invoke
>
>        $(MAKE) unpatch
>
> So that the inverse of "patch, then build" is correctly ordered as "clean,
> then unpatch"; especially because the Makefile and its "clean" target are
> patched as well.

That would be correct.

> (2) How do you make pbuilder build the package twice in a row?

I did that outside pbuilder with debuild. You could do it in pbuilder
with a hook probably.

> 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:

Mainly I was concerned about the "may be used uninitialized in this
function" warnings. I guess those are invalid though?

> 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").

The point is, PREFIX=/usr is out of bounds for stuff being installed
from source. PREFIX=/usr/local is the right default and there is no
other appropriate default for upstream build systems. Personally I use
~/opt for stuff installed from source and make a point of regularly
doing rm -rf ~/opt/* and preferring packages where possible. In any
case, GNU stow is helpful if you do use /usr/local. checkinstall is
another useful tool for this use-case (installing from source but
making it easy to remove stuff). That said, this is only an issue if
you merge the makefile patch upstream since Debian packages should use
/usr as a prefix.

> 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).

Fair enough.

> * sanity check: test.sh does this and more, and is available through the
> Makefile, and documented in the README.

The point was to provide both check variants in the upstream Makefile
so that distros can decide to run the simpler one when appropriate and
you can run the full test suite when making a new release.

BTW, I just ran the test suite and got this message a few times from
p7zip (4.58):

Error:
Reading archives from stdin is not implemented
Command exited with non-zero status 7

> 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.

Please do (3) in the next upload.

I didn't think it was possible to do (4). Anyway, that was about
letting the upstream Makefile install the manual page so that other
distros packaging lbzip2 don't need to do anything to get the manual
page install.

> 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.

Ok.

Anyways, no more blocking issues remain. I was uploading 0.16-1 and it
got aborted by an accidental ^C. Something is going on with the upload
queue so it might be a while before my dcut is processed and I can
re-upload.

ftp://ftp-master.debian.org/pub/UploadQueue/

Some more comments though:

Usually changelog entries that have been uploaded to Debian are not
modified, I refer here to the email changes. No a big deal and no need
to revert, just the standard practice.

You may want to add descriptive headers to the two patches as
suggested by lintian[1]. A proposal for a standardised format for that
is in discussion[2].

Thanks for your contribution to Debian and for working on free
software compression parallelism!

1. lintian --info --display-info --display-experimental --pedantic
--show-overrides --checksums --color auto
2. http://dep.debian.net/deps/dep3/

--
bye,
pabs

http://wiki.debian.org/PaulWise


--
To UNSUBSCRIBE, email to debian-mentors-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[ERSEK Laszlo-2]

On Thu, 29 Oct 2009, Paul Wise wrote:

> [apologies for the lateness of this reply]

Still too early :), I just noticed I'll have to fix the debian-sanity
target, because in lbzip2-0.16rc1, the set of recognized environment
variables changed.


> Mainly I was concerned about the "may be used uninitialized in this
> function" warnings. I guess those are invalid though?

Why thank you, I'm pleased to announce lbzip2-0.17. See below.


     main.c:709: warning: 'save_errno' may be used uninitialized in this function

This is invalid, see (0 == ret).


     lbunzip2.c:1476: warning: 'reord_needed.last_decompr' may be used unnitialized in this function
     lbunzip2.c:1402: note: 'reord_needed.last_decompr' was declared here
     lbunzip2.c:1476: warning: 'reord_needed.w2w_blk_id.last_bzip2' may be used uninitialized in this function
     lbunzip2.c:1402: note: 'reord_needed.w2w_blk_id.last_bzip2' was declared here

This is a portability bug in lbzip2, thank you for finding it. The gcc
version in lenny didn't report these warnings. The offending fields are
never accessed explicitly, so there is no harm on a "friendly" platform.
However, strictly adhereing to the C standard, the assignment on line 1476
might access a trap representation and lead to undefined behavior. I was
able to fix this bug by creating a dedicated structure without the
offending fields. I've put your name in the README and the manual.


     lbunzip2.c:582: warning: 'save_bitbuf' may be used uninitialized in this function

This is invalid, see (0u < ub).


     lbunzip2.c:1071: warning: 'ibitbuf' may be used uninitialized in this function
     lbunzip2.c:1071: note: 'ibitbuf' was declared here

This is invalid, see "ibits_left = 0u" and (0 == ibits_left).


     lbunzip2.c:1083: warning: 'rbitbuf' may be used uninitialized in this function
     lbunzip2.c:1083: note: 'rbitbuf' was declared here

This is invalid, see the (IST_IN_BZIP2 == istate) around the rbitbuf read
accesses and the only "istate = IST_IN_BZIP2" assignment following
"rbitbuf = 0u".


     lacos_rbtree.c:157: warning: 'cmp_res' may be used uninitialized in this function

This is invalid. The first read access occurs in the loop, but to get
there, an assignment has to happen first in the loop test. The second
access depends on "parent" being non-NULL. "parent" starts out NULL; the
only place where it can change to non-NULL is the loop body, but to get
there... see first case.


> BTW, I just ran the test suite and got this message a few times from
> p7zip (4.58):
>
> Error:
> Reading archives from stdin is not implemented
> Command exited with non-zero status 7

This is expected and handled; test.sh allows tested compressors to refuse
test cases.


> 1. lintian --info --display-info --display-experimental --pedantic
> --show-overrides --checksums --color auto

That's useful, thanks. Now that I executed lintian like this, it was
silent.

http://mentors.debian.net/debian/pool/main/l/lbzip2

Changes:
  lbzip2 (0.17-1) unstable; urgency=low
  .
    * New upstream release fixes possible undefined behavior, reported by Paul
      Wise in http://lists.debian.org/debian-mentors/2009/10/msg00470.html.
    * Adapt the "debian-sanity" target to the new environment variables
      introduced by lbzip2-0.16rc1.
    * Following http://lists.debian.org/debian-mentors/2009/10/msg00470.html:
      - Add -Wall to CFLAGS.
      - Add descriptive headers to the makefile and manual page patches.

Please review it :)


Thank you very much for helping newbies (me among them) with such
endurance and responsiveness!

Cheers,
lacos
deb-0_17-1-try-01


--
To UNSUBSCRIBE, email to debian-mentors-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[Paul Wise-3]

Sorry about the lateness of my reply.

Reviewed and uploaded.

Not sure if you've used zzuf before, but you might want to use it to
discover possible bugs that could lead to crashes/etc in lbzip2.

helgrind (and the other valgrind tools) might be useful to find bugs:

http://valgrind.org/info/tools.html#helgrind

--
bye,
pabs

http://wiki.debian.org/PaulWise


--
To UNSUBSCRIBE, email to debian-mentors-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...

[ERSEK Laszlo-2]

On Wed, 4 Nov 2009, Paul Wise wrote:

> Reviewed and uploaded.

Thank you!


> Not sure if you've used zzuf before, but you might want to use it to
> discover possible bugs that could lead to crashes/etc in lbzip2.

Thanks for the tip. I didn't use zzuf, but I did do fuzz testing, as early
as 0.02. I dedicated a whole section to this in the README ("Decompression
robustness"). Also, search for "lbzip2" under [0].


> helgrind (and the other valgrind tools) might be useful to find bugs:
>
> http://valgrind.org/info/tools.html#helgrind

I've used several valgrind tools for testing (no bugs were reported).

A side note: as written in [1], "7.5. Hints and Tips for Effective Use of
Helgrind":

     3. Avoid POSIX condition variables.

Condition variables are at the core of lbzip2. I published an article on
this topic in the form of a reddit self-post, see [2].

Thanks again!
lacos

[0] http://www.cert.fi/haavoittuvuudet/joint-advisory-archive-formats.html
[1] http://valgrind.org/docs/manual/hg-manual.html#hg-manual.effective-use
[2] http://www.reddit.com/r/programming/comments/9ynxv/utter_verbiage_how_to_design_condition_variables/


--
To UNSUBSCRIBE, email to debian-mentors-REQUEST@...
with a subject of "unsubscribe". Trouble? Contact listmaster@...