Control: tag -1 + patch Hi,
On Sat, 16 Jan 2016 20:04:37 +0100 Johannes Schauer <jo...@debian.org> wrote:
> 1. The EXTRA_REPOSITORIES are added to $dummy_archive_list_file in
> setup_apt_archive() in lib/Sbuild/ResolverBase.pm. But
> setup_apt_archive() is only called from install_deps() in the
> lib/Sbuild/*Resolver.pm files. This is too late because this means
> that the initial update/upgrade step from run_chroot_update() in
> lib/Sbuild/Build.pm will ignore any extra repositories passed on the
> command line. This is no problem if the extra repository is only
> used to install additional packages but it is a problem if the extra
> repository contains packages with higher versions than the installed
> essential or build-essential packages. Also see bug #792037
this has been taken care of and is in the sbuild git.
> 2. The function run_chroot_update() in lib/Sbuild/Build.pm should not
> be run multiple times. Especially not now that there is an external
> command hook for this step. Running update and upgrade once should
> be sufficient. Instead though, run_chroot_update() is called again
> if a new architecture was added in check_architectures(). Thus, the
> check whether foreign architectures have to be added should be moved
> to before the original call to run_chroot_update() in
> run_chroot_session_locked().
This is a tricky one because it creates a chicken-and-egg situation when sbuild
is instructed to download the source package itself. In that situation, the
source package's metadata is required *before* the chroot is updated, so that
extra architectures can be added to the chroot. But sbuild should only download
the source package (including its metadata) *after* the chroot is updated to
pick up the latest source package version. Here is the summery of the attached
patch that fixes this issue but introduces possible problems when sbuild
downloads the source package for the user:
lib/Sbuild/Build.pm: Avoid doing an sbuild update twice (closes: #811202)
Split the check_architectures() function into two parts. One that checks
the source package for build dependencies with explicit architecture
qualifiers and one that checks if the architectures from the source
package can be produced. The former has to be run *before* the chroot is
updated, so that any additionally required architectures can be added by
the update. The latter has to be done *after* the chroot has been
updated and build-essential is installed because it requires the
Dpkg::Arch module inside the chroot. This splitting and reordering is to
avoid calling the chroot update more than once because we want to avoid
running external commands more than once as we don't want to require
them to be idempotent.
This would all be no problem if sbuild would not also offer the option
to download source packages by itself. Ideally, we only want to download
the source packages *after* the chroot has been updated and after extra
repositories were added to the internal apt sources. Unfortunately, we
need the source package's metadata to analyze its build dependencies for
those with explicit architecture qualification *before* the chroot has
been updated. Using the output of "apt-cache showsrc" is only a crutch
because the information of that command might be outdated.
It is a chicken-and-egg problem because downloading the source package
needs an up-to-date chroot, but updating the chroot needs information
about the architecture qualifiers which in turn needs up-to-date
metadata.
This problem can only occur when the user instructs sbuild to download
the source package itself and the source package saw a recent upload
such that the metadata the chroot knows about before and after the
update differs.
In summary, this commit makes sbuild behave better in situations where
it is to build a given .dsc and external command hooks are to be run. It
makes it behave worse when it is instructed to download the source
package itself.
A possible solution to the chicken-and-egg problem would be to add a
separate step for updating the source package lists only, supply a way
to add extra source package lists and extra external command hooks for
exactly that step. I'm not sure the complexity is worth it though.
Anybody knows an elegant solution?
Patch attached.
Thanks!
cheers, josch
From 7b51cc82bdaf772782675191a554123e01a75527 Mon Sep 17 00:00:00 2001
From: Johannes 'josch' Schauer <jo...@mister-muffin.de>
Date: Mon, 4 Jul 2016 09:11:52 +0200
Subject: [PATCH] lib/Sbuild/Build.pm: Avoid doing an sbuild update twice
(closes: #811202)
Split the check_architectures() function into two parts. One that checks
the source package for build dependencies with explicit architecture
qualifiers and one that checks if the architectures from the source
package can be produced. The former has to be run *before* the chroot is
updated, so that any additionally required architectures can be added by
the update. The latter has to be done *after* the chroot has been
updated and build-essential is installed because it requires the
Dpkg::Arch module inside the chroot. This splitting and reordering is to
avoid calling the chroot update more than once because we want to avoid
running external commands more than once as we don't want to require
them to be idempotent.
This would all be no problem if sbuild would not also offer the option
to download source packages by itself. Ideally, we only want to download
the source packages *after* the chroot has been updated and after extra
repositories were added to the internal apt sources. Unfortunately, we
need the source package's metadata to analyze its build dependencies for
those with explicit architecture qualification *before* the chroot has
been updated. Using the output of "apt-cache showsrc" is only a crutch
because the information of that command might be outdated.
It is a chicken-and-egg problem because downloading the source package
needs an up-to-date chroot, but updating the chroot needs information
about the architecture qualifiers which in turn needs up-to-date
metadata.
This problem can only occur when the user instructs sbuild to download
the source package itself and the source package saw a recent upload
such that the metadata the chroot knows about before and after the
update differs.
In summary, this commit makes sbuild behave better in situations where
it is to build a given .dsc and external command hooks are to be run. It
makes it behave worse when it is instructed to download the source
package itself.
A possible solution to the chicken-and-egg problem would be to add a
separate step for updating the source package lists only, supply a way
to add extra source package lists and extra external command hooks for
exactly that step. I'm not sure the complexity is worth it though.
---
lib/Sbuild/Build.pm | 241 +++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 210 insertions(+), 31 deletions(-)
diff --git a/lib/Sbuild/Build.pm b/lib/Sbuild/Build.pm
index 5b9407b..f5dc55d 100644
--- a/lib/Sbuild/Build.pm
+++ b/lib/Sbuild/Build.pm
@@ -584,6 +584,15 @@ sub run_chroot_session_locked {
failstage => "resolver setup");
}
+ # we must check the architectures before updating the chroot because
+ # more architectures might be added and we don't want to
+ # update/upgrade/distupgrade the chroot multiple times
+ $self->check_abort();
+ if (!$self->check_architectures()) {
+ #Sbuild::Exception::Build->throw(error => "checking architectures failed",
+ # failstage => "architecture check");
+ }
+
$self->check_abort();
$self->run_chroot_update();
@@ -744,7 +753,7 @@ sub run_fetch_install_packages {
# run inside the chroot which requires the Dpkg::Arch module which is
# in libdpkg-perl which might not exist in the chroot but will get
# installed by the build-essential package
- if(!$self->check_architectures()) {
+ if(!$self->can_build_architectures()) {
Sbuild::Exception::Build->throw(error => "Architecture check failed",
failstage => "check-architecture");
}
@@ -1106,6 +1115,26 @@ sub fetch_source_files {
$build_conflicts_arch =~ s/\n\s+/ /g if defined $build_conflicts_arch;
$build_conflicts_indep =~ s/\n\s+/ /g if defined $build_conflicts_indep;
+ # the build dependencies have already been set by the check_architecture
+ # function. Here we make sure that the information we retrieved now is the
+ # same. It definitely should if we are parsing a .dsc but if we downloaded
+ # the source package, then the information might be different because the
+ # earlier values where retrieved from "apt-cache showsrc" *before* the
+ # chroot was updated/upgraded/distupgraded and might thus be out-of-date.
+ # If this is the case, we abort here.
+ unless(((!defined $build_depends && !defined $self->get('Build Depends'))
+ || (defined $build_depends && defined $self->get('Build Depends')
+ && $build_depends eq $self->get('Build Depends')))
+ && ((!defined $build_depends_arch && !defined $self->get('Build Depends Arch'))
+ || (defined $build_depends_arch && defined $self->get('Build Depends Arch')
+ && $build_depends_arch eq $self->get('Build Depends Arch')))
+ && ((!defined $build_depends_indep && !defined $self->get('Build Depends Indep'))
+ || (defined $build_depends_indep && defined $self->get('Build Depends Indep')
+ && $build_depends_indep eq $self->get('Build Depends Indep')))) {
+ $self->log_error("Build dependencies from before and after chroot upgrades don't agree.\n");
+ return 0;
+ }
+
$self->set('Build Depends', $build_depends);
$self->set('Build Depends Arch', $build_depends_arch);
$self->set('Build Depends Indep', $build_depends_indep);
@@ -1132,12 +1161,149 @@ sub fetch_source_files {
sub check_architectures {
my $self = shift;
my $resolver = $self->get('Dependency Resolver');
- my $dscarchs = $self->get('Dsc Architectures');
- my $host_arch = $self->get('Host Arch');
$self->log_subsection("Check architectures");
# Check for cross-arch dependencies
# parse $build_depends* for explicit :arch and add the foreign arches, as needed
+
+ # This check runs before the chroot is updated/upgraded/distupgraded so
+ # that that operation will take extra architectures found in this check
+ # into account. Unfortunately, when sbuild itself is tasked with "apt-get
+ # source"-ing the source package, then that operation will only be done
+ # *after* the update/upgrade/distupgrade step so that the most up-to-date
+ # information is taken into account for that downloading step. This means
+ # that in this case, this function does not have access to the original
+ # .dsc as this is yet to be downloaded. So instead, we will use the
+ # current state of the apt cache to "apt-cache showsrc" the build
+ # dependency fields.
+ #
+ # So there exists a corner case, where the user tasks sbuild with
+ # downloading a source package with architecture qualified build
+ # dependencies in an out-of-date chroot where this function will produce
+ # wrong results.
+ #
+ # Since most auto-builders do not use sbuild to download the source
+ # package, since the work-around is simple (update the chroot beforehand)
+ # and since most source package do not get uploaded in a very high
+ # frequency, we accept this downside of this order to do things. It is
+ # really a chicken and egg problem where downloading the most recent
+ # source package might need a chroot update but before we update the
+ # chroot we want to gather all architectures, which needs the source
+ # package...
+ my $build_depends = "";
+ my $build_depends_arch = "";
+ my $build_depends_indep = "";
+ if ($self->get('DSC Base') =~ m/\.dsc$/) {
+ # sbuild was given a .dsc, so we just parse it to get the build
+ # dependencies
+ my $dsc = $self->get('DSC');
+
+ my $pdsc = Dpkg::Control->new(type => CTRL_PKG_SRC);
+ $pdsc->set_options(allow_pgp => 1);
+ if (!$pdsc->load($dsc)) {
+ $self->log_error("Error parsing $dsc");
+ return 0;
+ }
+
+ $build_depends = $pdsc->{'Build-Depends'};
+ $build_depends_arch = $pdsc->{'Build-Depends-Arch'};
+ $build_depends_indep = $pdsc->{'Build-Depends-Indep'};
+ } else {
+ # sbuild was instructed to download the source package, so we use
+ # apt-cache showsrc to get the build dependencies
+ my $pkg = $self->get('DSC');
+ my $ver;
+
+ if ($pkg =~ m/_/) {
+ ($pkg, $ver) = split /_/, $pkg;
+ }
+
+ # Use apt to download the source files
+ $self->log_subsubsection("Check APT");
+ my %entries = ();
+ $self->log("Checking available source versions...\n");
+
+ my $pipe = $self->get('Dependency Resolver')->pipe_apt_command(
+ { COMMAND => [$self->get_conf('APT_CACHE'),
+ '-q', '--only-source', 'showsrc', $pkg],
+ USER => $self->get_conf('BUILD_USER'),
+ PRIORITY => 0,
+ DIR => '/'});
+ if (!$pipe) {
+ $self->log_error("Can't open pipe to ".$self->get_conf('APT_CACHE').": $!\n");
+ return 0;
+ }
+
+ my $key_func = sub {
+ return $_[0]->{Package} . '_' . $_[0]->{Version};
+ };
+
+ my $index = Dpkg::Index->new(get_key_func=>$key_func);
+
+ if (!$index->parse($pipe, 'apt-cache showsrc')) {
+ $self->log_error("Cannot parse output of apt-cache showsrc: $!\n");
+ return 0;
+ }
+
+ close($pipe);
+
+ if ($?) {
+ $self->log_error($self->get_conf('APT_CACHE') . " exit status $?: $!\n");
+ return 0;
+ }
+
+ my $highestversion;
+
+ foreach my $key ($index->get_keys()) {
+ my $cdata = $index->get_by_key($key);
+ my $pkgname = $cdata->{"Package"};
+ if (not defined($pkgname)) {
+ $self->log_warning("apt-cache output without Package field\n");
+ next;
+ }
+ if ($pkg ne $pkgname) {
+ $self->log_warning("apt-cache output for different package\n");
+ next;
+ }
+ my $pkgversion = $cdata->{"Version"};
+ if (not defined($pkgversion)) {
+ $self->log_warning("apt-cache output without Version field\n");
+ next;
+ }
+ if (defined($ver) and $ver ne $pkgversion) {
+ next;
+ }
+ if (!defined $highestversion) {
+ $highestversion = $pkgversion;
+ $build_depends = $cdata->{'Build-Depends'};
+ $build_depends_arch = $cdata->{'Build-Depends-Arch'};
+ $build_depends_indep = $cdata->{'Build-Depends-Indep'};
+ } else {
+ if (version_compare($highestversion, $pkgversion) < 0) {
+ $highestversion = $pkgversion;
+ $build_depends = $cdata->{'Build-Depends'};
+ $build_depends_arch = $cdata->{'Build-Depends-Arch'};
+ $build_depends_indep = $cdata->{'Build-Depends-Indep'};
+ }
+ }
+ }
+
+ if (!defined $highestversion) {
+ $self->log_error($self->get_conf('APT_CACHE') .
+ " returned no information about $pkg source\n");
+ $self->log_error("Are there any deb-src lines in your /etc/apt/sources.list?\n");
+ return 0;
+ }
+ }
+
+ $build_depends =~ s/\n\s+/ /g if defined $build_depends;
+ $build_depends_arch =~ s/\n\s+/ /g if defined $build_depends_arch;
+ $build_depends_indep =~ s/\n\s+/ /g if defined $build_depends_indep;
+
+ $self->set('Build Depends', "$build_depends");
+ $self->set('Build Depends Arch', $build_depends_arch);
+ $self->set('Build Depends Indep', $build_depends_indep);
+
sub get_explicit_arches
{
my $visited_deps = pop;
@@ -1193,11 +1359,9 @@ sub check_architectures {
my @explicit_arches = get_explicit_arches($merged_depends, {});
my @foreign_arches = grep {$_ !~ /any|all|native/} @explicit_arches;
- my $added_any_new;
for my $foreign_arch(@foreign_arches)
{
$resolver->add_foreign_architecture($foreign_arch);
- $added_any_new = 1;
}
my @keylist=keys %{$resolver->get('Initial Foreign Arches')};
@@ -1206,16 +1370,32 @@ sub check_architectures {
$self->log('Foreign Architectures in build-deps: '. join ' ', @foreign_arches, "\n\n")
if @foreign_arches;
- $self->run_chroot_update() if $added_any_new;
+ return 1;
+}
+# Check package arch makes sense to build by inspecting whether the
+# architectures of the packages the source package builds can be generated by
+# the chroot.
+# Because the check inside the chroot requires Dpkg::Arch, this function must
+# only be run after build-essential got installed into the chroot.
+sub can_build_architectures {
+ my $self = shift;
+ my $resolver = $self->get('Dependency Resolver');
+ my $dscarchs = $self->get('Dsc Architectures');
+ my $host_arch = $self->get('Host Arch');
- # Check package arch makes sense to build
if (!$dscarchs) {
$self->log_warning("dsc has no Architecture: field -- skipping arch check!\n");
- } else {
- my $valid_arch;
- for my $a (split(/\s+/, $dscarchs)) {
- my $command = <<"EOF";
+ return 1;
+ }
+
+ my $valid_arch;
+ for my $a (split(/\s+/, $dscarchs)) {
+ # This has to be executed inside the chroot because the host
+ # running sbuild might not know about new architectures while the
+ # chroot can be new enough or contain manual tweaks to support
+ # more architectures.
+ my $command = <<"EOF";
use strict;
use warnings;
use Dpkg::Arch;
@@ -1224,28 +1404,27 @@ sub check_architectures {
}
exit 1;
EOF
- $self->get('Session')->run_command(
- { COMMAND => ['perl',
- '-e',
- $command],
- USER => 'root',
- PRIORITY => 0,
- DIR => '/' });
- if ($? == 0) {
- $valid_arch = 1;
- last;
- }
- }
- if ($dscarchs ne "any" && !($valid_arch) &&
- !($dscarchs =~ /\ball\b/ && $self->get_conf('BUILD_ARCH_ALL')) ) {
- my $msg = "dsc: $host_arch not in arch list or does not match any arch wildcards: $dscarchs -- skipping\n";
- $self->log_error($msg);
- Sbuild::Exception::Build->throw(error => "dsc: $host_arch not in arch list or does not match any arch wildcards: $dscarchs -- skipping",
- status => "skipped",
- failstage => "arch-check");
- return 0;
+ $self->get('Session')->run_command(
+ { COMMAND => ['perl',
+ '-e',
+ $command],
+ USER => 'root',
+ PRIORITY => 0,
+ DIR => '/' });
+ if ($? == 0) {
+ $valid_arch = 1;
+ last;
}
}
+ if ($dscarchs ne "any" && !($valid_arch) &&
+ !($dscarchs =~ /\ball\b/ && $self->get_conf('BUILD_ARCH_ALL')) ) {
+ my $msg = "dsc: $host_arch not in arch list or does not match any arch wildcards: $dscarchs -- skipping\n";
+ $self->log_error($msg);
+ Sbuild::Exception::Build->throw(error => "dsc: $host_arch not in arch list or does not match any arch wildcards: $dscarchs -- skipping",
+ status => "skipped",
+ failstage => "arch-check");
+ return 0;
+ }
$self->log("Arch check ok ($host_arch included in $dscarchs)\n");
--
2.8.1
signature.asc
Description: signature
