|
»
»
Bug#229357: New Build-Options field and build-arch option, please review
|
View:
New views
9 Messages
—
Rating Filter:
Alert me
|
 |
Bug#229357: New Build-Options field and build-arch option, please review
by Raphael Hertzog-3
::
Rate this Message:
Hello,
in order to fix #229357 I decided to add a new Build-Options field. I modified Dpkg::BuildOptions to parse this field and DEB_BUILD_OPTIONS. And I added support for a build-arch option, that if present, will let dpkg-buildpackage call debian/rules build-arch and build-indep. It's not obvious that this was the right choice when you think of the currently existing build options but once you start thinking of possible additions (as requested in #489771), it becomes more evident that it makes sense. Even if some build options should really only be used in the field while others should only be used in the environment variable, the possibility to override the former with the latter is nice. The current patchset is available in my public repository but I'll attach it as well so that you can easily review it. I intend to merge it this week-end after some tests but feel free to test and comment in the mean time. http://git.debian.org/?p=users/hertzog/dpkg.git;a=shortlog;h=refs/heads/pu/bug229357-build-options The patchset only applies on top of master. Cheers, -- Raphaël Hertzog Le best-seller français mis à jour pour Debian Etch : http://www.ouaza.com/livre/admin-debian/ >From 1ebeff797bc36c91e50f02c6d32cba094e827add Mon Sep 17 00:00:00 2001 From: Raphael Hertzog <hertzog@...> Date: Sun, 6 Jul 2008 22:03:27 +0200 Subject: [PATCH] Refactor Dpkg::BuildOptions to handle Build-Options field * scripts/Dpkg/BuildOptions.pm: complete rewrite of the module to handle various sources of build options: some options are auto-set based on the standards version, then the maintainer can define options with the Build-Options field in debian/control and last the builder can use DEB_BUILD_OPTIONS to override everything. Some options are meant to be exported through DEB_BUILD_OPTIONS and some are not. * scripts/t/300_Dpkg_BuildOptions.t: adjust test suite for the new module * scripts/dpkg-buildpackage.pl: adjust to use the new Dpkg::BuildOptions API. * scripts/Dpkg/Fields.pm, scripts/Dpkg/Source/Package.pm: add the new Build-Options field as a valid field in the source section of debian/control (and in .dsc files). --- scripts/Dpkg/BuildOptions.pm | 257 +++++++++++++++++++++++++++++++++---- scripts/Dpkg/Fields.pm | 2 +- scripts/Dpkg/Source/Package.pm | 5 +- scripts/dpkg-buildpackage.pl | 10 +- scripts/t/300_Dpkg_BuildOptions.t | 61 +++++---- 5 files changed, 273 insertions(+), 62 deletions(-) diff --git a/scripts/Dpkg/BuildOptions.pm b/scripts/Dpkg/BuildOptions.pm index 9d6741b..5b2acdd 100644 --- a/scripts/Dpkg/BuildOptions.pm +++ b/scripts/Dpkg/BuildOptions.pm @@ -5,51 +5,252 @@ use warnings; use Dpkg::Gettext; use Dpkg::ErrorHandling qw(warning); +use Dpkg::Control; +use Dpkg::Version qw(compare_versions); -sub parse { - my ($env) = @_; +# Define behavior for known options: +# export -> the option is meant to be exported in DEB_BUILD_OPTIONS +# valued -> the option can have a value +# check_value_rx -> if defined, a regex to check the value, invalid value +# will lead to the option being discarded +# min_standards_version -> if the s-v field is >= to the version given, +# the option is auto-enabled +our %OPTIONS = ( + noopt => { + export => 1, + valued => 0, + }, + nostrip => { + export => 1, + valued => 0, + }, + nocheck => { + export => 1, + valued => 0, + }, + parallel => { + export => 1, + valued => 1, + check_value_rx => qr/^-?\d+$/, + }, +); - $env ||= $ENV{DEB_BUILD_OPTIONS}; +=head1 NAME - unless ($env) { return {}; } +Dpkg::BuildOptions - handle build options from debian/control and environment - my %opts; +=head1 DESCRIPTION - foreach (split(/\s+/, $env)) { - unless (/^([a-z][a-z0-9_-]*)(=(\S*))?$/) { - warning(_g("invalid flag in DEB_BUILD_OPTIONS: %s"), $_); - next; +It provides an object to analyze and manipulate build options as defined +by combining information provided by the debian/control file and by the +DEB_BUILD_OPTIONS environment variable. + +=head1 FUNCTIONS + +=over 4 + +=item $b = Dpkg::BuildOptions($file) + +Create a new Dpkg::BuildOptions object. The $file parameter is simply +forwarded to Dpkg::Control->new($file). If undef, it will simply use +debian/control by default. + +=cut +sub new { + my ($this, $ctl_file) = @_; + my $class = ref($this) || $this; + my $self = { + 'opts' => {}, + 'control' => Dpkg::Control->new($ctl_file), + }; + bless $self, $class; + $self->parse_options(); + return $self; +} + +=item $b->reset() + +Forget all options already parsed. Start afresh. + +=cut +sub reset { + my ($self) = @_; + $self->{'opts'} = {}; +} + +=item $b->parse_options() + +Do a full parse of options, including the Build-Options field in +debian/control and the DEB_BUILD_OPTIONS variable. + +=cut +sub parse_options { + my ($self) = @_; + $self->parse_standards_version(); + $self->parse_field(); + $self->parse_env(); +} + +=item $b->parse_standards_version() + +Update the options based on the current value of the Standards-Version +field. + +=cut +sub parse_standards_version { + my ($self, $sv) = @_; + + my $src = $self->{'control'}->get_source(); + $sv = $src->{'Standards-Version'} unless defined $sv; + return unless $sv; + + foreach my $opt (keys %OPTIONS) { + my $min_sv = $OPTIONS{$opt}{'min_standards_version'}; + next unless defined $min_sv; + if (compare_versions($sv, '>=', $min_sv)) { + $self->{'opts'}{$opt} = { %{$OPTIONS{$opt}} }; } + } +} + +=item $b->parse_field() + +Update the options based on the value of the Build-Options field in the +associated Dpkg::Control object. It will also define some options based +on the value of the Standards-Version field. + +=cut +sub parse_field { + my ($self, $field) = @_; + my $src = $self->{'control'}->get_source(); + $field = $src->{'Build-Options'} unless defined $field; + $self->_parse($field, 'field'); +} + +=item $b->parse_env() + +Update the options based on the value of DEB_BUILD_OPTIONS environment +variable. + +=cut +sub parse_env { + my ($self, $env) = @_; + $env = $ENV{'DEB_BUILD_OPTIONS'} unless defined $env; + $self->_parse($env, 'env'); +} - my ($k, $v) = ($1, $3 || ''); +=item my $env = $b->export - # Sanity checks - if ($k =~ /^(noopt|nostrip|nocheck)$/ && length($v)) { - $v = ''; - } elsif ($k eq 'parallel' && $v !~ /^-?\d+$/) { - next; - } +Export the current set of build options in the DEB_BUILD_OPTIONS +environment variable. Only options that are meant to be exported +will be included. For convenience, the return value also contains the +new value of the variable. - $opts{$k} = $v; +=cut +sub export { + my ($self) = @_; + my @flags; + foreach my $opt (sort keys %{$self->{'opts'}}) { + my $o = $self->{'opts'}{$opt}; + if ($o->{'export'}) { + if ($o->{'value'}) { + push @flags, "$opt=" . $o->{'value'}; + } else { + push @flags, $opt; + } + } } + my $env = join(" ", @flags); + $ENV{'DEB_BUILD_OPTIONS'} = $env; + return $env; +} + +=item $b->has($option) - return \%opts; +Return true if the option is defined, false otherwise. + +=cut +sub has { + my ($self, $opt) = @_; + return exists $self->{'opts'}{$opt}; } +=item $b->get($option) + +Return the current value of the option if it has any. + +=cut +sub get { + my ($self, $opt) = @_; + return $self->{'opts'}{$opt}{'value'}; +} + +=item $b->set($option, $value) + +Add a new option or overwrite the current one. + +=cut sub set { - my ($opts, $overwrite) = @_; - $overwrite = 1 if not defined($overwrite); + my ($self, $opt, $val) = @_; + $self->{'opts'}{$opt}{'value'} = $val || ''; + $self->{'opts'}{$opt}{'source'} = 'code'; +} - my $new = {}; - $new = parse() unless $overwrite; - while (my ($k, $v) = each %$opts) { - $new->{$k} = $v; - } +## Non-public interface below +sub _parse { + my ($self, $value, $source) = @_; + return unless $value; - my $env = join(" ", map { $new->{$_} ? $_ . "=" . $new->{$_} : $_ } sort keys %$new); + foreach (split(/\s+/, $value)) { + unless (/^([a-z][a-z0-9_-]*)(?:=(\S*))?$/) { + warning(_g("invalid flag in %s: %s"), $source eq "field" ? + "Build-Options" : "DEB_BUILD_OPTIONS", $_); + next; + } + my ($k, $v) = ($1, $2 || ''); - $ENV{DEB_BUILD_OPTIONS} = $env; - return $env; + if ($k =~ /^no-(.*)$/) { + # Disable an option + delete $self->{'opts'}{$1}; + next; + } + + # Define an (new) option + my %o; + if (exists $OPTIONS{$k}) { + %o = %{$OPTIONS{$k}}; + } elsif ($source eq "field") { + $o{'export'} = 0; # Unknown options from B-O: are not exported + } + if ($source eq "env") { + $o{'export'} = 1; # All options from environment are exported + } + $o{'source'} = $source; + $o{'value'} = $v; + + # Check/sanitize the option + if (defined($o{'valued'})) { + if (defined($o{'check_value_rx'})) { + unless ($v =~ $o{'check_value_rx'}) { + warning(_g("discarding build option %s due to " . + "invalid value: %s"), $k, $v); + next; + } + } + $o{'value'} = '' unless $o{'valued'}; + } + + # Store it + $self->{'opts'}{$k} = \%o; + } } +=back + +=head1 AUTHOR + +Raphael Hertzog <hertzog@...>. + +=cut + 1; diff --git a/scripts/Dpkg/Fields.pm b/scripts/Dpkg/Fields.pm index 6504a1f..cb7325e 100644 --- a/scripts/Dpkg/Fields.pm +++ b/scripts/Dpkg/Fields.pm @@ -15,7 +15,7 @@ our %EXPORT_TAGS = ('list' => [qw(%control_src_fields %control_pkg_fields # Some variables (list of fields) our %control_src_fields; our %control_pkg_fields; -$control_src_fields{$_} = 1 foreach (qw(Bugs Dm-Upload-Allowed +$control_src_fields{$_} = 1 foreach (qw(Bugs Build-Options Dm-Upload-Allowed Homepage Origin Maintainer Priority Section Source Standards-Version Uploaders Vcs-Browser Vcs-Arch Vcs-Bzr Vcs-Cvs Vcs-Darcs Vcs-Git Vcs-Hg Vcs-Mtn Vcs-Svn)); diff --git a/scripts/Dpkg/Source/Package.pm b/scripts/Dpkg/Source/Package.pm index 2ba8479..2b785dc 100644 --- a/scripts/Dpkg/Source/Package.pm +++ b/scripts/Dpkg/Source/Package.pm @@ -89,8 +89,9 @@ _darcs # Private stuff my @dsc_fields = (qw(Format Source Binary Architecture Version Origin Maintainer Uploaders Dm-Upload-Allowed Homepage - Standards-Version Vcs-Browser Vcs-Arch Vcs-Bzr - Vcs-Cvs Vcs-Darcs Vcs-Git Vcs-Hg Vcs-Mtn Vcs-Svn), + Standards-Version Build-Options Vcs-Browser Vcs-Arch + Vcs-Bzr Vcs-Cvs Vcs-Darcs Vcs-Git Vcs-Hg Vcs-Mtn + Vcs-Svn), @src_dep_fields, qw(Checksums-Md5 Checksums-Sha1 Checksums-Sha256 Files)); diff --git a/scripts/dpkg-buildpackage.pl b/scripts/dpkg-buildpackage.pl index 93d72a1..f335477 100755 --- a/scripts/dpkg-buildpackage.pl +++ b/scripts/dpkg-buildpackage.pl @@ -244,20 +244,20 @@ if ($signcommand) { } } -my $build_opts = Dpkg::BuildOptions::parse(); +my $build_opts = Dpkg::BuildOptions->new(); if ($parallel) { - $parallel = $build_opts->{parallel} if (defined $build_opts->{parallel}); + $parallel = $build_opts->get("parallel") if $build_opts->has("parallel"); $ENV{MAKEFLAGS} ||= ''; if ($parallel eq '-1') { $ENV{MAKEFLAGS} .= " -j"; } else { $ENV{MAKEFLAGS} .= " -j$parallel"; } - $build_opts->{parallel} = $parallel; - Dpkg::BuildOptions::set($build_opts); + $build_opts->set("parallel", $parallel); } +$build_opts->export(); -my $default_flags = defined $build_opts->{noopt} ? "-g -O0" : "-g -O2"; +my $default_flags = $build_opts->has("noopt") ? "-g -O0" : "-g -O2"; my %flags = ( CPPFLAGS => '', CFLAGS => $default_flags, CXXFLAGS => $default_flags, diff --git a/scripts/t/300_Dpkg_BuildOptions.t b/scripts/t/300_Dpkg_BuildOptions.t index dc43acd..0ad5fc1 100644 --- a/scripts/t/300_Dpkg_BuildOptions.t +++ b/scripts/t/300_Dpkg_BuildOptions.t @@ -1,12 +1,28 @@ # -*- mode: cperl;-*- -use Test::More tests => 6; +use Test::More tests => 11; use strict; use warnings; use_ok('Dpkg::BuildOptions'); +$Dpkg::BuildOptions::OPTIONS{'test-sv1'} = { + export => 1, + valued => 0, + min_standards_version => '3.0.1', +}; +$Dpkg::BuildOptions::OPTIONS{'test-sv2'} = { + export => 0, + valued => 0, + min_standards_version => '12.3', +}; +$Dpkg::BuildOptions::OPTIONS{'test_rx'} = { + export => 0, + valued => 1, + check_value_rx => qr/^\dx\d$/, +}; + { no warnings; # Disable warnings related to invalid values fed during @@ -16,36 +32,29 @@ use_ok('Dpkg::BuildOptions'); $ENV{DEB_BUILD_OPTIONS} = 'noopt foonostripbar parallel=3 bazNOCHECK'; -my $dbo = Dpkg::BuildOptions::parse(); - -my %dbo = ( - noopt => '', - foonostripbar => '', - parallel => 3, - ); -my %dbo2 = ( - no => '', - opt => '', - 'no-strip' => '', - nocheck => '', - ); +my $dbo = Dpkg::BuildOptions->new("/dev/null"); +$dbo->reset(); +$dbo->parse_standards_version("3.8.0"); +ok($dbo->has("test-sv1"), "test-sv1 is autoset"); +ok(!$dbo->has("test-sv2"), "test-sv2 is not autoset"); -is_deeply($dbo, \%dbo, 'parse'); +$dbo->parse_field("test_rx=bla"); +ok(!$dbo->has("test_rx"), "test_rx has been discarded"); -$dbo = Dpkg::BuildOptions::parse('no opt no-strip parallel = 5 nocheck'); +$dbo->parse_field("test_rx=4x4"); +ok($dbo->has("test_rx"), "testrx has been set"); +is($dbo->get("test_rx"), "4x4", "value of testrx is correct"); -is_deeply($dbo, \%dbo2, 'parse (param)'); +$dbo->set("test-sv2"); +ok($dbo->has("test-sv2"), "test-sv2 has been set"); -$dbo->{parallel} = 5; -$dbo->{noopt} = ''; +$dbo->parse_env("noopt=1 no-test-sv1 foonostripbar parallel=3 bazNOCHECK pasbon = 2"); -my $env = Dpkg::BuildOptions::set($dbo, 1); +ok(!$dbo->has("test-sv1"), "test-sv1 got removed"); +ok(!$dbo->has("no-test-sv1"), "no-testsv1 doesn't exist"); +is($dbo->get("noopt"), "", "noopt has no value"); -is($ENV{DEB_BUILD_OPTIONS}, $env, 'set (return value)'); -is_deeply(Dpkg::BuildOptions::parse(), $dbo, 'set (env)'); +$dbo->export(); +is($ENV{DEB_BUILD_OPTIONS}, "foonostripbar noopt parallel=3 pasbon", "exported options"); -$ENV{DEB_BUILD_OPTIONS} = 'foobar'; -$dbo = { noopt => '' }; -$env = Dpkg::BuildOptions::set($dbo, 0); -is($env, "foobar noopt", 'set (append)'); -- 1.5.6.2 >From de4a0d2935201352f3b24382a18b8893d9ae2bdf Mon Sep 17 00:00:00 2001 From: Raphael Hertzog <hertzog@...> Date: Wed, 9 Jul 2008 22:29:11 +0200 Subject: [PATCH] dpkg-buildpackage: use build-arch/indep target. Closes: #229357 * scripts/Dpkg/BuildOptions.pm: Add the new build-arch Build-Options. * scripts/dpkg-buildpackage.pl: Call the build-arch/build-indep target instead of the build target when possible. * man/dpkg-buildpackage.1: Document the above changes. --- man/dpkg-buildpackage.1 | 49 +++++++++++++++++++++++++++++++++++++++-- scripts/Dpkg/BuildOptions.pm | 5 ++++ scripts/dpkg-buildpackage.pl | 9 ++++++- 3 files changed, 58 insertions(+), 5 deletions(-) diff --git a/man/dpkg-buildpackage.1 b/man/dpkg-buildpackage.1 index 6ffc5cb..cf49a53 100644 --- a/man/dpkg-buildpackage.1 +++ b/man/dpkg-buildpackage.1 @@ -24,12 +24,16 @@ It calls \fBdpkg-source\fP to generate the source package (unless a binary-only build has been requested with \fB\-b\fP, \fB\-B\fP or \fB\-A\fP). .IP \fB5.\fP 3 -It calls \fBdebian/rules\fP \fBbuild\fP followed by +It calls \fBdebian/rules\fP \fIbuild-target\fP followed by \fBfakeroot debian/rules\fP \fIbinary-target\fP (unless a source-only build has been requested with \fB\-S\fP). Note that \fIbinary-target\fR is -either \fBbuild\fP (default case, or if \fB\-b\fP is specified) +either \fBbinary\fP (default case, or if \fB\-b\fP is specified) or \fBbinary-arch\fP (if \fB\-B\fP is specified) or \fBbinary-indep\fP -(if \fB\-A\fP is specified). +(if \fB\-A\fP is specified). \fIbuild-target\fP is usually \fBbuild\fP +(default case, or if \fB\-b\fP is specified) but it can also be +\fBbuild-arch\fP (with \fB\-B\fP) or \fBbuild-indep\fP (with \fB\-A\fP) +if the package advertises \fBbuild-arch\fP in its \fIBuild-Options\fP +field (see \fBBUILD OPTIONS\fP). .IP \fB6.\fP 3 It calls \fBgpg\fP to sign the \fB.dsc\fP file (if any, unless \fB\-us\fP is specified). @@ -197,6 +201,45 @@ Show the usage message and exit. .BR \-\-version Show the version and exit. . +.SH BUILD OPTIONS +Build options can be set: +.IP . 2 +by the package maintainer using the \fIBuild-Options\fP field in the +source stanza of \fBdebian/control\fP; +.IP . 2 +by the caller with the DEB_BUILD_OPTIONS environment variable. +.P +Both methods share a common syntax; options are space separated, +they can have an optional value appended with an equal sign +(\fIoption-name\fP\fB=\fP\fIvalue\fP) and their names are composed of +lower-case alphanumeric characters, dashes and underscores. The first +character must be a letter. +.P +The options are evaluated in the order given above, and it's +possible to remove an option previously set by adding an option +\fBno-\fP\fIoption-name\fP. +.P +The following options are commonly used: +.TP +.B build-arch +Indicates that the rules file supports the \fBbuild-arch\fP and +\fBbuild-indep\fP targets. +.TP +.BR nocheck " (*)" +Disable test-suite and other runtime checks run during the build. +.TP +.BR noopt " (*)" +Disable compiler optimizations. +.TP +.BR nostrip " (*)" +Disable stripping of compiled binaries. +.TP +.BI parallel= value "\fR (*)\fP" +Let the build-system run up to \fIvalue\fP concurrent jobs during build. +.P +The options marked with (*), when set, are always exported in the +DEB_BUILD_OPTIONS environment variable. +. .SH ENVIRONMENT VARIABLES .SS Vendor identification The variable \fBDEB_VENDOR\fR will be set to the name of the current vendor diff --git a/scripts/Dpkg/BuildOptions.pm b/scripts/Dpkg/BuildOptions.pm index 5b2acdd..0580e00 100644 --- a/scripts/Dpkg/BuildOptions.pm +++ b/scripts/Dpkg/BuildOptions.pm @@ -33,6 +33,11 @@ our %OPTIONS = ( valued => 1, check_value_rx => qr/^-?\d+$/, }, + 'build-arch' => { + export => 0, + valued => 0, + min_standards_version => undef, + }, ); =head1 NAME diff --git a/scripts/dpkg-buildpackage.pl b/scripts/dpkg-buildpackage.pl index f335477..2009716 100755 --- a/scripts/dpkg-buildpackage.pl +++ b/scripts/dpkg-buildpackage.pl @@ -101,9 +101,12 @@ my $checkbuilddep = 1; my $signsource = 1; my $signchanges = 1; my $diffignore = ''; +my $buildtarget = 'build'; my $binarytarget = 'binary'; my $targetarch = my $targetgnusystem = ''; +my $build_opts = Dpkg::BuildOptions->new(); + while (@ARGV) { $_ = shift @ARGV; @@ -162,6 +165,7 @@ while (@ARGV) { $binaryonly = '-b'; @checkbuilddep_args = (); $binarytarget = 'binary'; + $buildtarget = 'build'; if ($sourceonly) { usageerr(_g("cannot combine %s and %s"), '-b', '-S'); } @@ -169,6 +173,7 @@ while (@ARGV) { $binaryonly = '-B'; @checkbuilddep_args = ('-B'); $binarytarget = 'binary-arch'; + $buildtarget = 'build-arch' if $build_opts->has("build-arch"); if ($sourceonly) { usageerr(_g("cannot combine %s and %s"), '-B', '-S'); } @@ -176,6 +181,7 @@ while (@ARGV) { $binaryonly = '-A'; @checkbuilddep_args = (); $binarytarget = 'binary-indep'; + $buildtarget = 'build-indep' if $build_opts->has("build-arch"); if ($sourceonly) { usageerr(_g("cannot combine %s and %s"), '-A', '-S'); } @@ -244,7 +250,6 @@ if ($signcommand) { } } -my $build_opts = Dpkg::BuildOptions->new(); if ($parallel) { $parallel = $build_opts->get("parallel") if $build_opts->has("parallel"); $ENV{MAKEFLAGS} ||= ''; @@ -369,7 +374,7 @@ unless ($binaryonly) { chdir($dir) or failure("chdir $dir"); } unless ($sourceonly) { - withecho(@debian_rules, 'build'); + withecho(@debian_rules, $buildtarget); withecho(@rootcommand, @debian_rules, $binarytarget); } if ($usepause && -- 1.5.6.2 |
|
|
Bug#229357: New Build-Options field and build-arch option, please review
by Felipe Sateler
::
Rate this Message:
El 10/07/08 18:02 Raphael Hertzog escribió:
> Hello, > > in order to fix #229357 I decided to add a new Build-Options field. > I modified Dpkg::BuildOptions to parse this field and DEB_BUILD_OPTIONS. > And I added support for a build-arch option, that if present, will let > dpkg-buildpackage call debian/rules build-arch and build-indep. > > It's not obvious that this was the right choice when you think of the > currently existing build options but once you start thinking of possible > additions (as requested in #489771), it becomes more evident that it makes > sense. Even if some build options should really only be used in > the field while others should only be used in the environment variable, > the possibility to override the former with the latter is nice. here: declare that a package supports something, and asking the package to do something. This difference is blurred now, and I think it is confusing. OTOH, it gives the benefit of being able to ignore the package capabilities via the environment (ie, unset a given option). I fear it will give rise to abuses such as setting parallel=n in the control file. Saludos, Felipe Sateler |
|
|
Bug#229357: New Build-Options field and build-arch option, please review
by Raphael Hertzog-3
::
Rate this Message:
On Thu, 10 Jul 2008, Felipe Sateler wrote:
> El 10/07/08 18:02 Raphael Hertzog escribió: > > Hello, > > > > in order to fix #229357 I decided to add a new Build-Options field. > > I modified Dpkg::BuildOptions to parse this field and DEB_BUILD_OPTIONS. > > And I added support for a build-arch option, that if present, will let > > dpkg-buildpackage call debian/rules build-arch and build-indep. > > > > It's not obvious that this was the right choice when you think of the > > currently existing build options but once you start thinking of possible > > additions (as requested in #489771), it becomes more evident that it makes > > sense. Even if some build options should really only be used in > > the field while others should only be used in the environment variable, > > the possibility to override the former with the latter is nice. > > I'm not really sure this is right. There are two things that we want to do > here: declare that a package supports something, and asking the package to do > something. This difference is blurred now, and I think it is confusing. Even if there's only two things, the fact is that the package maintainer wants not only to decide what is supported but he might also want to enable some features... if you check the case that I listed above, we also want to use Build-Options to _enable_ specific hardening measures. Because the maintainer knows best which hardening measures should be enabled. But we also want the builder to be able to override them for example to test if the package now supports a previously disabled hardening measure. The meaning of each build options is specific to each, there's no global rule that works for all cases. That's why we have documentation of each option in dpkg-buildpackage. > I fear it will give rise to abuses such as setting parallel=n in the control > file. There are dozens of ways to "abuse" any interface if you choose to use it in a way that contradicts the documentation. But that's not a reason to limit the flexibility offered by an interface. Cheers, -- Raphaël Hertzog Le best-seller français mis à jour pour Debian Etch : http://www.ouaza.com/livre/admin-debian/ -- To UNSUBSCRIBE, email to debian-dpkg-bugs-REQUEST@... with a subject of "unsubscribe". Trouble? Contact listmaster@... |
|
|
Bug#489771: New Build-Options field and build-arch option, please review
by Bill Allombert-3
::
Rate this Message:
On Thu, Jul 10, 2008 at 07:19:16PM -0400, Felipe Sateler wrote:
> El 10/07/08 18:02 Raphael Hertzog escribió: > > Hello, > > > > in order to fix #229357 I decided to add a new Build-Options field. > > I modified Dpkg::BuildOptions to parse this field and DEB_BUILD_OPTIONS. > > And I added support for a build-arch option, that if present, will let > > dpkg-buildpackage call debian/rules build-arch and build-indep. > > > > It's not obvious that this was the right choice when you think of the Maybe it is not obvious, but since noone proposed another working solution in the ten years this issue exists, there is no alternative. > > currently existing build options but once you start thinking of possible > > additions (as requested in #489771), it becomes more evident that it makes > > sense. Even if some build options should really only be used in > > the field while others should only be used in the environment variable, > > the possibility to override the former with the latter is nice. > > I'm not really sure this is right. There are two things that we want to do > here: declare that a package supports something, and asking the package to do > something. This difference is blurred now, and I think it is confusing. > OTOH, it gives the benefit of being able to ignore the package capabilities > via the environment (ie, unset a given option). > I fear it will give rise to abuses such as setting parallel=n in the control > file. I concur. This also create a namespace problem by conflating the 'Build-Options' namespace with the DEB_BUILD_OPTIONS namespace. Since a developer can put virtually anything in DEB_BUILD_OPTIONS (and check for it in debian/rules) even if it is not mentionned in policy, this is a real issue. Cheers, -- Bill. <ballombe@...> Imagine a large red swirl here. -- To UNSUBSCRIBE, email to debian-dpkg-bugs-REQUEST@... with a subject of "unsubscribe". Trouble? Contact listmaster@... |
|
|
Bug#489771: New Build-Options field and build-arch option, please review
by Russ Allbery-2
::
Rate this Message:
Raphael Hertzog <hertzog@...> writes:
> Even if there's only two things, the fact is that the package maintainer > wants not only to decide what is supported but he might also want to > enable some features... if you check the case that I listed above, we > also want to use Build-Options to _enable_ specific hardening > measures. Because the maintainer knows best which hardening measures > should be enabled. But we also want the builder to be able to override > them for example to test if the package now supports a previously > disabled hardening measure. This doesn't make sense to me. The maintainer writes debian/rules; why would they need to change Build-Options in debian/control to enable anything about the build? I'd rather see Build-Options in debian/control be clearly defined as capabilities that the package supports and not used as a substitute for the existing DEB_BUILD_OPTIONS method of controlling what the build does in practice. (And I'd prefer it to be called Build-Options-Supported or something along those lines.) I think this still fits for #489771; the presence of the hardening option in Build-Options-Supported indicates that the package can usefully be built with hardening (it doesn't cause the package build to break or the binaries to malfunction). If the package maintainer wants the package to always be built with those options, they should make that change directly in debian/rules, not via this method. They're going to have to test each flag that goes into the hardening options separately anyway to make sure that it works (the current proposed hardening flags break many packages, and if you follow debian/changelog files, you'll see that many maintainers have added them blindly and then had to roll back when they break). Using a debian/control field to set DEB_BUILD_OPTIONS in dpkg-buildpackage is a solution looking for a problem, IMO, and I'd rather not see that tangled up with the much-needed problem of specifying which options a package supports and finally dealing with the whole build-arch/build-indep mess. -- Russ Allbery (rra@...) <http://www.eyrie.org/~eagle/> -- To UNSUBSCRIBE, email to debian-dpkg-bugs-REQUEST@... with a subject of "unsubscribe". Trouble? Contact listmaster@... |
|
|
|
|
|
Bug#489771: New Build-Options field and build-arch option, please review
by Russ Allbery-2
::
Rate this Message:
Raphael Hertzog <hertzog@...> writes:
> On Fri, 11 Jul 2008, Russ Allbery wrote: >> This doesn't make sense to me. The maintainer writes debian/rules; why >> would they need to change Build-Options in debian/control to enable >> anything about the build? > Because they want that anyone can easily rebuild it with that option > disabled? That is already supported using the existing DEB_BUILD_OPTIONS mechanism. I may be confused about your mental model here, but it seems like you're moving rules about how the package is built from the package itself into dpkg-buildpackage. If that's really what's happening, I think that is a truly dreadful idea and strongly object. It should be possible to build the package using whatever flags and options are the default by running debian/rules build without involving dpkg-buildpackage at all, which implies that the package should not be relying on dpkg-buildpackage to provide compiler and linker flags. Those defaults should be in debian/rules, just as they always have been for Debian packages. If some set of flags, such as hardening, should be possible to easily disable, this is exactly the same case as we have right now with optimization and with stripping. The way to support that is to specify another DEB_BUILD_OPTIONS flag which, if set, instructs the package to modify its behavior accordingly. Furthermore, that allows the package maintainer to provide more useful defaults specific to that package, such as exactly the hardening flags that *that* package supports, rather than some default (and possibly changing) set from dpkg-buildpackage. DEB_BUILD_OPTIONS then stays clearly semantically separate from the Build-Options-Supported field; the latter specifies which interfaces the package supports, and the former is the way to actually *use* those interfaces, with some exceptions for interfaces that can be used other ways (such as build-arch/build-indep). >> If the package maintainer wants the package to always be built with >> those options, they should make that change directly in debian/rules, >> not via this method. > Why? (and it's not "always", it's by _default_) See above. By moving the logic from debian/rules into dpkg-buildpackage, we would be breaking a common workflow when working with packages. Running debian/rules build in an unpacked source package to test would no longer be a reasonable development step since you may get a completely different compile than dpkg-buildpackage would give you. I think the way that optimization and stripping are handled right now works fairly well in practice, and I think we should be building on that as a model, not replacing it with some entirely different method that relies on additional external programs to wrap debian/rules. The choice between always and by default can be handled using the existing DEB_BUILD_OPTIONS mechanism just as optimization and stripping are now. > I find it rather nice that we have a common way to enable this for all > packages: add a hardening-wrapper to Build-Depends, add the option > indicating which of the hardenings flags to enable, and you're done and > it works for all packages. Instead of doing that, you add hardening-wrapper to Build-Depends and modify debian/rules to invoke it. The process is just as simple. > Of course, you can also set the right variables in debian/rules directly > but then you make it complex for anyone to disable those build options > (for example to verify if a failure can be attributed to one of these > hardening options). Not if you implement a DEB_BUILD_OPTIONS flag at the same time. You can then make hardening-wrapper trigger off of the DEB_BUILD_OPTIONS flag and the package maintainer doesn't even have to handle it directly (very similar to how debhelper packages let dh_strip handle DEB_BUILD_OPTIONS for that flag). -- Russ Allbery (rra@...) <http://www.eyrie.org/~eagle/> -- To UNSUBSCRIBE, email to debian-dpkg-bugs-REQUEST@... with a subject of "unsubscribe". Trouble? Contact listmaster@... |
|
|
Bug#489771: New Build-Options field and build-arch option, please review
by Raphael Hertzog-3
::
Rate this Message:
On Sun, 13 Jul 2008, Russ Allbery wrote:
> > Because they want that anyone can easily rebuild it with that option > > disabled? > > That is already supported using the existing DEB_BUILD_OPTIONS mechanism. > > I may be confused about your mental model here, but it seems like you're > moving rules about how the package is built from the package itself into > dpkg-buildpackage. If that's really what's happening, I think that is a > truly dreadful idea and strongly object. It should be possible to build > the package using whatever flags and options are the default by running > debian/rules build without involving dpkg-buildpackage at all, which > implies that the package should not be relying on dpkg-buildpackage to > provide compiler and linker flags. Those defaults should be in > debian/rules, just as they always have been for Debian packages. I think we're already on that path for quite some time. If your package uses DEB_(BUILD|HOST)_* variables, you rely on dpkg-buildpackage setting them for you (with dpkg-architecture). The same is expected with default values of builder/linker flags now that dpkg-buildpackage provides reasonable defaults. So yes, I'm somehow building on this model where dpkg-buildpackage can simplify the work of packager by providing some distribution-wide reasonable defaults. People have noticed that and already requested that we can call arbitrary targets of debian/rules with all the proper initialization done precisely for test purpose during packaging work (see #477916). > If some set of flags, such as hardening, should be possible to easily > disable, this is exactly the same case as we have right now with > optimization and with stripping. The way to support that is to specify > another DEB_BUILD_OPTIONS flag which, if set, instructs the package to > modify its behavior accordingly. Furthermore, that allows the package > maintainer to provide more useful defaults specific to that package, such > as exactly the hardening flags that *that* package supports, rather than > some default (and possibly changing) set from dpkg-buildpackage. Ok makes sense. In the case of hardening, it means that we have to modify each and every package to enable it though. I suppose that the people pushing this proposal would like to have the option to enable it globally and have broken packages opt out and/or disable specific hardening options. Without taking into account the specific risks associated to any default activation of build hardening, I find that having a generic system where you can start early with an opt-in policy, have the stuff matures, and switch to an opt-out policy later can make sense (if that plan is announced early and that people know by advance how to opt-out explicitely). > See above. By moving the logic from debian/rules into dpkg-buildpackage, > we would be breaking a common workflow when working with packages. > Running debian/rules build in an unpacked source package to test would no > longer be a reasonable development step since you may get a completely > different compile than dpkg-buildpackage would give you. That might be so, but I'm not sure why it would be a major problem. It can take some time to change habits but unless you see real drawbacks, I'm not convinced that there are good reasons to revert in that direction. > I think the way that optimization and stripping are handled right now > works fairly well in practice, and I think we should be building on that > as a model, not replacing it with some entirely different method that > relies on additional external programs to wrap debian/rules. > > The choice between always and by default can be handled using the existing > DEB_BUILD_OPTIONS mechanism just as optimization and stripping are now. Well, right now buildd do not use DEB_BUILD_OPTIONS at all AFAIK. So there's no way to enable anything globally with this method in practice. And I certainly wouldn't want to have to manually set DEB_BUILD_OPTIONS to get a build similar to what the buildd would do. The current practice only has options to disable something that is enabled by default. I'm not sure you can usefully build on that to provide a mechanism where something is disabled by default and that can be enabled either by the maintainer or by the builder. But maybe such a scheme is not desirable in general, we might not want to offer any option for the builder that has not been validated by the maintainer. I don't know. Maybe we won't have any other situation similar to the hardening one and it's over-kill to try to generalize it. the Cheers, -- Raphaël Hertzog Le best-seller français mis à jour pour Debian Etch : http://www.ouaza.com/livre/admin-debian/ -- To UNSUBSCRIBE, email to debian-dpkg-bugs-REQUEST@... with a subject of "unsubscribe". Trouble? Contact listmaster@... |
|
|
Bug#489771: New Build-Options field and build-arch option, please review
by Russ Allbery-2
::
Rate this Message:
Raphael Hertzog <hertzog@...> writes:
> I think we're already on that path for quite some time. If your package > uses DEB_(BUILD|HOST)_* variables, you rely on dpkg-buildpackage setting > them for you (with dpkg-architecture). I most certainly do not rely on dpkg-buildpackage setting anything. I call dpkg-architecture directly, which is also what's in our best practice documents. DEB_HOST_GNU_TYPE ?= $(shell dpkg-architecture -qDEB_HOST_GNU_TYPE) DEB_BUILD_GNU_TYPE ?= $(shell dpkg-architecture -qDEB_BUILD_GNU_TYPE) I would consider packages that didn't do that and just assumed that those variables were already set to be buggy. > The same is expected with default values of builder/linker flags now > that dpkg-buildpackage provides reasonable defaults. Yeah, that bothered me too. I made a perhaps poor tactical decision to not fight about it since it seemed that it had a lot of momentum and I couldn't think of specific problems other than the one that we ran into. But this is going beyond setting some defaults that are already set in nearly all of our packages. > So yes, I'm somehow building on this model where dpkg-buildpackage can > simplify the work of packager by providing some distribution-wide > reasonable defaults. > > People have noticed that and already requested that we can call arbitrary > targets of debian/rules with all the proper initialization done precisely > for test purpose during packaging work (see #477916). I must say, I really do not like this direction. debhelper and cdbs and similar sytsems are the places to provide this help where people want to use it, in my opinion. We have a lot of past experience with that and we have the compatibility level to handle smoothing transitions. (And to provide a way for people to never transition, I admit, and I see where that's the problem that you're solving, but I prefer that problem to the problems introduced by the instability of having the package build infrastructure change the input to the builds without coordination with the package.) Note that if you're requiring a package to participate by adding something to Build-Options in debian/control, you have the same transition problem, so I think that it's pretty equivalent to changing debian/rules; it's only when you want packages to be able to change with external defaults that you get the transition advantage. I don't want to underestimate the transition advantage -- that is pretty significant. I do understand the problem that you're trying to solve, and I understand that what I'm proposing is going to make transitions a lot harder. > Ok makes sense. In the case of hardening, it means that we have to > modify each and every package to enable it though. Well, not if you can do it via debhelper, which now with version 7 is much more likely. Similarly with cdbs. But in general, yes. For hardening, I think this is a feature; the flags aren't ones that can just be applied to every package without breaking things. > I suppose that the people pushing this proposal would like to have the > option to enable it globally and have broken packages opt out and/or > disable specific hardening options. I think we've already found that this isn't a great approach for hardening options in particular, since they break too many packages (and those packages are not necessarily broken; in some cases it's the compiler that's broken, or the assumptions behind the options). > Without taking into account the specific risks associated to any default > activation of build hardening, I find that having a generic system where > you can start early with an opt-in policy, have the stuff matures, and > switch to an opt-out policy later can make sense (if that plan is > announced early and that people know by advance how to opt-out > explicitely). I agree with the benefit, but I think it's better to implement that sort of thing in the packaging tools that already do that sort of magic and which we already have a way of dealing with (compatibility levels in debhelper, for example), and which continue to work with debian/rules build. >> See above. By moving the logic from debian/rules into >> dpkg-buildpackage, we would be breaking a common workflow when working >> with packages. Running debian/rules build in an unpacked source >> package to test would no longer be a reasonable development step since >> you may get a completely different compile than dpkg-buildpackage would >> give you. > That might be so, but I'm not sure why it would be a major problem. It > can take some time to change habits but unless you see real drawbacks, I'm > not convinced that there are good reasons to revert in that direction. I'm somewhat disturbed by this. Until this discussion, I had no idea that you were planning on deprecating debian/rules build and expecting everyone to use dpkg-buildpackage to get a reproducible build. I'm not even sure how to use dpkg-buildpackage to do the equivalent of just running debian/rules build without the binary-* targets. It seems like this is a significant enough change that it would warrant Policy changes and a significant announcement in debian-devel-announce, and I don't think we've had one about the high-level semantic change (but maybe I missed it?). > Well, right now buildd do not use DEB_BUILD_OPTIONS at all AFAIK. I think the buildds should always build packages with the defaults set by the maintainer of that package. > The current practice only has options to disable something that > is enabled by default. The current practice has options that either disable something *or* enable something; parallel=N is in the latter category. DEB_BUILD_OPTIONS is used to change the defaults, in whichever direction. -- Russ Allbery (rra@...) <http://www.eyrie.org/~eagle/> -- To UNSUBSCRIBE, email to debian-dpkg-bugs-REQUEST@... with a subject of "unsubscribe". Trouble? Contact listmaster@... |
signature.asc (204 bytes) 
| Free Forum Powered by Nabble | Forum Help |