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
>work.
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
example:
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:
http://qa.debian.org/developer.php?login=Smedegaard&comaint=yes
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>