On Tue, Mar 29, 2011 at 12:44:54AM +0800, Chow Loong Jin wrote: > On Monday 28,March,2011 10:46 PM, Peter Pentchev wrote: > > [...] > > Hi, > > Hi Peter, > > Thanks for your patches. I've looked through them, and I think I'll accept just > the second one (regarding --watchfile). Comments are interleaved below:- > > > Bearing in mind that I'm not a DD yet and cannot really help you by > > uploading the package, what do you think about the attached four patches > > that IMHO might improve the packaging a bit further? > > > > - get the CPPFLAGS, CFLAGS and LDFLAGS variables from the dpkg-buildflags > > tool introduced in dpkg-dev 1.15.7 > > Shouldn't these be automatically exported before the build process? At least, I > believe this is what has been used previously. Was there anything wrong with that? The whole point of introducing dpkg-buildflags is to allow the rules file to behave the same way no matter how it has been invoked. In most cases, it will be invoked by dpkg-buildpackage (i.e. the build process will have some other tool invoke dpkg-buildpackage, as debuild, svn-buildpackage, git-buildpackage, pdebuild, etc. all do), in which case the rules file will have the *FLAGS in its environment. However, sometimes it is convenient - at least for me, and apparently for others, too, since I've seen this discussed on the lists - to do something like "debian/rules configure", then "debian/rules build", see the build fail, fix a little thing, run "debian/rules build" again (don't want to go through all the configure stuff, no changes there for the tiny fix I've made), see it fail, fix something else, "debian/rules build" again... And when I think I've fixed it completely, "debuild clean && debuild". In this case, dpkg-buildpackage is not invoked at all until the very final step, when debuild runs it; so when I run debian/rules by hand, it will inherit whatever *FLAGS are in my shell environment - not quite what is intended, I believe :) There have been some discussions on various Debian lists about that, and dpkg-buildflags has been one of the results :) Of course, its goal was also to cater to distribution-specific flags; ISTR an outline of three different origins for build flags - three different types of flags to come - described in an e-mail message, but I can't find it right now :( Still, of course, it's your package, your decision :) > > - properly pass --watchfile and not --watchfie to uscan ;) > Applied and uploaded to mentors.debian.net, thanks. > > > - no need to pass the changelog name to dh_installchangelogs since 7.0 > Well yeah, but it still didn't detect the ChangeLog, for some reason, so I added > it there. I should probably debug this issue and file a bug on debhelper. Hm, it worked just fine for me - I tested the patch before sending it :) Try running it with DH_VERBOSE=1 in the environment to see what dh_installchangelogs will really do. > > - bump the debhelper compatibility level to 8 with no further changes > If there's no reason for it, so I'd rather not bump the compat level (and the > version of the debhelper build-dep) unnecessarily. Well, for this particular package there is really no difference, although I personally like the new stricter handling and - for other packages - the dpkg-gensymbols invocation. Thanks for taking the time to consider my suggestions! :) G'luck, Peter -- Peter Pentchev roam@ringlet.net roam@FreeBSD.org peter@packetscale.com PGP key: http://people.FreeBSD.org/~roam/roam.key.asc Key fingerprint FDBA FD79 C26F 3C51 C95E DF9E ED18 B68D 1619 4553 If you think this sentence is confusing, then change one pig.
Attachment:
signature.asc
Description: Digital signature