Skip to content

Commit

Permalink
Fix extracting compressed downloaded files
Browse files Browse the repository at this point in the history
This regressed in #2736. When we want to download a compressed
file and extract it to produce the 'final' asset, we'll be doing
something like downloading 'http://example.com/file.img.gz' to
an asset called 'file.img'. After we download the asset, we write
it to a temporary file and feed that temporary file into
Archive::Extract, then extract it to the final asset location.
Archive::Extract uses the filename of the input file to determine
its type, so we should base the name of the temporary file on the
path component of the URL (where the compression is indicated) -
'file.img.gz' in this case - and not on the final target filename
(where the compression is not indicated) - 'file.img' in this
case. Before #2736, that's what the code did, but in #2736 it was
changed so the temporary filename is based on the target file.
I guess it was hard for @kraih to work this out without a real-
world example to understand how it's actually used in practice
(I think only Fedora uses this feature).

This returns the code to something similar to how it was before,
but using `path()->to_string` as @kraih's version did. It also
tweaks the test cases, which were clearly the wrong way round
once you know how this is supposed to work (it doesn't make
sense to expect extracting 'test.gz' to 'test.gz' to succeed,
but extracting 'test.gz' to 'test' to fail).

Signed-off-by: Adam Williamson <awilliam@redhat.com>
  • Loading branch information
AdamWill committed Mar 20, 2020
1 parent 2966923 commit 01f6178
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 9 deletions.
2 changes: 1 addition & 1 deletion lib/OpenQA/Downloader.pm
Expand Up @@ -97,7 +97,7 @@ sub _get {
if ($size == $headers->content_length) {

if ($options->{extract}) {
my $tempfile = path($ENV{MOJO_TMPDIR}, $file)->to_string;
my $tempfile = path($ENV{MOJO_TMPDIR}, Mojo::URL->new($url)->path->parts->[-1])->to_string;
$log->info(qq{Extracting "$tempfile" to "$target"});
$asset->move_to($tempfile);

Expand Down
16 changes: 8 additions & 8 deletions t/25-downloader.t
Expand Up @@ -160,29 +160,29 @@ subtest 'Size differs' => sub {
};

subtest 'Decompressing archive failed' => sub {
$to = $tempdir->child('test');
my $from = "http://$host/test.gz";
$to = $tempdir->child('test.gz');
my $from = "http://$host/test";
ok !$downloader->download($from, $to, {extract => 1}), 'Failed';

ok !-e $to, 'File not downloaded';

like $cache_log, qr/Downloading "test" from "$from"/, 'Download attempt';
like $cache_log, qr/Extracting ".*test" to ".*test"/, 'Extracting download';
like $cache_log, qr/Downloading "test.gz" from "$from"/, 'Download attempt';
like $cache_log, qr/Extracting ".*test" to ".*test.gz"/, 'Extracting download';
like $cache_log, qr/Extracting ".*test" failed: Could not determine archive type/, 'Extracting failed';
$cache_log = '';
};

subtest 'Decompressing archive' => sub {
$to = $tempdir->child('test.gz');
$to = $tempdir->child('test');
my $from = "http://$host/test.gz";
ok $downloader->download($from, $to, {extract => 1}), 'Success';

ok -e $to, 'File downloaded';
is $to->slurp, 'This file was compressed!', 'File was decompressed';

like $cache_log, qr/Downloading "test.gz" from "$from"/, 'Download attempt';
like $cache_log, qr/Extracting ".*test.gz" to ".*test.gz"/, 'Extracting download';
unlike $cache_log, qr/Extracting ".*test" failed:/, 'Extracting did not fail';
like $cache_log, qr/Downloading "test" from "$from"/, 'Download attempt';
like $cache_log, qr/Extracting ".*test.gz" to ".*test"/, 'Extracting download';
unlike $cache_log, qr/Extracting ".*test.gz" failed:/, 'Extracting did not fail';
$cache_log = '';
};

Expand Down
5 changes: 5 additions & 0 deletions t/lib/OpenQA/Test/Utils.pm
Expand Up @@ -89,6 +89,11 @@ sub fake_asset_server {
my $archive = gzip 'This file was compressed!';
$c->render(data => $archive);
});
$r->get(
'/test' => sub {
my $c = shift;
$c->render(text => 'This file was not compressed!', format => 'txt');
});
$r->get(
'/tests/:job/asset/:type/:filename' => sub {
my $c = shift;
Expand Down

0 comments on commit 01f6178

Please sign in to comment.