[PATCH] t/spawn: fix with unusual env locations

[PATCH] t/spawn: fix with unusual env locations

[Alyssa Ross]

The call to spawn clears the environment, including PATH.  This means
that an env in a non-default location wouldn't be found, while all the
other tests work because they use PATH.  We can fix this by looking up
which PATH to use beforehand.

I ran into this when packaging public-inbox for Nixpkgs.  We build in
a chroot, and in this case the env I wanted to use was at
/nix/store/7rmjki86923bw1inx0czpp4wgy0kk687-coreutils-8.31/bin/env.
---
 t/spawn.t | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/spawn.t b/t/spawn.t
index aba1a26..5549ca8 100644
--- a/t/spawn.t
+++ b/t/spawn.t
@@ -41,7 +41,8 @@ use PublicInbox::Spawn qw(which spawn popen_rd);
 {
 	my ($r, $w);
 	pipe $r, $w or die "pipe failed: $!";
-	my $pid = spawn(['env'], {}, { -env => 1, 1 => fileno($w) });
+	my $env = which('env');
+	my $pid = spawn([$env], {}, { -env => 1, 1 => fileno($w) });
 	close $w or die "close pipe[1] failed: $!";
 	ok(!defined(<$r>), 'read stdout of spawned from pipe');
 	is(waitpid($pid, 0), $pid, 'waitpid succeeds on spawned process');
-- 
2.22.1

Re: [PATCH] t/spawn: fix with unusual env locations

[Eric Wong]

Alyssa Ross <hi@alyssa.is> wrote:
> The call to spawn clears the environment, including PATH.  This means
> that an env in a non-default location wouldn't be found, while all the
> other tests work because they use PATH.  We can fix this by looking up
> which PATH to use beforehand.

Thanks for reporting this issue, but... is that it?
We also use spawn() for 'git', among other things...

> I ran into this when packaging public-inbox for Nixpkgs.  We build in
> a chroot, and in this case the env I wanted to use was at
> /nix/store/7rmjki86923bw1inx0czpp4wgy0kk687-coreutils-8.31/bin/env.

The Inline::C version of spawn (enabled by setting
PERL_INLINE_DIRECTORY) should already be using the result of
which() for every execve(2) call it does.

For the pure Perl codepath, this ought to be a more encompassing
alternative to your patch:

diff --git a/lib/PublicInbox/SpawnPP.pm b/lib/PublicInbox/SpawnPP.pm
index 25c8c87f..29b13371 100644
--- a/lib/PublicInbox/SpawnPP.pm
+++ b/lib/PublicInbox/SpawnPP.pm
@@ -38,11 +38,13 @@ sub pi_fork_exec ($$$$$$) {
 		}
 
 		if ($ENV{MOD_PERL}) {
-			exec qw(env -i), @$env, @$cmd;
+			exec which('env'), '-i', @$env, @$cmd;
 			die "exec env -i ... $cmd->[0] failed: $!\n";
 		} else {
 			local %ENV = map { split(/=/, $_, 2) } @$env;
-			exec @$cmd;
+			my @cmd = @$cmd;
+			$cmd[0] = $f;
+			exec @cmd;
 			die "exec $cmd->[0] failed: $!\n";
 		}
 	}

Can you confirm?  Thanks.

Re: [PATCH] t/spawn: fix with unusual env locations

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]

>> The call to spawn clears the environment, including PATH.  This means
>> that an env in a non-default location wouldn't be found, while all the
>> other tests work because they use PATH.  We can fix this by looking up
>> which PATH to use beforehand.
>
> Thanks for reporting this issue, but... is that it?
> We also use spawn() for 'git', among other things...

Yes, I can confirm that this was the only failing test case in my build.
(It may be of note however that I did not have Xapian enabled.  Trying
to build with that caused a host of other failures that were beyond my
ability to troubleshoot, so I decided to just give up on running the
tests for now, since they seem very dependent on the environment and the
environment inside our build sandboxes is so different from what
public-inbox will have on a running system anyway.)

> For the pure Perl codepath, this ought to be a more encompassing
> alternative to your patch:
>
> [ ... snip ... ]
>
> Can you confirm?  Thanks.

I can confirm that this patch also solves my problem.  Thank you very
much.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] t/spawn: fix with unusual env locations

[Eric Wong]

Alyssa Ross <hi@alyssa.is> wrote:
> >> The call to spawn clears the environment, including PATH.  This means
> >> that an env in a non-default location wouldn't be found, while all the
> >> other tests work because they use PATH.  We can fix this by looking up
> >> which PATH to use beforehand.
> >
> > Thanks for reporting this issue, but... is that it?
> > We also use spawn() for 'git', among other things...
> 
> Yes, I can confirm that this was the only failing test case in my build.

Thanks for confirming, pushed my patch as
4b3e278d9c4e5d1f55c1781aa0a7c9f0453f3852 to master.

In my original reply, I missed the test was using the "-env"
option to clear %ENV.  It's probably safe to drop the "-env"
option for spawn() to clear %ENV since we don't use it in real
code...

> (It may be of note however that I did not have Xapian enabled.  Trying
> to build with that caused a host of other failures that were beyond my
> ability to troubleshoot, so I decided to just give up on running the
> tests for now, since they seem very dependent on the environment and the
> environment inside our build sandboxes is so different from what
> public-inbox will have on a running system anyway.)

I don't know much about Nix, but I hope/expect public-inbox
tests to work on every reasonably modern Unix-like OS which
Perl5, git, and SQLite run on.

If you post logs of the failures you encountered, it would
improve the chances that somebody here (maybe not me) can
help fix them.  Thanks.

Re: Test failures in build sandbox

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 4516 bytes --]

Eric Wong <e@80x24.org> writes:

>> (It may be of note however that I did not have Xapian enabled.  Trying
>> to build with that caused a host of other failures that were beyond my
>> ability to troubleshoot, so I decided to just give up on running the
>> tests for now, since they seem very dependent on the environment and the
>> environment inside our build sandboxes is so different from what
>> public-inbox will have on a running system anyway.)
>
> I don't know much about Nix, but I hope/expect public-inbox
> tests to work on every reasonably modern Unix-like OS which
> Perl5, git, and SQLite run on.
>
> If you post logs of the failures you encountered, it would
> improve the chances that somebody here (maybe not me) can
> help fix them.  Thanks.

As requested, following a test log, edited down to just failures /
successes with error output:

PERL_DL_NONLAZY=1 "/nix/store/5r85g4zzk0djffzs47zaimvkxfmbpp3q-perl-5.30.0/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
t/convert-compact.t ........ 1/? error: unable to create temporary file: Operation not permitted
fatal: failed to write object
t/search.t ................. fatal: Could not make /build/pi-search-i646IK/a.git/branches/ writable by group
t/search.t ................. 1/?
#   Failed test 'git init (main)'
#   at t/search.t line 20.
#          got: '0'
#     expected: '32768'

#   Failed test 'undefined permission is group'
#   at t/search.t line 40.
#          got: '0'
#     expected: '432'
t/search.t ................. 92/?
#   Failed test 'sharedRepository respected for msgmap.sqlite3'
#   at t/search.t line 455.
#          got: '420'
#     expected: '432'

#   Failed test 'sharedRepository respected for public-inbox'
#   at t/search.t line 455.
#          got: '493'
#     expected: '1528'

#   Failed test 'sharedRepository respected for xapian15'
#   at t/search.t line 455.
#          got: '493'
#     expected: '1528'

#   Failed test 'sharedRepository respected for docdata.glass'
#   at t/search.t line 455.
#          got: '420'
#     expected: '432'

#   Failed test 'sharedRepository respected for flintlock'
#   at t/search.t line 455.
#          got: '420'
#     expected: '432'

#   Failed test 'sharedRepository respected for iamglass'
#   at t/search.t line 455.
#          got: '420'
#     expected: '432'

#   Failed test 'sharedRepository respected for over.sqlite3'
#   at t/search.t line 455.
#          got: '420'
#     expected: '432'

#   Failed test 'sharedRepository respected for over.sqlite3-journal'
#   at t/search.t line 455.
#          got: '420'
#     expected: '432'

#   Failed test 'sharedRepository respected for position.glass'
#   at t/search.t line 455.
#          got: '420'
#     expected: '432'

#   Failed test 'sharedRepository respected for postlist.glass'
#   at t/search.t line 455.
#          got: '420'
#     expected: '432'

#   Failed test 'sharedRepository respected for spelling.glass'
#   at t/search.t line 455.
#          got: '420'
#     expected: '432'

#   Failed test 'sharedRepository respected for synonym.glass'
#   at t/search.t line 455.
#          got: '420'
#     expected: '432'

#   Failed test 'sharedRepository respected for termlist.glass'
#   at t/search.t line 455.
#          got: '420'
#     expected: '432'
# Looks like you failed 15 tests of 114.
t/search.t ................. Dubious, test returned 15 (wstat 3840, 0xf00)
Failed 15/114 subtests
t/v2writable.t ............. 3/? error: unable to create temporary file: Operation not permitted
fatal: failed to write object
t/v2writable.t ............. 48/? error: unable to create temporary file: Operation not permitted
fatal: failed to write object
t/v2writable.t ............. 97/? error: unable to create temporary file: Operation not permitted
fatal: failed to write object
error: unable to create temporary file: Operation not permitted
fatal: failed to write object
t/v2writable.t ............. 101/? error: unable to create temporary file: Operation not permitted
fatal: failed to write object
t/v2writable.t ............. ok

Test Summary Report
-------------------
t/search.t               (Wstat: 3840 Tests: 114 Failed: 15)
  Failed tests:  1, 3, 93-105
  Non-zero exit status: 15
Files=84, Tests=2967, 574 wallclock secs ( 1.03 usr  0.21 sys + 85.43 cusr 102.57 csys = 189.24 CPU)
Result: FAIL
Failed 1/84 test programs. 15/2967 subtests failed.
make: *** [Makefile:1050: test_dynamic] Error 255

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: Test failures in build sandbox

[Eric Wong]

Alyssa Ross <hi@alyssa.is> wrote:
> Eric Wong <e@80x24.org> writes:
> 
> >> (It may be of note however that I did not have Xapian enabled.  Trying
> >> to build with that caused a host of other failures that were beyond my
> >> ability to troubleshoot, so I decided to just give up on running the
> >> tests for now, since they seem very dependent on the environment and the
> >> environment inside our build sandboxes is so different from what
> >> public-inbox will have on a running system anyway.)
> >
> > I don't know much about Nix, but I hope/expect public-inbox
> > tests to work on every reasonably modern Unix-like OS which
> > Perl5, git, and SQLite run on.
> >
> > If you post logs of the failures you encountered, it would
> > improve the chances that somebody here (maybe not me) can
> > help fix them.  Thanks.
> 
> As requested, following a test log, edited down to just failures /
> successes with error output:

Thanks.

> PERL_DL_NONLAZY=1 "/nix/store/5r85g4zzk0djffzs47zaimvkxfmbpp3q-perl-5.30.0/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
> t/convert-compact.t ........ 1/? error: unable to create temporary file: Operation not permitted
> fatal: failed to write object

What kind of filesystem and mount options was the code and
(if set, TMPDIR) on?

Since Nix seems to use odd paths, what's the default temporary
directory? ("/tmp" on my systems)

AFAIK, it should be possible to run tests with the
source/worktree on a read-only FS (only TMPDIR needs to be
writable); but it's not something that's fun/quick to test.

> t/search.t ................. fatal: Could not make /build/pi-search-i646IK/a.git/branches/ writable by group
> t/search.t ................. 1/?
> #   Failed test 'git init (main)'
> #   at t/search.t line 20.
> #          got: '0'
> #     expected: '32768'
> 
> #   Failed test 'undefined permission is group'
> #   at t/search.t line 40.
> #          got: '0'
> #     expected: '432'
> t/search.t ................. 92/?
> #   Failed test 'sharedRepository respected for msgmap.sqlite3'
> #   at t/search.t line 455.
> #          got: '420'
> #     expected: '432'
> 
> #   Failed test 'sharedRepository respected for public-inbox'
> #   at t/search.t line 455.
> #          got: '493'
> #     expected: '1528

<snip>

Sidenote, it would be easier to read if Test::More output those
as octal numbers.  I use iprint ("i") from Debian oldstable
installs, but it's no longer in Debian buster :<

printf '%0o\n' 493
printf '%0o\n' 1528
...
works, too, but it's more typing

Re: Test failures in build sandbox

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]

> What kind of filesystem and mount options was the code and
> (if set, TMPDIR) on?
> Since Nix seems to use odd paths, what's the default temporary
> directory? ("/tmp" on my systems)

TMPDIR is /build.  The code is extracted to /build/public-inbox.
Relevant mount output is:

    tmp on /build type tmpfs (rw,relatime)

>> t/search.t ................. fatal: Could not make /build/pi-search-i646IK/a.git/branches/ writable by group
>> t/search.t ................. 1/?
>> #   Failed test 'git init (main)'
>> #   at t/search.t line 20.
>> #          got: '0'
>> #     expected: '32768'

After some rather extensive debugging, I've determined that most of
these are to do with not being able to set the setgid bit.  If I mask it
off in git's adjust_shared_perm function, I get down to two failures.  I
don't know enough about filesystem permissions to know why this wouldn't
be allowed inside the sandbox if it is allowed normally, though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: Test failures in build sandbox

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

Alyssa Ross <hi@alyssa.is> writes:

> After some rather extensive debugging, I've determined that most of
> these are to do with not being able to set the setgid bit.  If I mask it
> off in git's adjust_shared_perm function, I get down to two failures.  I
> don't know enough about filesystem permissions to know why this wouldn't
> be allowed inside the sandbox if it is allowed normally, though.

Aha!  setuid and setgid permissions are disallowed in Nix builds using
seccomp[1].

So, I suppose it would be reasonable for us to either disable the test
suite, or try to disable just the tests that try to create setgid
things, unless you're open to adding a check for such a restriction and
skipping tests if so.  More than reasonable not to, though -- seccomp is
a nightmare and next to impossible to reasonably support as upstream.

[1]: https://github.com/NixOS/nix/blob/5038e1bec43a71c97ae7f8be07218a8a2edbb6a1/src/libstore/build.cc#L2678

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: Test failures in build sandbox

[Eric Wong]

Alyssa Ross <hi@alyssa.is> wrote:
> Alyssa Ross <hi@alyssa.is> writes:
> 
> > After some rather extensive debugging, I've determined that most of
> > these are to do with not being able to set the setgid bit.  If I mask it
> > off in git's adjust_shared_perm function, I get down to two failures.  I
> > don't know enough about filesystem permissions to know why this wouldn't
> > be allowed inside the sandbox if it is allowed normally, though.
> 
> Aha!  setuid and setgid permissions are disallowed in Nix builds using
> seccomp[1].
> 
> So, I suppose it would be reasonable for us to either disable the test
> suite, or try to disable just the tests that try to create setgid
> things, unless you're open to adding a check for such a restriction and
> skipping tests if so.  More than reasonable not to, though -- seccomp is
> a nightmare and next to impossible to reasonably support as upstream.

How do "git init --shared" tests work for the Nix git package?  We could
try to follow that as an example...

> 
> [1]: https://github.com/NixOS/nix/blob/5038e1bec43a71c97ae7f8be07218a8a2edbb6a1/src/libstore/build.cc#L2678

Re: Test failures in build sandbox

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 258 bytes --]

> How do "git init --shared" tests work for the Nix git package?  We could
> try to follow that as an example...

They're disabled, hehe:

<https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/version-management/git-and-tools/git/default.nix#L274>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[PATCH] t/search: bail out on `git init --shared' failures

[Eric Wong]

Alyssa Ross <hi@alyssa.is> wrote:
> > How do "git init --shared" tests work for the Nix git package?  We could
> > try to follow that as an example...
> 
> They're disabled, hehe:
> 
> <https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/version-management/git-and-tools/git/default.nix#L274>

Yeah, seems reasonable to disable here, too.

Thanks for all your time debugging and documenting this!  Using
BAIL_OUT could save future testers a lot of time.  And I also
made another patch to make mode failures more obvious:
https://public-inbox.org/meta/20191003073817.13259-1-e@80x24.org/

----------8<-----------
Subject: [PATCH] t/search: bail out on `git init --shared' failures

We can save future testers some time if we bail out early
on "git init --shared" failures, since things like seccomp
or non-POSIX FSes would trigger failures.

BAIL_OUT has been in Test::Simple since Perl v5.10.0, so it's
old-enough to call for our purposes.

Thanks-to: Alyssa Ross <hi@alyssa.is>
Link: https://public-inbox.org/meta/878sq2hd08.fsf@alyssa.is/
---
 t/search.t | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/search.t b/t/search.t
index 830c668e..a728d79f 100644
--- a/t/search.t
+++ b/t/search.t
@@ -17,7 +17,8 @@ my $git_dir = "$tmpdir/a.git";
 my $ibx = PublicInbox::Inbox->new({ mainrepo => $git_dir });
 my ($root_id, $last_id);
 
-is(0, system(qw(git init --shared -q --bare), $git_dir), "git init (main)");
+is(0, system(qw(git init --shared -q --bare), $git_dir), "git init (main)")
+	or BAIL_OUT("`git init --shared' failed, weird FS or seccomp?");
 eval { PublicInbox::Search->new($ibx)->xdb };
 ok($@, "exception raised on non-existent DB");
 
-- 
EW

Re: [PATCH] t/search: bail out on `git init --shared' failures

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 189 bytes --]

> Subject: [PATCH] t/search: bail out on `git init --shared' failures

Nice one!  If you want to, consider it

Reviewed-by: Alyssa Ross <hi@alyssa.is>
Tested-by: Alyssa Ross <hi@alyssa.is>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]