[Build-common-hackers] a WAF class for CDBS

Jonas Smedegaard dr at jones.dk
Sat Dec 25 21:18:35 UTC 2010

On Wed, Dec 22, 2010 at 12:08:11AM +0100, Rémi Thebault wrote:
>I'd be glad to commit through git by myself, but I can't get it to 

I noticed you succeeded to upload.  Welcome aboard! :-D

And you added some documentation to Docbook. Cool!

I have a few comments about the new snippets:

There are a few trailing spaces that should be removed.  If you enable 
colored output in git they are easy to spot.  Something like this:

   git config --global color.ui auto
   git log -p

You provide a separate waf-vars.mk.  That's similar to other snippets, 
but still I suggest to merge with the main waf.mk file: Separation is 
useful when same config is shared across multiple classes or when the 
main class file is reused but need to override the config part.  I don't 
expect us to need any of that for waf - and if it turns out later that 
we do, it is ease to separate then (but difficult to merge later, as we 
can't know if anyone has started rely explicitly on the config snippet).

I recommend to support global, package-default and per-package vars, 
even if not yet supporting per-package builts yet - that makes it easier 
to add later on.  See the recent change to python-distutils.mk for an 

   git log -p 0900cb^..0900cb

Instead of explicitly touching stampfiles, it is more elegant to do 
"touch $@" (expands to current build target so essentially do the same).

You mention that to use it needs including debhelper.mk.  That's not 
true - it works perfectly fine without CDBS'ifying debhelper too.

Please keep comment lines below 72 chars.  Code lines are ok to be 
longer if necessary.

I dislike your "define" contruct.  It is resolved early like ifeq 
constructs, which is generally problematic as is force a specific order 
of snippet inclusions etc.  This particular one does no harm but I would 
prefer to avoid it anyway for the sake of discouraging those - and it 
seems to me that it could just as well be done as a simple variable:

   checkwaf = test -f ./waf -a -x ./waf

Oh, and there's another problem in that you use a short variable name 
with a high risk of clashing with user code.  Better to always include a 
leading "cdbs_" and also include a snippet-specific keyword to avoid 
clashing with other parts of CDBS.  Like this:

   cdbs_waf_checkwaf = test -f ./waf -a -x ./waf

...but really it is more elegant IMO a) do the checks separately (the -a 
might not be POSIX compliant although in recent Debian this restraint 
might be relaxed) and b) only check in initial targets.  Like this:

   # ensure waf is executable
   pre-build clean::
           test -f ./waf
           test -x ./waf

As a sidenote, I really dislike the fundamental design of waf: Shipping 
a self-extracting scripting environment, obfuscated by an odd format, 
and telling users to execute that code blindly.  It is a security risk!

We really should extend the initial check to e.g. keep a checksum and 
whenever the waf file do not match the checksum (including at initial 
packaging when it hasn't been calculated yet) we instead just unpack the 
waf and fail with a message informing a) the checksum and b) that the 
packager needs to manually proof-read the unpacked waf content and check 
that nothing malicious is there.

Much better would be a separately packaged waf tool.  This has been 
tried, but upstream obstructed that to the extend that it has now been 
dropped from Debian.

Instead of only mentioning in documentation that waf needs Python, it is 
more elegant to support build-dependency auto-resolving.  Look at 
CDBS_BUILD_DEPENDS* vars in many of the other snippets, and (as with 
everything else) just ask if it isn't obvious how to do it.

Please commit code changes separately from changes to changelog file.  
This eases potential later reverting or cherry-picking across branches. 
Also, the changelog file can be (semi-)automatically updated later using 
"git dch -a", so try to kep in mind to phrase (first line of) the commit 
messages in same style as would make sense showing up in the Debian 
changelog.  Not a big deal if not perfect, though - I always proof-read 
the changelog before releasing (and often do some nitpicking changes 
like adding a comma or some such anyways...) :-)

Please add yourself as "Uploader" in debian/control.in.  Even if you 
technically cannot upload to Debian (i.e. if you are not an official 
Debian Developer) this still helps as an indicator of you being an 
active contributor.  It also means you can track the "health" of all 
packages that you are involved in in Debian, on a page like this:


That's it.  Can't find any more to complain about just now :-)

  - Jonas

  * Jonas Smedegaard - idealist & Internet-arkitekt
  * Tlf.: +45 40843136  Website: http://dr.jones.dk/

  [x] quote me freely  [ ] ask before reusing  [ ] keep private
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/build-common-hackers/attachments/20101225/39c870a2/attachment.pgp>

More information about the Build-common-hackers mailing list