Merge pull request #1521 from NixOS/download-regression

Fix download regression

Author: Mic92

src/lib/Hydra/Controller/Build.pm

@@ -212,7 +212,7 @@ sub checkPath {
 sub serveFile {
     my ($c, $path) = @_;
 
-    my $res = run(cmd => ["nix", "--experimental-features", "nix-command",
+    my $res = runCommand(cmd => ["nix", "--experimental-features", "nix-command",
                           "ls-store", "--store", getStoreUri(), "--json", "$path"]);
 
     if ($res->{status}) {

src/lib/Hydra/Helper/Nix.pm

@@ -44,7 +44,7 @@ our @EXPORT = qw(
     readNixFile
     registerRoot
     restartBuilds
-    run
+    runCommand
     $MACHINE_LOCAL_STORE
     );
 
@@ -466,7 +466,7 @@ sub readIntoSocket{
 
 
 
-sub run {
+sub runCommand {
     my (%args) = @_;
     my $res = { stdout => "", stderr => "" };
     my $stdin = "";
@@ -506,7 +506,7 @@ sub run {
 
 sub grab {
     my (%args) = @_;
-    my $res = run(%args, grabStderr => 0);
+    my $res = runCommand(%args, grabStderr => 0);
     if ($res->{status}) {
         my $msgloc = "(in an indeterminate location)";
         if (defined $args{dir}) {

src/lib/Hydra/Plugin/DarcsInput.pm

@@ -32,7 +32,7 @@ sub fetchInput {
     my $stdout = ""; my $stderr = ""; my $res;
     if (! -d $clonePath) {
         # Clone the repository.
-        $res = run(timeout => 600,
+        $res = runCommand(timeout => 600,
                    cmd => ["darcs", "get", "--lazy", $uri, $clonePath],
                    dir => $ENV{"TMPDIR"});
         die "Error getting darcs repo at `$uri':\n$stderr" if $res->{status};

src/lib/Hydra/Plugin/GitInput.pm

@@ -137,35 +137,35 @@ sub fetchInput {
     my $res;
     if (! -d $clonePath) {
         # Clone everything and fetch the branch.
-        $res = run(cmd => ["git", "init", $clonePath]);
-        $res = run(cmd => ["git", "remote", "add", "origin", "--", $uri], dir => $clonePath) unless $res->{status};
+        $res = runCommand(cmd => ["git", "init", $clonePath]);
+        $res = runCommand(cmd => ["git", "remote", "add", "origin", "--", $uri], dir => $clonePath) unless $res->{status};
         die "error creating git repo in `$clonePath':\n$res->{stderr}" if $res->{status};
     }
 
     # This command forces the update of the local branch to be in the same as
     # the remote branch for whatever the repository state is.  This command mirrors
     # only one branch of the remote repository.
     my $localBranch = _isHash($branch) ? "_hydra_tmp" : $branch;
-    $res = run(cmd => ["git", "fetch", "-fu", "origin", "+$branch:$localBranch"], dir => $clonePath,
+    $res = runCommand(cmd => ["git", "fetch", "-fu", "origin", "+$branch:$localBranch"], dir => $clonePath,
                timeout => $cfg->{timeout});
-    $res = run(cmd => ["git", "fetch", "-fu", "origin"], dir => $clonePath, timeout => $cfg->{timeout}) if $res->{status};
+    $res = runCommand(cmd => ["git", "fetch", "-fu", "origin"], dir => $clonePath, timeout => $cfg->{timeout}) if $res->{status};
     die "error fetching latest change from git repo at `$uri':\n$res->{stderr}" if $res->{status};
 
     # If deepClone is defined, then we look at the content of the repository
     # to determine if this is a top-git branch.
     if (defined $deepClone) {
 
         # Is the target branch a topgit branch?
-        $res = run(cmd => ["git", "ls-tree", "-r", "$branch", ".topgit"], dir => $clonePath);
+        $res = runCommand(cmd => ["git", "ls-tree", "-r", "$branch", ".topgit"], dir => $clonePath);
 
         if ($res->{stdout} ne "") {
             # Checkout the branch to look at its content.
-            $res = run(cmd => ["git", "checkout", "--force", "$branch"], dir => $clonePath);
+            $res = runCommand(cmd => ["git", "checkout", "--force", "$branch"], dir => $clonePath);
             die "error checking out Git branch '$branch' at `$uri':\n$res->{stderr}" if $res->{status};
 
             # This is a TopGit branch.  Fetch all the topic branches so
             # that builders can run "tg patch" and similar.
-            $res = run(cmd => ["tg", "remote", "--populate", "origin"], dir => $clonePath, timeout => $cfg->{timeout});
+            $res = runCommand(cmd => ["tg", "remote", "--populate", "origin"], dir => $clonePath, timeout => $cfg->{timeout});
             print STDERR "warning: `tg remote --populate origin' failed:\n$res->{stderr}" if $res->{status};
         }
     }

t/Hydra/Controller/Build/download.t

@@ -0,0 +1,74 @@
+use strict;
+use warnings;
+use Setup;
+use Test2::V0;
+use Catalyst::Test ();
+use HTTP::Request::Common;
+
+my %ctx = test_init();
+
+Catalyst::Test->import('Hydra');
+
+my $db = Hydra::Model::DB->new;
+hydra_setup($db);
+
+my $project = $db->resultset('Projects')->create({name => "tests", displayname => "", owner => "root"});
+
+# Create a simple Nix expression that uses the existing build-product-simple.sh
+my $jobsdir = $ctx{jobsdir};
+my $nixfile = "$jobsdir/simple.nix";
+open(my $fh, '>', $nixfile) or die "Cannot create simple.nix: $!";
+print $fh <<"EOF";
+with import ./config.nix;
+{
+  simple = mkDerivation {
+    name = "build-product-simple";
+    builder = ./build-product-simple.sh;
+  };
+}
+EOF
+close($fh);
+
+# Create a jobset that uses the simple build
+my $jobset = createBaseJobset("simple", "simple.nix", $ctx{jobsdir});
+
+ok(evalSucceeds($jobset), "Evaluating simple.nix should succeed");
+is(nrQueuedBuildsForJobset($jobset), 1, "Should have 1 build queued");
+
+my $build = (queuedBuildsForJobset($jobset))[0];
+ok(runBuild($build), "Build should succeed");
+
+$build->discard_changes();
+
+subtest "Test downloading build products (regression test for #1520)" => sub {
+    # Get the build URL
+    my $build_id = $build->id;
+
+    # First, check that the build has products
+    my @products = $build->buildproducts;
+    ok(scalar @products >= 1, "Build should have at least 1 product");
+
+    # Find the doc product (created by build-product-simple.sh)
+    my ($doc_product) = grep { $_->type eq "doc" } @products;
+    ok($doc_product, "Should have a doc product");
+
+    if ($doc_product) {
+        # Test downloading via the download endpoint
+        # This tests the serveFile function which was broken in #1520
+        my $download_url = "/build/$build_id/download/" . $doc_product->productnr . "/text.txt";
+        my $response = request(GET $download_url);
+
+        # The key test: should not return 500 error with "Can't use string ("1") as a HASH ref"
+        isnt($response->code, 500, "Download should not return 500 error (regression test for #1520)");
+        is($response->code, 200, "Download should succeed with 200")
+            or diag("Response code: " . $response->code . ", Content: " . $response->content);
+
+        like($response->header('Content-Security-Policy') // '', qr/\bsandbox\b/, 'CSP header present with sandbox');
+
+        # Check that we get actual content
+        ok(length($response->content) > 0, "Should receive file content");
+        is($response->content, "Hello\n", "Should get expected content");
+    }
+};
+
+done_testing();