Hi,
the idea seems good, in particular since we had to remove this
from dpkg-buildpackage because the information outputted there could
not be accurate (i.e. no access to DEB_*_MAINT_* variables).
Here's a review so that you can bring your patch in a state where I can
merge it.
On Thu, 15 Mar 2012, Bernhard R. Link wrote:
> --- a/man/dpkg-buildflags.1
> +++ b/man/dpkg-buildflags.1
> @@ -72,6 +72,17 @@ Print the list of flags supported by the current vendor
> (one per line). See the \fBSUPPORTED FLAGS\fP section for more
> information about them.
> .TP
> +.BI \-\-status
> +Print all information to standard output:
> +Environment variables that might have had some influence,
> +the current vendor,
> +the state of all feature flags, and finally
> +all compiler flags together with their origin and values.
Suggestion of a better sentence:
Display any information that can be useful to explain the behaviour
of dpkg-buildflags: relevant environment variables, current vendor,
state of all feature flags. Also print the resulting compiler
flags with their origin.
> +This is intended to be run from debian/rules, so that the log
> +contains all the information or to debug why the flags are that
> +they end up to be.
Reworded:
This is intended to be run from \fBdebian/rules\fP, so that the
build log keeps a clear trace of the build flags used. This can
be useful to diagnose problems related to them.
> --- a/scripts/Dpkg/BuildFlags.pm
> +++ b/scripts/Dpkg/BuildFlags.pm
Please document the fact that you added a new method in the corresponding
section at the end of the file. ($VERSION="1.02" has not been released
yet, otherwise you'd have to increase it to "1.03" and document it there)
> diff --git a/scripts/dpkg-buildflags.pl b/scripts/dpkg-buildflags.pl
> index d0f9fa8..890076b 100755
> --- a/scripts/dpkg-buildflags.pl
> +++ b/scripts/dpkg-buildflags.pl
> @@ -24,6 +24,7 @@ use Dpkg;
> use Dpkg::Gettext;
> use Dpkg::ErrorHandling;
> use Dpkg::BuildFlags;
> +use Dpkg::Vendor qw(get_current_vendor);
>
> textdomain("dpkg-dev");
>
> @@ -52,6 +53,7 @@ Actions:
> compilation flags in a shell script, in make,
> or on a ./configure command line.
> --dump output all compilation flags with their values
> + --status informational message about current status
Better:
print a synopsis with all parameters affecting
the behaviour of dpkg-buildflags
> @@ -80,6 +82,10 @@ while (@ARGV) {
> usageerr(_g("two commands specified: --%s and --%s"), "list",
> $action)
> if defined($action);
> $action = "list";
> + } elsif (m/^--status$/) {
> + usageerr(_g("two commands specified: --%s and --%s"), "status",
> $action)
> + if defined($action);
> + $action = "status";
This would be the 3rd copy of the same boiler-plate code. Please merge the
3 copies in one (using /^--(status|list|dump)$/ and later $1 instead of
the hardcode value).
> +} elsif ($action eq "status") {
> + # prefix everything with "dpkg-buildflags: " to allow easy extraction
> + # from a buildd log.
IMO this is not the proper way to extract it automatically. If you want
this feature, you should add a unique (and fixed) start/end marker.
Maybe something like this ?
--- BEGIN DPKG-BUILDFLAGS STATUS ---
[…]
--- END DPKG-BUILDFLAGS STATUS ---
If you really want to keep the "dpkg-builflags: " prefix, then you should
use one of the functions exported by Dpkg::ErrorHandling. But I don't
think it's required.
> + # results: (would be nice to only print those having an effect for
> + # the current vendor, but getting that information here would be
> + # quite tough):
You can easily do that... instead of hardcoding it here, create
a new vendor hook for this purpose. Either the vendor hook
allows to extend your @envvars or it prints directly supplementary
information to include...
> + my @envvars = ('DEB_VENDOR', 'DEB_BUILD_OPTIONS',
> + 'DEB_BUILD_MAINT_OPTIONS', 'DEB_BUILD_HARDENING');
And indeed I was puzzled by seeing DEB_BUILD_HARDENING but realized thanks
to your comment that it was only relevant for Ubuntu...
> + foreach my $flag ($build_flags->list()) {
> + push @envvars, "DEB_" . $flag . "_SET",
> + "DEB_" . $flag . "_STRIP",
> + "DEB_" . $flag . "_APPEND",
> + "DEB_" . $flag . "_PREPEND",
> + "DEB_" . $flag . "_MAINT_SET",
> + "DEB_" . $flag . "_MAINT_STRIP",
> + "DEB_" . $flag . "_MAINT_APPEND",
> + "DEB_" . $flag . "_MAINT_PREPEND";
> + }
This list should not be hardcoded at this level but in Dpkg::BuildFlags.
Please create a new method list_envvar() that returns this list and use it
here.
> + # note that DEB_*_MAINT_* currently is not reflected
> + # by $origin...
This was on purpose. It's a choice of the maintainer and thus of the
vendor. And since it happens at the end, it would hide any
user/system-wide customization... and I don't want this.
Cheers,
--
Raphaël Hertzog ◈ Debian Developer
Pre-order a copy of the Debian Administrator's Handbook and help
liberate it: http://debian-handbook.info/liberation/
--
To UNSUBSCRIBE, email to [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]
