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

[Rémi Thebault]

Hello

Le samedi 25 décembre 2010 à 22:18 +0100, Jonas Smedegaard a écrit :

> There are a few trailing spaces that should be removed.

Done


> I suggest to merge [waf-vars.mk] with the main waf.mk file

Done



> I recommend to support global, package-default and per-package vars

I use $(cdbs_curdestdir) in common-install-impl target. I tried
something with calling cdbs_expand_curvar, but no success.
the  var isn't expanded as I expected. I commented out for the moment


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

Done



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

building and staging works, but cleaning let some files in the debian
dir (the stage is one of them)
and the binary deb file isn't built without debhelper.mk. Is this
behaviour desired ?



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

Done



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

Done. I created a new target (cdbs-waf-sanity-check) upon which depends
configure and clean targets


> 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

Do you mean keeping track of a checksum in cdbs source code ?
If yes, it means we must keep an eye on every waf new release. I don't
think it could work on long term.


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

This would be ideal, but the Waf user guide itself discourage a
system-wide installation and advice to ship it in the tarball source
tree



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

Done




> Please add yourself as "Uploader" in debian/control.in.

Done



> That's it.  Can't find any more to complain about just now :-)
> 
> 
>   - Jonas
> 
> _______________________________________________
> Build-common-hackers mailing list
> Build-common-hackers at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/build-common-hackers


Rémi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/build-common-hackers/attachments/20101227/9266e4a0/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 316 bytes
Desc: This is a digitally signed message part
URL: <http://lists.alioth.debian.org/pipermail/build-common-hackers/attachments/20101227/9266e4a0/attachment-0001.pgp>