Skip to content

Commit

Permalink
Sbuild::ResolverBase: Implement update_archive using apt-get update
Browse files Browse the repository at this point in the history 
David Kalnischkies pointed out in bug #714877 a clever way to make
apt-get update only affect a single sources.list-format file, using various
apt options to override the sources.list and sources.list.d locations,
and keep the lists for existing sources. This works "in any apt
version", and is cleaner than faking the list operations ourselves.
  • Loading branch information
Geoffrey Thomas committed Jul 4, 2013
1 parent d7f31ed commit 557d394
Showing 1 changed file with 17 additions and 36 deletions.
53 changes: 17 additions & 36 deletions lib/Sbuild/ResolverBase.pm
View file
Expand Up @@ -188,45 +188,26 @@ sub update_archive {
DIR => '/' });
return $?;
} else {
# Example archive list names:
#_tmp_resolver-XXXXXX_apt%5farchive_._Packages
#_tmp_resolver-XXXXXX_apt%5farchive_._Release
#_tmp_resolver-XXXXXX_apt%5farchive_._Release.gpg
#_tmp_resolver-XXXXXX_apt%5farchive_._Sources

# Update lists directly; only updates local archive

# Filename quoting for safety taken from apt URItoFileName and
# QuoteString.
my $session = $self->get('Session');
my $dummy_dir = $self->get('Dummy package path');
my $dummy_archive_dir = $session->strip_chroot_path($dummy_dir.'/apt_archive/./');
my @chars = split('', $dummy_archive_dir);

foreach(@chars) {
if (index('\\|{}[]<>"^~_=!@#$%^&*', $_) != -1 || # "Bad" characters
!m/[[:print:]]/ || # Not printable
ord($_) == 0x25 || # Percent '%' char
ord($_) <= 0x20 || # Control chars
ord($_) >= 0x7f) { # Control chars
$_ = sprintf("%%%02x", ord($_));
}
my $dummy_archive_list_file = $self->get('Dummy archive list file');
# We need an empty sources.list.d directory, so that we have an option
# to pass in to skip the real one.
my $dummy_sources_list_d = $self->get('Dummy package path') . '/sources.list.d';
if (!(-d $dummy_sources_list_d || mkdir $dummy_sources_list_d, 0700)) {
$self->log_warning('Could not create build-depends dummy sources.list directory ' . $dummy_sources_list_d . ': ' . $!);
$self->cleanup_apt_archive();
return 0;

This comment has been minimized.

Sign in to view

Copy link
@em-

em- Jul 4, 2013

AIUI this is not needed, just pass /dev/null as Dir::Etc::sourceparts.

This comment has been minimized.

Sign in to view

Copy link
@geofft

geofft Jul 4, 2013

Owner

Hm, okay. @DonKult claimed that /dev/null only worked in recent versions of apt. He remembered wheezy, but it does seem to work fine in squeeze. I'd also worry about lucid, which I haven't tested, and maybe it's worthwhile not to break people's builders for dead distributions if they have them still around.

This comment has been minimized.

Sign in to view

Copy link
@DonKult

DonKult Jul 4, 2013

The problem was that APT was checking for file-existence instead of checking for directory-existence and opening a file as a directory is not a good idea (even if it is /dev/null) – fixed in 448eaf8b5999b6866237a069d678337b647968c4 (Thu May 6 11:55:54 2010 +0200). What you can do is choosing a name which does not exist at all if you want to be version independent (or create an empty directory as you do it here of course).

(I remembered a here unrelated issue: If you set dir::etc=/dev/null APT would try to read files like /etc/null/sources.list – fixed in af13d1437cbcb383de89f126b316c02587e4b691 (Mon Apr 23 19:33:32 2012 +0200))

This comment has been minimized.

Sign in to view

Copy link
@em-

em- Jul 4, 2013

@DonKult, thanks for the very detailed analysis! :D
@geofft, would you care to add this and the gencaches stuff as comments in the source? Thanks!
}

my $uri = join('', @chars);
$uri =~ s;/;_;g; # Escape slashes

foreach my $file ("Packages", "Release", "Release.gpg", "Sources") {
$session->run_command(
{ COMMAND => ['cp', $dummy_archive_dir . '/' . $file,
'/var/lib/apt/lists/' . $uri . $file],
USER => 'root',
PRIORITY => 0 });
if ($?) {
$self->log("Failed to copy file from dummy archive to apt lists.\n");
return 1;
}
}
$self->run_apt_command(
{ COMMAND => [$self->get_conf('APT_GET'), 'update',
'-o', 'Dir::Etc::sourcelist=' . $session->strip_chroot_path($dummy_archive_list_file),
'-o', 'Dir::Etc::sourceparts=' . $session->strip_chroot_path($dummy_sources_list_d),

This comment has been minimized.

Sign in to view

Copy link
@em-

em- Jul 4, 2013

'Dir::Etc::sourceparts=/dev/null' should work just fine (or at least it did in my quick local test :)
'-o', 'APT::List-Cleanup=0'],

This comment has been minimized.

Sign in to view

Copy link
@geofft

geofft Jul 5, 2013

Owner

--no-list-cleanup is shorter, and exists at least as far back as Lucid (and possibly indefinitely).
ENV => {'DEBIAN_FRONTEND' => 'noninteractive'},
USER => 'root',
DIR => '/' });
return $? if $?;

$self->run_apt_command(
{ COMMAND => [$self->get_conf('APT_CACHE'), 'gencaches'],

This comment has been minimized.

Sign in to view

Copy link
@em-

em- Jul 4, 2013

Since we're now relying on apt-get instead of playing tricks, I suppose that the explict call to apt-cache gencaches is no longer needed.

This comment has been minimized.

Sign in to view

Copy link
@geofft

geofft Jul 4, 2013

Owner

That was my first thought, but in some local testing I found that e.g. apt-cache policy consistently takes 4-5 seconds on my desktop after doing one of these partial updates, which goes back down to a fraction of a second after a gencaches. (You'll also note that a partial update does its "Reading package lists... done" much faster than a full one, which is exactly what gencaches does.)

Then again, the next thing we run is apt-get install, which is suitably privileged to update the caches properly, so maybe you're right that this isn't needed. I'll test more.

This comment has been minimized.

Sign in to view

Copy link
@DonKult

DonKult Jul 4, 2013

You still "have to" call gencaches as the cache APT is generating based on this update command is bogus (it includes only this source), so the next "normal" APT-command will notice that the cache is invalid and update it (if it has the rights to, otherwise built it in memory for own temporal usage). As a minor optimization you can add -o pkgCacheFile::Generate=0 to the update command as it will prevent the generation of the bogus file. The cache is still invalid (as it doesn't include the new source), but APT is at least not replacing an outdated with a bogus file.

This comment has been minimized.

Sign in to view

Copy link
@geofft

geofft Aug 15, 2013

Owner

As a minor optimization you can add -o pkgCacheFile::Generate=0 to the update command as it will prevent the generation of the bogus file.

This seems to be broken on Precise (apt 0.8.16~), at least:

(precise-amd64)root@salmon-of-wisdom:/tmp/q# apt-get update -o pkgCacheFile::Generate=0
[... normal update process ...]
Fetched 4015 kB in 3s (1047 kB/s)                                      
E: Could not open file /var/cache/apt/pkgcache.bin - open (2: No such file or directory)
E: Unable to determine the file size - fstat (9: Bad file descriptor)
E: Can't mmap an empty file
E: Empty package cache
(precise-amd64)root@salmon-of-wisdom:/tmp/q# echo $?
100

It's not broken on Quantal (apt 0.9.7.5), which vaguely implicates commit 872ed75 from apt 0.9.1. So I'm going to skip providing that option, since it's just an optimization. I'm also going to keep the explicit call to gencaches because why not -- it's a little more robust, in case we start doing anything between then and the apt-get install.
Expand Down

6 comments on commit 557d394

@em-
Copy link

@em- em- commented on 557d394 Jul 30, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geofft, do you have the chance to check the issues highlighted in the previous comments? I'd like to start poking people around to get this stuff merged once the patchset is ready. Thanks!

@geofft
Copy link
Owner

@geofft geofft commented on 557d394 Jul 30, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm meaning to get to it but work's been keeping me busy. I'm at a conference this week, but I'll try to get to it this weekend at latest.

@em-
Copy link

@em- em- commented on 557d394 Jul 30, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! No need to hurry though, I was just pinging to prevent this stuff from going out of your radar (and mine). :)

@em-
Copy link

@em- em- commented on 557d394 Aug 12, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping! :D

More seriously, would you prefer having me polish up the branch a bit so you can review it and submit it upstream?

@geofft
Copy link
Owner

@geofft geofft commented on 557d394 Aug 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@em-, thanks for the pings. I've pushed a new version to the extra-package branch (partly 'cause I renamed the option, partly 'cause I wasn't sure where the comments on this branch would go). Let me know if you have thoughts.

Do you want to rebase this on top of your refactoring, or should I just send this to the bug?

@em-
Copy link

@em- em- commented on 557d394 Aug 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@em-, thanks for the pings. I've pushed a new version to the extra-package branch (partly 'cause I renamed the option, partly 'cause I wasn't sure where the comments on this branch would go). Let me know if you have thoughts.

Yup, I too find how github comments are bound to the commit hash a bit confusing when you're rebasing a branch.

Do you want to rebase this on top of your refactoring, or should I just send this to the bug?

No, keep your stuff simple and easier to merge, I'll rebase my refactoring on top of your changes once they enter the upstream repo.

Thanks!

Please sign in to comment.