Hi! Now back to your comments on the code: Guillem Jover: > > diff --git a/debian/usertags b/debian/usertags > > index 0fc26f2..0419488 100644 > > --- a/debian/usertags > > +++ b/debian/usertags > > @@ -65,6 +65,7 @@ dpkg-checkbuilddeps [DPKG-CHECKBUILDDEPS] > > dpkg-deb [DPKG-DEB] > > dpkg-distaddfile [DPKG-DISTADDFILE] > > dpkg-divert [DPKG-DIVERT] > > +dpkg-genbuildinfo [DPKG-GENBUILDINFO] > > The pseudo-tag is only needed for old tools for legacy reasons. I > should just do a round of «bts retitle» at some point. Removed. > > dpkg-genchanges [DPKG-GENCHANGES] > > dpkg-gencontrol [DPKG-GENCONTROL] > > dpkg-gensymbols [DPKG-GENCSYMBOLS] > > diff --git a/man/dpkg-genbuildinfo.1 b/man/dpkg-genbuildinfo.1 > > new file mode 100644 > > index 0000000..626bf66 > > --- /dev/null > > +++ b/man/dpkg-genbuildinfo.1 > > @@ -0,0 +1,89 @@ > > +.\" dpkg manual page - dpkg-genbuildinfo(1) > > +.\" > > +.\" Copyright © 2015 Jérémy Bobbio <lunar@debian.org> > > This probably needs to be completed with the © from the sources it's > borrowing from. Added. > The changes to man/po/po4a.conf are missing. Added. > > diff --git a/scripts/Dpkg/Control/FieldsCore.pm b/scripts/Dpkg/Control/FieldsCore.pm > > index 8a5695b..75de185 100644 > > --- a/scripts/Dpkg/Control/FieldsCore.pm > > +++ b/scripts/Dpkg/Control/FieldsCore.pm > > @@ -361,6 +373,11 @@ our %FIELD_ORDER = ( > > Vcs-Svn Testsuite), &field_list_src_dep(), qw(Package-List), > > @checksum_fields, qw(Files) > > ], > > + CTRL_FILE_BUILDINFO() => [ > > + qw(Format Build-Architecture Source Binary Architecture Version > > + Build-Environment Build-Path), > > + @checksum_fields, > > + ], > > Probably pack all «Build-» fields together at the end after the checksum > ones, and leave Build-Environment as the last of those, as it's the one > taking the most vertical space. Done. > > CTRL_FILE_CHANGES() => [ > > qw(Format Date Source Binary Binary-Only Built-For-Profiles Architecture > > Version Distribution Urgency Maintainer Changed-By Description > > diff --git a/scripts/dpkg-buildpackage.pl b/scripts/dpkg-buildpackage.pl > > index a3cca87..018f37d 100755 > > --- a/scripts/dpkg-buildpackage.pl > > +++ b/scripts/dpkg-buildpackage.pl > > @@ -158,6 +158,7 @@ my $since; > > my $maint; > > my $changedby; > > my $desc; > > +my @buildinfo_opts; > > my @changes_opts; > > my @hook_names = qw( > > init preclean source build binary changes postclean check sign done > > The buildinfo hook name is missing in the @hook_names array. Added. > > @@ -567,6 +570,25 @@ if ($include & BUILD_BINARY) { > > withecho(@debian_rules, $buildtarget); > > run_hook('binary', 1); > > withecho(@rootcommand, @debian_rules, $binarytarget); > > + > > + if (-e "../$pv.dsc") { > > Why is the buildinfo file tied to whether there's or not a source > package? I'd say it might make sense to not generate a buildinfo file > only if we are not building binary packages. To check for that use the > BUILD_ constants. I've added this test after seeing build failures with cross-binutils which calls `dpkg-buildpackage -B` without a source previously generated: https://sources.debian.net/src/cross-binutils/0.22/debian/rules/#L53 In my mind, the buildinfo files are tying a given source to binary packages. So if they can't contain the hash of a .dsc, then they should not be generated. If we say buildinfo are of a more general interest, dpkg-genbuildinfo could skip including the .dsc if there's none available. The whole block you quoted is conditional `$include & BUILD_BINARY`. Is there something more that needs to be added to only generate a buildinfo file only when building binary packages? > > + run_hook('buildinfo', 1); > > The hook should not be conditional to the code being executed. The > second argument should represent that. Something like the following, then? run_hook('buildinfo', $include & BUILD_BINARY); I'm not sure why this has to be different than the 'binary' hook? > > + push @buildinfo_opts, "--admindir=$admindir" if $admindir; > > + > > + my $buildinfo_path = "../$pva.buildinfo"; > > + my $buildinfo = Dpkg::Control->new(type => CTRL_FILE_BUILDINFO); > > + > > + print { *STDERR } " dpkg-genbuildinfo @buildinfo_opts >$buildinfo_path\n"; > > + > > + open my $buildinfo_fh, '-|', 'dpkg-genbuildinfo', @buildinfo_opts > > + or subprocerr('dpkg-genbuildinfo'); > > + $buildinfo->parse($buildinfo_fh, _g('parse buildinfo file')); > > + $buildinfo->save($buildinfo_path); > > + close $buildinfo_fh or subprocerr(_g('dpkg-genbuildinfo')); > > There's no need for all this, just call dpkg-genbuildinfo redirecting > its output to the destination file. The $changes code where this is > originating from is accessed later on, so that's why it's being parsed. Changed. > > diff --git a/scripts/dpkg-genbuildinfo.pl b/scripts/dpkg-genbuildinfo.pl > > new file mode 100755 > > index 0000000..d7784ee > > --- /dev/null > > +++ b/scripts/dpkg-genbuildinfo.pl > > @@ -0,0 +1,263 @@ > > +#!/usr/bin/perl > > +# > > +# dpkg-genbuildinfo > > +# > > +# Copyright © 2003-2013 Yann Dirson <dirson@debian.org> > > +# Copyright © 2014 Niko Tyni <ntyni@debian.org> > > +# Copyright © 2014-2015 Jérémy Bobbio <lunar@debian.org> > > This also probably needs to be completed with the © from the other > sources it's borrowing from. Added. > > +my $quiet = 0; > > There's nothing to be quiet about here. Removed. > > +my $buildinfo_format = '1.9'; > > Why is this format 1.9? Good question. I think I misread the policy while working on this during DebConf14. Now, I could invent a clever explaination: we could consider this format as an evolution from what dh-buildinfo produced. This would buildinfo “2.0”, and it's not stable yet, it's “1.9”. Should we make this 1.0 instead? :) > > +my $checksums = Dpkg::Checksums->new(); > > +my %archadded; > > +my @archvalues; > > + > > +# There's almost the same function in dpkg-checkbuilddeps, > > +# they probably should be factored out. > > +sub parse_status { > > I've a local commit switching the parse_status() in dpkg-checkbuilddeps > to use Dpkg::Index, unfortunately due to the current implementation > it incurs a significant speed regression, but I'm reworking the > Dpkg::Control parser to speed it up, so this code will be able to > switch to something similar too. Nice. :) > > + my $status = shift; > > + > > + my $facts = Dpkg::Deps::KnownFacts->new(); > > + my %depends; > > + local $/ = ''; > > + open(my $status_fh, '<', $status) > > + or syserr(_g('cannot open %s'), $status); > > + while (<$status_fh>) { > > + next unless /^Status: .*ok installed$/m; > > + > > + my ($package) = /^Package: (.*)$/m; > > + my ($version) = /^Version: (.*)$/m; > > + my ($arch) = /^Architecture: (.*)$/m; > > + my ($multiarch) = /^Multi-Arch: (.*)$/m; > > + $facts->add_installed_package($package, $version, $arch, > > + $multiarch); > > + > > + if (/^Provides: (.*)$/m) { > > + my $provides = deps_parse($1, reduce_arch => 1, union => 1); > > + next if not defined $provides; > > + foreach (grep { $_->isa('Dpkg::Deps::Simple') } > > + $provides->get_deps()) > > + { > > + $facts->add_provided_package($_->{package}, > > + $_->{relation}, $_->{version}, > > + $package); > > + } > > + } > > + > > + if (/^(?:Pre-)?Depends: (.*)$/m) { > > + foreach (split(/,\s*/, $1)) { > > + push @{$depends{$package}}, $_; > > This will merge all dependencies from all Multi-Arch:same instances. > But in any case why record the dependencies of the interesting packages? Sorry, I'm not sure I understand what you are asking. We want to record the version of all packages involved in the build. > > +# include .dsc > > +my $spackage = $changelog->{'Source'}; > > +(my $sversion = $changelog->{'Version'}) =~ s/^\d+://; > > This will grab the binary version, not the source version, this will > break on binNMUs. > > We probably need to record both source and binary versions in the > buildinfo file. And in case they differ the changelog entry, which > tends to differ per arch. I see. I wish handling binNMU could be simplier. I think I've added enough support for binNMU: * Like for *.changes files, the source version is added in Source between parenthesis if it differs from binary version. * The latest changelog entry is recorded in an optional Changes field. > > +foreach my $file (sort $dist->get_files()) { > > Why the sort, the function is supposed to preserve the same insertion > order. Now I think I remember what was happening. In case of parallel builds, it might be possible that files get added to debian/files in different orders. To make the buildinfo file itself reproducible, sorting the file list here is the easiest solution. Is there another option? > > +# parse essential list > > +open (RAWFILE, '/usr/share/build-essential/essential-packages-list') > > + or error("cannot read /usr/share/build-essential/essential-packages-list: $!\n"); > > +# skip header > > +while (my $line = <RAWFILE>) { > > + chomp $line; > > + last if $line eq ''; > > +} > > +while (my $line = <RAWFILE>) { > > + chomp $line; > > + $env_pkgs{$line} = 1; > > +} > > +close RAWFILE; > > Hmm, please just record installed packages that have the Essential:yes > field. I don't want the code to rely on such externally defined files. Done. > > +append_deps(\%env_pkgs, 'build-essential', > > + $control->{source}->{'Build-Depends-Indep'}, > > + $control->{source}->{'Build-Depends'}); > > Missing Build-Depends-Arch field. I missed it. I guess the policy and <https://wiki.debian.org/Build-Depends-Indep> should be updated. > But those fields should not be recorded if the build does not concern them. Done. > And this uses an undocumented access to the $control object. Fixed. > > + if (defined $architecture && > > + $architecture ne 'all' && $architecture ne $build_arch) { > > This should probably be host_arch. Sorry if I'm confused. Here's the reasoning: dpkg-architecture(1) says “build machine: the machine the package is built on”. We record this in the Build-Architecture field of the buildinfo. We thus only need to record when the architecture of a dependency differs from this default. > I get the impression the code tracking the used installed packages is > made much more difficult than it needs to be. I agree. I'm not happy with it. I will wait for your other changes in parse_status() before reworking it. > And I don't feel very comfortable assuming the .buildinfo name here > when it is not being generated in this same script, and when the > script generating the output is not even defining the name itself. Right. I've moved the logic to dpkg-buildpackage. Thanks again for your review! -- Lunar .''`. lunar@debian.org : :Ⓐ : # apt-get install anarchism `. `'` `-
Attachment:
signature.asc
Description: Digital signature