On Sun, 05 Jun 2011, Bill Allombert wrote:
> Please find a new patch that use the name Build-Features and the module name
> Dpkg::BuildFeatures.
Thanks, here's a short review with a few details to clean.
1/ You're not consistent with the coding style (see doc/coding-style.txt).
2/ Why are you not calling "build-indep" for dpkg-buildpackage -A? AFAIU it
should be exactly like the binary target:
With flag | Without flag
-b => build | build
-B => build-arch | build
-A => build-indep | build
-F => build | build
3/ You should document the new field in po/deb-src-control.5
> --- a/man/dpkg-buildpackage.1
> +++ b/man/dpkg-buildpackage.1
The doc needs to be updated as per 2/ above.
> --- /dev/null
> +++ b/scripts/Dpkg/BuildFeatures.pm
> +package Dpkg::BuildFeatures;
> +
> +use strict;
> +use warnings;
> +
> +our $VERSION = "1.00";
Please use version 0.01. I don't want this module to be part of the
stable API yet.
> +The Dpkg::BuildFeatures object can be used to query the options
> +stored in the Build-Features control field
Missing dot at the end.
> +=head1 FUNCTIONS
> +
> +=over 4
> +
> +=item my $bo = Dpkg::BuildFeatures->new($controlfile)
s/bo/bf/
> + if (defined($src_fields->{'Build-Features'}))
> + {
> + my @buildfeats= split(/\s*,\s*/m, $src_fields->{'Build-Features'});
missing space before "="
> + %buildfeats = map { $_ => 1 } @buildfeats;
> + } else
> + {
> + %buildfeats=();
> + }
bad indentation and curly braces on the wrong lines
> + my $self = { options => \%buildfeats };
> + bless $self, $class;
> + return $self;
> +}
> +=item $bo->has($option)
Missing empty line before the POD doc.
s/bo/bp/
> diff --git a/scripts/Makefile.am b/scripts/Makefile.am
> index 9b4d394..be376c9 100644
> --- a/scripts/Makefile.am
> +++ b/scripts/Makefile.am
> @@ -56,6 +56,7 @@ nobase_dist_perllib_DATA = \
> Dpkg/Arch.pm \
> Dpkg/BuildFlags.pm \
> Dpkg/BuildOptions.pm \
> + Dpkg/BuildFeatures.pm \
> Dpkg/Changelog.pm \
> Dpkg/Changelog/Debian.pm \
> Dpkg/Changelog/Entry.pm \
You also want to add it to scripts/po/POTFILES.in in case we add
translatable strings in the future.
> diff --git a/scripts/dpkg-buildpackage.pl b/scripts/dpkg-buildpackage.pl
> index aaea544..17294cb 100755
> --- a/scripts/dpkg-buildpackage.pl
> +++ b/scripts/dpkg-buildpackage.pl
> @@ -27,6 +27,7 @@ use Dpkg::Gettext;
> use Dpkg::ErrorHandling;
> use Dpkg::BuildFlags;
> use Dpkg::BuildOptions;
> +use Dpkg::BuildFeatures;
> use Dpkg::Compression;
> use Dpkg::Version;
> use Dpkg::Changelog::Parse;
> @@ -100,6 +101,8 @@ Options:
> --version show the version.
> "), $progname;
> }
> +my $controlfile = "debian/control";
> +my $buildfeats = Dpkg::BuildFeatures->new($controlfile);
Do not use a useless intermediary variable. In fact, do not give a value
at all and fix the object's new method to assume "debian/control" if no
explicit value has been given.
> my @debian_rules = ("debian/rules");
> my @rootcommand = ();
> @@ -119,6 +122,7 @@ my $checkbuilddep = 1;
> my $signsource = 1;
> my $signchanges = 1;
> my $binarytarget = 'binary';
> +my $buildtarget = 'build';
Don't align the value when nothing else is aligned.
> my $targetarch = my $targetgnusystem = '';
> my $call_target = '';
> my $call_target_as_root = 0;
> @@ -247,6 +251,9 @@ if ($noclean) {
> $include = BUILD_BINARY if ($include & BUILD_DEFAULT);
> }
>
> +$buildtarget='build-arch' if ( $binarytarget eq 'binary-arch' &&
> + $buildfeats->has('build-arch'));
Define buildtarget to build-indep/build-arch while parsing the options
but reset it to "build" here if the flag is missing:
$buildtarget = "build" unless $buildfeats->has('build-arch');
Thank you for your work on this patch!
Cheers,
--
Raphaël Hertzog ◈ Debian Developer
Follow my Debian News ▶ http://RaphaelHertzog.com (English)
▶ http://RaphaelHertzog.fr (Français)
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
