From d62a2c16574113828b36e363b9c332be9ca31b23 Mon Sep 17 00:00:00 2001 From: Graham Christensen Date: Wed, 17 Mar 2021 11:07:19 -0400 Subject: [PATCH 1/4] NixExprs: extract the `escape` function and test it --- src/lib/Hydra/Helper/Escape.pm | 14 ++++++++++++++ src/lib/Hydra/View/NixExprs.pm | 25 +++++++++---------------- t/Helper/escape.t | 25 +++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 16 deletions(-) create mode 100644 src/lib/Hydra/Helper/Escape.pm create mode 100644 t/Helper/escape.t diff --git a/src/lib/Hydra/Helper/Escape.pm b/src/lib/Hydra/Helper/Escape.pm new file mode 100644 index 00000000..3312c5ea --- /dev/null +++ b/src/lib/Hydra/Helper/Escape.pm @@ -0,0 +1,14 @@ +package Hydra::Helper::Escape; + +use strict; +use base qw(Exporter); + +our @EXPORT = qw(escapeString); + +sub escapeString { + my ($s) = @_; + $s =~ s|\\|\\\\|g; + $s =~ s|\"|\\\"|g; + $s =~ s|\$|\\\$|g; + return "\"" . $s . "\""; +} diff --git a/src/lib/Hydra/View/NixExprs.pm b/src/lib/Hydra/View/NixExprs.pm index 7bfa3109..fa2f7086 100644 --- a/src/lib/Hydra/View/NixExprs.pm +++ b/src/lib/Hydra/View/NixExprs.pm @@ -3,19 +3,12 @@ package Hydra::View::NixExprs; use strict; use base qw/Catalyst::View/; use Hydra::Helper::Nix; +use Hydra::Helper::Escape; use Archive::Tar; use IO::Compress::Bzip2 qw(bzip2); use Encode; -sub escape { - my ($s) = @_; - $s =~ s|\\|\\\\|g; - $s =~ s|\"|\\\"|g; - $s =~ s|\$|\\\$|g; - return "\"" . $s . "\""; -} - sub process { my ($self, $c) = @_; @@ -62,7 +55,7 @@ EOF my $first = 1; foreach my $system (keys %perSystem) { $res .= "else " if !$first; - $res .= "if system == ${\escape $system} then {\n\n"; + $res .= "if system == ${\escapeString $system} then {\n\n"; foreach my $job (keys %{$perSystem{$system}}) { my $pkg = $perSystem{$system}->{$job}; @@ -70,21 +63,21 @@ EOF $res .= " # Hydra build ${\$build->id}\n"; my $attr = $build->get_column('job'); $attr =~ s/\./-/g; - $res .= " ${\escape $attr} = (mkFakeDerivation {\n"; + $res .= " ${\escapeString $attr} = (mkFakeDerivation {\n"; $res .= " type = \"derivation\";\n"; - $res .= " name = ${\escape ($build->get_column('releasename') or $build->nixname)};\n"; - $res .= " system = ${\escape $build->system};\n"; + $res .= " name = ${\escapeString ($build->get_column('releasename') or $build->nixname)};\n"; + $res .= " system = ${\escapeString $build->system};\n"; $res .= " meta = {\n"; - $res .= " description = ${\escape $build->description};\n" + $res .= " description = ${\escapeString $build->description};\n" if $build->description; - $res .= " license = ${\escape $build->license};\n" + $res .= " license = ${\escapeString $build->license};\n" if $build->license; - $res .= " maintainers = ${\escape $build->maintainers};\n" + $res .= " maintainers = ${\escapeString $build->maintainers};\n" if $build->maintainers; $res .= " };\n"; $res .= " } {\n"; my @outputNames = sort (keys %{$pkg->{outputs}}); - $res .= " ${\escape $_} = ${\escape $pkg->{outputs}->{$_}};\n" foreach @outputNames; + $res .= " ${\escapeString $_} = ${\escapeString $pkg->{outputs}->{$_}};\n" foreach @outputNames; my $out = defined $pkg->{outputs}->{"out"} ? "out" : $outputNames[0]; $res .= " }).$out;\n\n"; } diff --git a/t/Helper/escape.t b/t/Helper/escape.t new file mode 100644 index 00000000..f614cec3 --- /dev/null +++ b/t/Helper/escape.t @@ -0,0 +1,25 @@ +use strict; +use Setup; +use Data::Dumper; +use Test2::V0; +use Hydra::Helper::Escape; + +subtest "checking individual attribute set elements" => sub { + my %values = ( + "" => '""', + "." => '"."', + "foobar" => '"foobar"', + "foo.bar" => '"foo.bar"', + "🌮" => '"🌮"', + 'foo"bar' => '"foo\"bar"', + 'foo\\bar' => '"foo\\\\bar"', + '$bar' => '"\\$bar"', + ); + + for my $input (keys %values) { + my $value = $values{$input}; + is(escapeString($input), $value, "Escaping the value: " . $input); + } +}; + +done_testing; From 88e0198a8ec92060328a42fee4aa54e044fe08e4 Mon Sep 17 00:00:00 2001 From: Graham Christensen Date: Wed, 17 Mar 2021 11:53:00 -0400 Subject: [PATCH 2/4] Create a helper for dealing with nested attribute sets --- src/lib/Hydra/Helper/AttributeSet.pm | 56 ++++++++++++++++++++++++++++ src/lib/Hydra/Helper/Escape.pm | 9 ++++- t/Helper/attributeset.t | 53 ++++++++++++++++++++++++++ t/Helper/escape.t | 19 ++++++++++ 4 files changed, 136 insertions(+), 1 deletion(-) create mode 100644 src/lib/Hydra/Helper/AttributeSet.pm create mode 100644 t/Helper/attributeset.t diff --git a/src/lib/Hydra/Helper/AttributeSet.pm b/src/lib/Hydra/Helper/AttributeSet.pm new file mode 100644 index 00000000..b750d6e1 --- /dev/null +++ b/src/lib/Hydra/Helper/AttributeSet.pm @@ -0,0 +1,56 @@ +package Hydra::Helper::AttributeSet; + +use strict; +use warnings; + +sub new { + my ($self) = @_; + return bless { "paths" => [] }, $self; +} + +sub registerValue { + my ($self, $attributePath) = @_; + + my @pathParts = splitPath($attributePath); + + pop(@pathParts); + if (scalar(@pathParts) == 0) { + return; + } + + my $lineage = ""; + for my $pathPart (@pathParts) { + $lineage = $self->registerChild($lineage, $pathPart); + } +} + +sub registerChild { + my ($self, $parent, $attributePath) = @_; + if ($parent ne "") { + $parent .= "." + } + + my $name = $parent . $attributePath; + if (!grep { $_ eq $name} @{$self->{"paths"}}) { + push(@{$self->{"paths"}}, $name); + } + return $name; +} + +sub splitPath { + my ($s) = @_; + + if ($s eq "") { + return ('') + } + + return split(/\./, $s, -1); +} + +sub enumerate { + my ($self) = @_; + my @paths = sort { length($a) <=> length($b) } @{$self->{"paths"}}; + return wantarray ? @paths : \@paths; +} + +1; diff --git a/src/lib/Hydra/Helper/Escape.pm b/src/lib/Hydra/Helper/Escape.pm index 3312c5ea..f7682a4f 100644 --- a/src/lib/Hydra/Helper/Escape.pm +++ b/src/lib/Hydra/Helper/Escape.pm @@ -2,8 +2,9 @@ package Hydra::Helper::Escape; use strict; use base qw(Exporter); +use Hydra::Helper::AttributeSet; -our @EXPORT = qw(escapeString); +our @EXPORT = qw(escapeString escapeAttributePath); sub escapeString { my ($s) = @_; @@ -12,3 +13,9 @@ sub escapeString { $s =~ s|\$|\\\$|g; return "\"" . $s . "\""; } + +sub escapeAttributePath { + my ($s) = @_; + + return join(".", map( { escapeString($_) } Hydra::Helper::AttributeSet::splitPath($s))); +} diff --git a/t/Helper/attributeset.t b/t/Helper/attributeset.t new file mode 100644 index 00000000..112cd9be --- /dev/null +++ b/t/Helper/attributeset.t @@ -0,0 +1,53 @@ +use strict; +use warnings; +use Setup; +use Data::Dumper; +use Test2::V0; +use Hydra::Helper::AttributeSet; + + +subtest "splitting an attribute path in to its component parts" => sub { + my %values = ( + "" => [''], + "." => ['', ''], + "...." => ['', '', '', '', ''], + "foobar" => ['foobar'], + "foo.bar" => ['foo', 'bar'], + "🌮" => ['🌮'], + + # not supported: 'foo."bar.baz".tux' => [ 'foo', 'bar.baz', 'tux' ] + # the edge cases are fairly significant around escaping and unescaping. + ); + + for my $input (keys %values) { + my @value = @{$values{$input}}; + my @components = Hydra::Helper::AttributeSet::splitPath($input); + is(\@components, \@value, "Splitting the attribute path: " . $input); + } +}; + +my $attrs = Hydra::Helper::AttributeSet->new(); +$attrs->registerValue("foo"); +$attrs->registerValue("bar.baz.tux"); +$attrs->registerValue("bar.baz.bux.foo.bar.baz"); + +is( + $attrs->enumerate(), + [ + # "foo": skipped since we're registering values, and we + # only want to track nested attribute sets. + + # "bar.baz.tux": expand the path + "bar", + "bar.baz", + + #"bar.baz.bux.foo.bar.baz": expand the path, but only register new + # attribute set names. + "bar.baz.bux", + "bar.baz.bux.foo", + "bar.baz.bux.foo.bar", + ], + "Attribute set paths are registered." +); + +done_testing; diff --git a/t/Helper/escape.t b/t/Helper/escape.t index f614cec3..22dd4d47 100644 --- a/t/Helper/escape.t +++ b/t/Helper/escape.t @@ -22,4 +22,23 @@ subtest "checking individual attribute set elements" => sub { } }; +subtest "escaping path components of a nested attribute" => sub { + my %values = ( + "" => '""', + "." => '"".""', + "...." => '""."".""."".""', + "foobar" => '"foobar"', + "foo.bar" => '"foo"."bar"', + "🌮" => '"🌮"', + 'foo"bar' => '"foo\"bar"', + 'foo\\bar' => '"foo\\\\bar"', + '$bar' => '"\\$bar"', + ); + + for my $input (keys %values) { + my $value = $values{$input}; + is(escapeAttributePath($input), $value, "Escaping the attribute path: " . $input); + } +}; + done_testing; From 019aef3d419b03266f7a137300b54bcfa167350e Mon Sep 17 00:00:00 2001 From: Graham Christensen Date: Wed, 17 Mar 2021 11:20:19 -0400 Subject: [PATCH 3/4] Test the fake derivations channel, asserting nested packages are properly represented. This is a breaking change. Previously, packages named `packageset.foo` would be exposed in the fake derivation channel as `packageset-foo`. Presumably this was done to avoid needing to track attribute sets, and to avoid the complexity. I think this now correctly handles the complexity and properly mirrors the input expressions layout. --- src/lib/Hydra/View/NixExprs.pm | 16 ++++++--- t/Controller/Jobset/channel.t | 62 ++++++++++++++++++++++++++++++++++ t/jobs/nested-attributes.nix | 36 ++++++++++++++++++++ 3 files changed, 109 insertions(+), 5 deletions(-) create mode 100644 t/Controller/Jobset/channel.t create mode 100644 t/jobs/nested-attributes.nix diff --git a/src/lib/Hydra/View/NixExprs.pm b/src/lib/Hydra/View/NixExprs.pm index fa2f7086..194c51ec 100644 --- a/src/lib/Hydra/View/NixExprs.pm +++ b/src/lib/Hydra/View/NixExprs.pm @@ -4,10 +4,11 @@ use strict; use base qw/Catalyst::View/; use Hydra::Helper::Nix; use Hydra::Helper::Escape; +use Hydra::Helper::AttributeSet; use Archive::Tar; use IO::Compress::Bzip2 qw(bzip2); use Encode; - +use Data::Dumper; sub process { @@ -56,14 +57,15 @@ EOF foreach my $system (keys %perSystem) { $res .= "else " if !$first; $res .= "if system == ${\escapeString $system} then {\n\n"; - + my $attrsets = Hydra::Helper::AttributeSet->new(); foreach my $job (keys %{$perSystem{$system}}) { my $pkg = $perSystem{$system}->{$job}; my $build = $pkg->{build}; - $res .= " # Hydra build ${\$build->id}\n"; my $attr = $build->get_column('job'); - $attr =~ s/\./-/g; - $res .= " ${\escapeString $attr} = (mkFakeDerivation {\n"; + $attrsets->registerValue($attr); + + $res .= " # Hydra build ${\$build->id}\n"; + $res .= " ${\escapeAttributePath $attr} = (mkFakeDerivation {\n"; $res .= " type = \"derivation\";\n"; $res .= " name = ${\escapeString ($build->get_column('releasename') or $build->nixname)};\n"; $res .= " system = ${\escapeString $build->system};\n"; @@ -82,6 +84,10 @@ EOF $res .= " }).$out;\n\n"; } + for my $attrset ($attrsets->enumerate()) { + $res .= " ${\escapeAttributePath $attrset}.recurseForDerivations = true;\n\n"; + } + $res .= "}\n\n"; $first = 0; } diff --git a/t/Controller/Jobset/channel.t b/t/Controller/Jobset/channel.t new file mode 100644 index 00000000..2b034025 --- /dev/null +++ b/t/Controller/Jobset/channel.t @@ -0,0 +1,62 @@ +use feature 'unicode_strings'; +use strict; +use Setup; +use IO::Uncompress::Bunzip2 qw(bunzip2); +use Archive::Tar; +use JSON qw(decode_json); +use Data::Dumper; +my %ctx = test_init(); + +require Hydra::Schema; +require Hydra::Model::DB; +require Hydra::Helper::Nix; + +use Test2::V0; +require Catalyst::Test; +Catalyst::Test->import('Hydra'); + +my $db = Hydra::Model::DB->new; +hydra_setup($db); + +my $project = $db->resultset('Projects')->create({name => "tests", displayname => "", owner => "root"}); + +# Most basic test case, no parameters +my $jobset = createBaseJobset("nested-attributes", "nested-attributes.nix", $ctx{jobsdir}); + +ok(evalSucceeds($jobset)); +is(nrQueuedBuildsForJobset($jobset), 4); + +for my $build (queuedBuildsForJobset($jobset)) { + ok(runBuild($build), "Build '".$build->job."' should exit with code 0"); + my $newbuild = $db->resultset('Builds')->find($build->id); + is($newbuild->finished, 1, "Build '".$build->job."' should be finished."); + is($newbuild->buildstatus, 0, "Build '".$build->job."' should have buildstatus 0."); +} + +my $compressed = get('/jobset/tests/nested-attributes/channel/latest/nixexprs.tar.bz2'); +my $tarcontent; +bunzip2(\$compressed => \$tarcontent); +open(my $tarfh, "<", \$tarcontent); +my $tar = Archive::Tar->new($tarfh); + +my $defaultnix = $ctx{"tmpdir"} . "/channel-default.nix"; +$tar->extract_file("channel/default.nix", $defaultnix); + +print STDERR $tar->get_content("channel/default.nix"); + +(my $status, my $stdout, my $stderr) = Hydra::Helper::Nix::captureStdoutStderr(5, "nix-env", "--json", "--query", "--available", "--attr-path", "--file", $defaultnix); +is($stderr, "", "Stderr should be empty"); +is($status, 0, "Querying the packages should succeed"); + +my $packages = decode_json($stdout); +my $keys = [sort keys %$packages]; +is($keys, [ + "packageset-nested", + "packageset.deeper.deeper.nested", + "packageset.nested", + "packageset.nested2", +]); +is($packages->{"packageset-nested"}->{"name"}, "actually-top-level"); +is($packages->{"packageset.nested"}->{"name"}, "actually-nested"); + +done_testing; diff --git a/t/jobs/nested-attributes.nix b/t/jobs/nested-attributes.nix new file mode 100644 index 00000000..4cd90d9b --- /dev/null +++ b/t/jobs/nested-attributes.nix @@ -0,0 +1,36 @@ +with import ./config.nix; +rec { + # Given a jobset containing a package set named X with an interior member Y, + # expose the interior member Y with the name X-Y. This is to exercise a bug + # in the NixExprs view's generated Nix expression which flattens the + # package set namespace from `X.Y` to `X-Y`. If the bug is present, the + # resulting expression incorrectly renders two `X-Y` packages. + packageset = { + recurseForDerivations = true; + deeper = { + recurseForDerivations = true; + deeper = { + recurseForDerivations = true; + + nested = mkDerivation { + name = "much-too-deep"; + builder = ./empty-dir-builder.sh; + }; + }; + }; + + nested = mkDerivation { + name = "actually-nested"; + builder = ./empty-dir-builder.sh; + }; + + nested2 = mkDerivation { + name = "actually-nested2"; + builder = ./empty-dir-builder.sh; + }; + }; + packageset-nested = mkDerivation { + name = "actually-top-level"; + builder = ./empty-dir-builder.sh; + }; +} From 6b7ca554f95425b2e86117f4a990bb3755edba83 Mon Sep 17 00:00:00 2001 From: Graham Christensen Date: Thu, 18 Mar 2021 16:27:21 -0400 Subject: [PATCH 4/4] Update src/lib/Hydra/Helper/Escape.pm: fewer ()s Co-authored-by: Stig --- src/lib/Hydra/Helper/Escape.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/Hydra/Helper/Escape.pm b/src/lib/Hydra/Helper/Escape.pm index f7682a4f..3037951f 100644 --- a/src/lib/Hydra/Helper/Escape.pm +++ b/src/lib/Hydra/Helper/Escape.pm @@ -17,5 +17,5 @@ sub escapeString { sub escapeAttributePath { my ($s) = @_; - return join(".", map( { escapeString($_) } Hydra::Helper::AttributeSet::splitPath($s))); + return join ".", map { escapeString($_) } Hydra::Helper::AttributeSet::splitPath($s); }