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

Re: request for review: lbzip2-0.16rc1-1



[apologies for the lateness of this reply]

On Thu, Oct 22, 2009 at 7:00 AM, ERSEK Laszlo <lacos@caesar.elte.hu> 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


Reply to: