Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failures in high level script functionality on illumos #5472

Closed
Mno-hime opened this issue Jan 3, 2019 · 8 comments
Closed

Failures in high level script functionality on illumos #5472

Mno-hime opened this issue Jan 3, 2019 · 8 comments

Comments

@Mno-hime
Copy link

Mno-hime commented Jan 3, 2019

On Openindiana I see following failures in "high level script functionality" test:

Testing file cd.in ... fail
Error output differs for file cd.in. Diff follows:
--- cd.tmp.err	2019-01-03 07:36:57.439371310 +0000
+++ cd.err	2019-01-03 07:36:22.809496268 +0000
@@ -4,9 +4,3 @@
 
 ####################
 # cd symlink completion
-rm: Cannot remove any directory in the path of the current working directory
-/tmp/cdcomp_test//through/the/looking/glass
-rm: Unable to remove directory /tmp/cdcomp_test//through/the/looking: Directory not empty
-rm: Unable to remove directory /tmp/cdcomp_test//through/the: Directory not empty
-rm: Unable to remove directory /tmp/cdcomp_test//through: Directory not empty
-rm: Unable to remove directory /tmp/cdcomp_test/: Directory not empty
Exit status differs for file cd.in.
Unexpected test exit status 2.

For cases like this, where grep option is required (-o here), which our grep does not have, we prepend /usr/gnu/bin in $PATH, so GNU utilities are executed preferentially. However, from unknown reason it fails here:

Testing file psub.in ... fail
Output differs for file psub.in. Diff follows:
--- psub.tmp.out	2019-01-03 07:36:58.476779984 +0000
+++ psub.out	2019-01-03 07:36:22.789465657 +0000
@@ -2,6 +2,6 @@
 bar
 baz
 psub file was deleted
-psub filename does not end with .cc
-psub filename does not end with .cc
+psub filename ends with .cc
+psub filename ends with .cc
 psub directory was deleted
Error output differs for file psub.in. Diff follows:
--- psub.tmp.err	2019-01-03 07:36:58.450481324 +0000
+++ psub.err	2019-01-03 07:36:22.812413444 +0000
@@ -1,6 +0,0 @@
-grep: illegal option -- o
-usage:  grep [-E|-F] [-bchHilnqrRsvx] [-A num] [-B num] [-C num|-num]
-             [-e pattern_list]... [-f pattern_file]... [pattern_list] [file]...
-grep: illegal option -- o
-usage:  grep [-E|-F] [-bchHilnqrRsvx] [-A num] [-B num] [-C num|-num]
-             [-e pattern_list]... [-f pattern_file]... [pattern_list] [file]...
Testing file realpath.in ... fail
Output differs for file realpath.in. Diff follows:
--- realpath.tmp.out	2019-01-03 07:36:58.892421022 +0000
+++ realpath.out	2019-01-03 07:36:22.789097243 +0000
@@ -5,7 +5,6 @@
 /x
 /abc
 /def
-/tmp/tmp.vOyXE8Ylyr
 fish-symlink handled correctly
 fish-symlink/nonexistent-file-relative-to-a-symlink correctly converted
 fish-symlink/symlink_file handled correctly
Error output differs for file realpath.in. Diff follows:
--- realpath.tmp.err	2019-01-03 07:36:58.881966614 +0000
+++ realpath.err	2019-01-03 07:36:22.810613146 +0000
@@ -1,2 +1,2 @@
 builtin realpath: /this/better/be/an/invalid/path: No such file or directory
-rmdir: failed to remove '/tmp/tmp.vOyXE8Ylyr': Invalid argument
+builtin realpath: .: No such file or directory
Testing file status.in ... fail
Error output differs for file status.in. Diff follows:
--- status.tmp.err	2019-01-03 07:36:59.059772300 +0000
+++ status.err	2019-01-03 07:36:22.823276164 +0000
@@ -1,5 +1,5 @@
 <W> fish: An error occurred while redirecting file '.'
-open: Invalid argument
+open: Is a directory
 status: Invalid combination of options,
 you cannot do both 'is-interactive' and 'is-login' in the same invocation
 status: Invalid combination of options,
@faho
Copy link
Member

faho commented Jan 3, 2019

we prepend /usr/gnu/bin in $PATH

Huh. How do you prepend that? It's possible that we set our PATH for the tests.

And should we be prepending it, like we do /usr/xpg4/bin if it exists?

Anyway, I had seen that, and I'd instead just drop the grep in favor of string, and the diff in favor of comm.

The rm differences are a bit more annoying. Do the GNU tools behave like we want them? Because otherwise I'd be inclined to just skip these.

@faho faho added this to the fish 3.1.0 milestone Jan 3, 2019
faho added a commit that referenced this issue Jan 3, 2019
These aren't available on OpenIndiana.

`grep -o` is easily changed to `string`, `diff -q` imitated with
`comm` and `test`.

See #5472.
@Mno-hime
Copy link
Author

Mno-hime commented Jan 3, 2019

we prepend /usr/gnu/bin in $PATH

Huh. How do you prepend that? It's possible that we set our PATH for the tests.

We set PATH in the build environment: https://github.com/OpenIndiana/oi-userland/blob/oi/hipster/make-rules/shared-macros.mk#L25. It defaults to PATH.illumos but is changed to PATH.gnu, where needed.

And should we be prepending it, like we do /usr/xpg4/bin if it exists?

That should work for OI, may not for other illumos distributions. Safe should be to let the user define the environment (here PATH) and propagate it verbatim, where possible.

Other possibility is to test for features and then choosing the right binary, or approach to the binary.

Anyway, I had seen that, and I'd instead just drop the grep in favor of string, and the diff in favor of comm.

Thanks. And sorry for our tools not being up-to-date with what one expects.

The rm differences are a bit more annoying. Do the GNU tools behave like we want them? Because otherwise I'd be inclined to just skip these.

"GNU tools" (coreutils, GAWK, tar, ...) behave as expected, when they are available (and they are available by default on OI), they are available both by their expected name from /usr/gnu/bin and by prefixing them with g and sticking to the default PATH: GNU tar becomes gtar.

@faho
Copy link
Member

faho commented Jan 3, 2019

It defaults to PATH.illumos but is changed to PATH.gnu, where needed.

Ah, okay. So... I, personally, would heavily favor the GNU tools, but then I actually prefer having lots of features over clean minimalism. But I'm not an OpenIndiana user. And given that we can't rely on the GNU tools everywhere anyway, as long as the non-GNU tools are reasonably standards-compliant, we should just deal with it.

The problem here is that your rm has an additional safety measure that stops us from doing the thing we want, which ordinarily would be a very stupid thing to do. But tests sometimes need to test stupid things.

Can you check if

sh -c "cd ..; rm $PWD"

works?

That should be a usable workaround that we can just plonk in here. I don't want to call uname if we can help it, and we can't "test" rm. Another possibility would be to use grm if it exists.


The final test tests a redirection - > . leads to an error, because . of course is a directory. I don't know how to check for either of these because we can't redirect the error itself, so the test harness will always complain about this.

We could again skip this by checking uname, or we could change the error (we pass along what we get from the OS here)...

@Mno-hime
Copy link
Author

Mno-hime commented Jan 3, 2019

Can you check if

sh -c "cd ..; rm $PWD"

works?

Well, I guess it depends on what it means "to work"... You expect the rm to remove directory or I am missing something?

{global} newman@lenovo:~/Downloads/tmp1/tmp2 $ sh -c "cd ..; rm $PWD"
rm: /export/home/newman/Downloads/tmp1/tmp2 is a directory
{global} newman@lenovo:~/Downloads/tmp1/tmp2 $ echo $?
2

{global} newman@lenovo:~/Downloads/tmp1/tmp2 $ sh -c "cd ..; grm $PWD"
grm: cannot remove '/export/home/newman/Downloads/tmp1/tmp2': Is a directory
{global} newman@lenovo:~/Downloads/tmp1/tmp2 $ echo $?
1

I did not understand the case with redirection.

@faho
Copy link
Member

faho commented Jan 3, 2019

You expect the rm to remove directory or I am missing something?

Basically, yes. We expect it to remove $PWD, to test what fish does when $PWD is removed.

(Well, actually that's just one bit - the cd completion tests just want to clean up, so that's not all that important. Adding a commit for that now.)

{global} newman@lenovo:~/Downloads/tmp1/tmp2 $ sh -c "cd ..; rm $PWD"

Sorry, rm -r or rmdir. We need to remove a directory.

I did not understand the case with redirection.

It's just a case of OpenIndiana having a different error message than "everything" else. This is calling somecommand > .. Which makes no sense, because "." is a directory. This is supposed to error out, and on other systems it returns EISDIR, which has the message "Is a directory", but OI returns EINVAL, which has the message "Invalid argument". We simply expected a different error string.

It's just a bit tricky to get the tests to be okay with either.

faho added a commit that referenced this issue Jan 3, 2019
Otherwise this'd run afoul of OpenIndiana's "no removing $PWD" rule. Spoilsports!

See #5472.
@Mno-hime
Copy link
Author

Mno-hime commented Jan 4, 2019

{global} newman@lenovo:~/Downloads/tmp1/tmp2 $ sh -c "cd ..; rm $PWD"

Sorry, rm -r or rmdir. We need to remove a directory.

Right, that confused me.

In fish I see what I expected:

newman@lenovo ~/D/t/tmp2> sh -c "cd ..; rmdir $PWD"; echo $status                      
0
newman@lenovo ~/D/t/tmp2> sh -c "cd ..; grmdir $PWD"; echo $status
0

And exactly the same with rm -r/grm -r.

Now I understand your case with redirection. I see the same thing.

@faho
Copy link
Member

faho commented Jan 18, 2019

For those playing along at home, the remaining issues here are:

  • realpath.in, which uses rmdir to delete $PWD, which OI protects against - so we have to go around those protections, most likely via sh -c "cd ..; rmdir $PWD", which would disconnect the rmdir from the cwd.

  • status.in, which attempts a redirection from a directory, and simply gets a different (more informational!) error message. We either skip that test or change the error - though that requires re-testing what the actual error is. Unless we simply declare getting an EISDIR from open means it's EINVAL. Which, I suppose, it is? But other systems may also sometimes return EISDIR. Or we test this differently, with an invocation test, so we can then intercept the error just for the test?

faho added a commit to faho/fish-shell that referenced this issue Jan 18, 2019
This tested fish-shell#1728, where redirecting a directory (`begin; something;
end < .`) would cause `status` to misbehave.

Unfortunately, on Illumos/OpenIndiana/SunOS, this returns a different
error (EINVAL instead of EISDIR), so we can't check that with our test harness, because
we can't redirect it.

Since it's not important that this gives the same error across
systems (and indeed we provide no way of intercepting the error!),
use an invocation test instead, because that allows different output per-uname.

See fish-shell#5472.
faho added a commit to faho/fish-shell that referenced this issue Jan 18, 2019
Illumos/OpenIndiana/SunOS/Solaris has an rm/rmdir that tries to
protect the user by not allowing them to delete $PWD.

Normally, this would be a good thing as deleting $PWD is a stupid
thing to do. Except in this case, we absolutely need to do that.

So instead we weasel around it by invoking an sh to cd out of the
directory to then invoke an `rmdir` to delete it. That should throw
off any attempts at protection (we could also have tried $PWD/. or
similar, but that's possibly still protected against).

This is the last failing test on
Illumos/OpenIndiana/SunOS/Solaris/afunnyquip, so:

Fixes fish-shell#5472.
faho added a commit to faho/fish-shell that referenced this issue Jan 19, 2019
This tested fish-shell#1728, where redirecting a directory (`begin; something;
end < .`) would cause `status` to misbehave.

Unfortunately, on Illumos/OpenIndiana/SunOS, this returns a different
error (EINVAL instead of EISDIR), so we can't check that with our test harness, because
we can't redirect it.

Since it's not important that this gives the same error across
systems (and indeed we provide no way of intercepting the error!),
use an invocation test instead, because that allows different output per-uname.

See fish-shell#5472.
faho added a commit to faho/fish-shell that referenced this issue Jan 19, 2019
Illumos/OpenIndiana/SunOS/Solaris has an rm/rmdir that tries to
protect the user by not allowing them to delete $PWD.

Normally, this would be a good thing as deleting $PWD is a stupid
thing to do. Except in this case, we absolutely need to do that.

So instead we weasel around it by invoking an sh to cd out of the
directory to then invoke an `rmdir` to delete it. That should throw
off any attempts at protection (we could also have tried $PWD/. or
similar, but that's possibly still protected against).

This is the last failing test on
Illumos/OpenIndiana/SunOS/Solaris/afunnyquip, so:

Fixes fish-shell#5472.
ridiculousfish pushed a commit that referenced this issue Jan 22, 2019
Otherwise this'd run afoul of OpenIndiana's "no removing $PWD" rule. Spoilsports!

See #5472.
faho added a commit that referenced this issue Feb 13, 2019
This tested #1728, where redirecting a directory (`begin; something;
end < .`) would cause `status` to misbehave.

Unfortunately, on Illumos/OpenIndiana/SunOS, this returns a different
error (EINVAL instead of EISDIR), so we can't check that with our test harness, because
we can't redirect it.

Since it's not important that this gives the same error across
systems (and indeed we provide no way of intercepting the error!),
use an invocation test instead, because that allows different output per-uname.

See #5472.
@faho faho closed this as completed in 1ee57e9 Feb 13, 2019
faho added a commit that referenced this issue Feb 13, 2019
It turns out the default gettext on the sunny operating system with
the many names interprets at least `\n` itself, so we'd end up
swallowing it.

This allows us to move past the interactive tests and onto the expect
ones.

See #5472.
@Mno-hime
Copy link
Author

Confirmed it works. Thanks!

faho added a commit that referenced this issue Jan 30, 2020
Solaris/OpenIndiana/Illumos `rm` checks that and errors out.

In these cases we don't actually need it to be a part of $PWD as
it's just for cleanup, so we `cd` out before.

See #5472
See 1ee57e9
Fixes #6555
Fixes #6558
faho added a commit that referenced this issue Jan 30, 2020
Solaris/OpenIndiana/Illumos `rm` checks that and errors out.

In these cases we don't actually need it to be a part of $PWD as
it's just for cleanup, so we `cd` out before.

See #5472
See 1ee57e9
Fixes #6555
Fixes #6558

(cherry picked from commit 9cbd3d5)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants