Hello, On trečiadienis 15 Birželis 2011 00:02:18 Joey Hess wrote: > So, my only interest now WRT this bug is to support dpkg-buildflags in > dh and dh_auto_* in a generic manner, and to support > DEB_BUILD_OPTIONS=noopt as best possible. > > Since dpkg-buildflags could be used to set *any* variable, it does not > seem to make sense to add the build-system specific settings like > "./configure CFLAGS=foo", as Marcelo earlier prototyped. Avoiding that > build-system specific stuff and only setting environment variables (that > are not already set)
I have some doubts about "(that are not already set)" part. Right now dpkg-
buildpackage is way more aggressive in this regard as it sets environment
variables regardless if they already have been set or not. Relevant excerpt
from dpkg-buildpackage code:
my $build_flags = Dpkg::BuildFlags->new();
$build_flags->load_config();
foreach my $flag ($build_flags->list()) {
$ENV{$flag} = $build_flags->get($flag);
printf(_g("%s: export %s from dpkg-buildflags (origin: %s): %s\n"),
$progname, $flag, $build_flags->get_origin($flag), $ENV{$flag});
}
The upside of this aggressiveness is that sensitive environment variables,
that might already have been set before debian/rules, will be overwritten and
won't affect build of the package. I do understand that doing the same would
break backwards compatibility in debhelper, but maybe that's a good idea to do
for compat(9) or at least provide a dh/dh_auto_* option (e.g. --force-
buildflags) to give maintainers a choice to enable this. However, I very much
prefer the former as in my opinion, most package maintainers will want (or
assume) this behavior. Who would want random shell environment variables to
affect package build? What's more, "clean environment" is even implied in
examples of the Debian Policy ifself [1].
By the way, it's also a bit sad that:
export CFLAGS += -Wall
%:
dh $@
will break. Fortunately, this can be replaced by DEB_CFLAGS_APPEND += -Wall.
Maybe it's worthwhile to add this to the example of minimal rules?
[1] http://www.debian.org/doc/debian-policy/ch-source.html#s-debianrules-
options (see how CFLAGS is handled)
> means that supporting dpkg-buildflags will not
> require a debhelper compat level change.
>
> So, all that's needed is for dh (when running override targets, or just
> generally) and dh_auto_* (for when not called by dh) to run
> dpkg-buildflags --export, parse the shell formatted output, and shove it
> into %ENV.
I don't understand why you do not use Dpkg::BuildFlags directly. Its API is
marked as stable (our $VERSION = "1.00" [2]). I'm CC'ing Raphael in case I'm
suggesting something wrong.
[2] /usr/share/perl5/Dpkg/BuildFlags.pm:21
> Now, as to DEB_BUILD_OPTIONS=noopt, probably the best way would for be
> dh_auto_configure/dh_auto_build to support this by passing CFLAGS="-O0"
> drectly to configure, etc. That way it would have the most chance
> of turning on debugging build. But, that is in conflict with my
> conclusions above. And perhaps the less perfect but much simpler way to
> get debug builds is to use dpkg-buildflags to do it.
1) dpkg-buildflags already emits -O0 in case of DEB_BUILD_OPTIONS=noopt.
2) Why do you think that it is a good idea to *always* replace (s/-O\d+/-O0/)
in flags even if they might have already been set by maintainer (+ given
behaviour without noopt is different)? How is it backwards compatible?
The patch (0001) addressing Dpkg::BuildFlags usage, points 1) and 2) is
attached. It does not implement that compat(9) suggestion of mine but it would
be trivial to add.
--
Modestas Vainius <modes...@vainius.eu>
From 6091ffa2b8c1d6729bf10c8263bac1e47fb9d7d1 Mon Sep 17 00:00:00 2001
From: Modestas Vainius <mo...@debian.org>
Date: Sat, 18 Jun 2011 23:02:42 +0300
Subject: [PATCH] Use Dpkg::BuildFlags module directly in set_buildflags().
Dpkg::BuildFlags API is declared stable. It should be safe to use it directly
rather than dpkg-buildflags wrapper. In addition, do not do any
DEB_BUILD_OPTIONS=noopt handling in debhelper. Dpkg::BuildFlags already does it
for us.
---
Debian/Debhelper/Dh_Lib.pm | 33 ++++++++++++---------------------
1 files changed, 12 insertions(+), 21 deletions(-)
diff --git a/Debian/Debhelper/Dh_Lib.pm b/Debian/Debhelper/Dh_Lib.pm
index 51f16a6..0c779d1 100644
--- a/Debian/Debhelper/Dh_Lib.pm
+++ b/Debian/Debhelper/Dh_Lib.pm
@@ -901,32 +901,23 @@ sub cross_command {
}
# Sets environment variables from dpkg-buildflags. Avoids changing
-# any existing environment variables. Supports DEB_BUILD_OPTIONS=noopt.
+# any existing environment variables.
sub set_buildflags {
# optimisation
return if $ENV{DH_INTERNAL_BUILDFLAGS};
$ENV{DH_INTERNAL_BUILDFLAGS}=1;
- my $noopt=$ENV{DEB_BUILD_OPTIONS}=~/noopt/;
-
- my @shell=`dpkg-buildflags --export`;
- foreach my $line (@shell) {
- chomp $line;
- if ($line=~/^export\s+([^=]+)=(["'])(.*)\2$/) {
- my $var=$1;
- my $val=$3;
- if ($noopt) {
- $val=$ENV{$var} if exists $ENV{$var};
- $val=~s/-O\d+/-O0/;
- $ENV{$var}=$val;
- next;
- }
- elsif (! exists $ENV{$var}) {
- $ENV{$var}=$val;
- }
- }
- else {
- warning "unparsable line from dpkg-buildflags: $line";
+ eval "use Dpkg::BuildFlags";
+ if ($@) {
+ warning "unable to load build flags: $@";
+ }
+
+ my $buildflags = Dpkg::BuildFlags->new();
+ $buildflags->load_config();
+ foreach my $flag ($buildflags->list()) {
+ next unless $flag =~ /^[A-Z]/; # Skip flags starting with lowercase
+ if (! exists $ENV{$flag}) {
+ $ENV{$flag} = $buildflags->get($flag);
}
}
}
--
1.7.5.4
signature.asc
Description: This is a digitally signed message part.
