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

Fix test suite on Cygwin #757

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix test suite on Cygwin #757

wants to merge 1 commit into from

Conversation

m0vie
Copy link
Contributor

@m0vie m0vie commented Aug 9, 2020

'make test' fails under Cygwin:

$ make test
ZSH_PATCHLEVEL=zsh-5.8-243-g5b4cfb1
Running test brackets
env: ‘zsh’: No such file or directory
Running test main
env: ‘zsh’: No such file or directory
Running test pattern
env: ‘zsh’: No such file or directory
Running test regexp
env: ‘zsh’: No such file or directory
make: *** [Makefile:40: test] Error 127

This is because on Cygwin, env -i zsh does not work as expected (Cygwin does not lookup the command in /usr/bin/ or /usr).

After fixing that, there are two more problems:

## parameter-to-global-alias
# BUFFER=$'$s'
not ok 1 - [1,2] «$s» - expected (1 2 "unknown-token"), observed (1 2 "command").
ok 2 - cardinality check
# expected_region_highlight  region_highlight
# '1 2 unknown-token'        '0 2 command'
## precommand-killing1
# BUFFER=$'sudo -e /etc/passwd'
ok 1 - [1,4] «sudo»
ok 2 - [6,7] «-e»
not ok 3 - [9,19] «/etc/passwd» - expected (9 19 "path"), observed (9 19 "default").
ok 4 - cardinality check
# expected_region_highlight   region_highlight
# '1 4 precommand'            '0 4 precommand'
# '6 7 single-hyphen-option'  '5 7 single-hyphen-option'
# '9 19 path'                 '8 19 default'

@danielshahaf
Copy link
Member

Thanks. Haven't reviewed yet, but question: Is there a Cygwin CI service we could use to catch such problems sooner?

Makefile Outdated
@@ -41,7 +41,7 @@ test:
for test in highlighters/*; do \
if [ -d $$test/test-data ]; then \
echo "Running test $${test##*/}"; \
env -i QUIET=$$QUIET $${TERM:+"TERM=$$TERM"} $(ZSH) -f tests/test-highlighting.zsh "$${test##*/}"; \
env -i QUIET=$$QUIET $${TERM:+"TERM=$$TERM"} PATH="$$PATH" $(ZSH) -f tests/test-highlighting.zsh "$${test##*/}"; \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would leave $PATH set for the entirety of the test run, though. How about:

--- a/Makefile
+++ b/Makefile
@@ -4,7 +4,7 @@ INSTALL?=install -c
 PREFIX?=/usr/local
 SHARE_DIR?=$(DESTDIR)$(PREFIX)/share/$(NAME)
 DOC_DIR?=$(DESTDIR)$(PREFIX)/share/doc/$(NAME)
-ZSH?=zsh # zsh binary to run tests with
+ZSH?="`which zsh`" # zsh binary to run tests with
 
 all:
        cd docs && \

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doens't work:

$ make test
ZSH_PATCHLEVEL=zsh-5.8-243-g5b4cfb1
Running test brackets
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
Running test main
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
Running test pattern
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
Running test regexp
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
/usr/bin/env: 'zsh': No such file or directory
make: *** [Makefile:40: test] Error 2

Reason: The shebang of test-highlighting.zsh contains #!/usr/bin/env zsh, i.e. another env call, which then again won't find zsh anymore if PATH is not set.

I guess we are better of with having PATH set. Potential external commands inside test-cases itself also need to be found.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shebang line? Really? I'd have expected it not to matter when -c isn't given.

How about $(ZSH) -fc '. tests/test-highlighting.zsh to bypass the shebang line? Would that break anything?

Copy link
Member

@danielshahaf danielshahaf Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shebang line? Really? I'd have expected it not to matter when -c isn't given.

On Linux and on FreeBSD it doesn't matter. What's the output of zsh -f =(<<<$'#!/usr/bin/env date') on Cygwin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction: It is not the shebang of test-highlighting.zsh but the other external scripts at are being run from there. E.g.

print > >($results_filter | $root/tests/tap-colorizer.zsh) -r -- "# global (driver) tests"
print > >($results_filter | $root/tests/tap-colorizer.zsh) -r -- "1..1"

What this actually means is that if ZSH is set in the Makefile, this does not guarantee that every part of the test suite is executed with that exact ZSH binary. Those called scripts might not be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tap-colorizer needn't be run under the same zsh version as everything else. The only reason it's written in zsh is to minimize dependencies.

Anyway, feel free to PR the fix.

@m0vie
Copy link
Contributor Author

m0vie commented Aug 10, 2020

Is there a Cygwin CI service we could use to catch such problems sooner?

I am not aware of any.

Comment on lines 32 to 39
touch foo

BUFFER='sudo -e /etc/passwd'
BUFFER='sudo -e ./foo'

expected_region_highlight=(
'1 4 precommand' # sudo
'6 7 single-hyphen-option' # -e
'9 19 path' # /etc/passwd
'9 13 path' # ./foo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cherry-picked to master: 79b6e7e.

danielshahaf added a commit that referenced this pull request Aug 11, 2020
…x' happens to be a valid external command name.

Reported on issue #757 along with other issues.
Comment on lines 31 to 32
alias -g global_alias_which_hopefully_does_not_clash_with_an_external_command_name=y
local s=global_alias_which_hopefully_does_not_clash_with_an_external_command_name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deployed a partial fix in 6d5372a. If you'd like to improve things further, you're very welcome to do so, but do so in a new issue or pull request — do not overload this one.

Copy link
Contributor Author

@m0vie m0vie Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the solution of 6d5372a. This just skips the test now for no good reason.

Avoiding the name of the alias to be a command could at least be done by also choosing something other than x which is not as likely to clash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielshahaf
Copy link
Member

@m0vie Ping?

@m0vie
Copy link
Contributor Author

m0vie commented Aug 14, 2020

I have already pushed an updated commit (improved commit message) to this PR.

What else is needed?

@danielshahaf
Copy link
Member

@m0vie
Copy link
Contributor Author

m0vie commented Aug 14, 2020

There is nothing really to address.

Keeping PATH completely unset is not an option on Cygwin, because the default fallback of /bin and /usr/bin won't be searched, like e.g. on Linux.

That means that every (external) utility that is being invoked with a shebang in the test code will fail.

The only thing I can think of is something along the line of

env -i ... PATH=/usr/bin:/bin ...

i.e, to force the fallback behavior of most platforms explicitly.

Then again, I see no harm in simply passing PATH along unchanged. You'd have to have a really weird development environment for that to break anything, wouldn't you? Travis seems to agree.

@danielshahaf
Copy link
Member

danielshahaf commented Aug 14, 2020 via email

@m0vie
Copy link
Contributor Author

m0vie commented Aug 15, 2020

Are you sure about this? What about, say, env -i /bin/bash and therein (interactively) running env? What does that print?

zsh:~$ env -i /bin/bash
bash:~$ env
LS_COLORS=
BLOCK_SIZE='1
DISPLAY=:0.0
SYSTEMROOT=C:\WINDOWS
PWD=/home/mv
DF_BLOCK_SIZE='M
WINDIR=C:\WINDOWS
SHLVL=1
EXECIGNORE=*.dll
_=/usr/bin/env
bash:~$ env bash
env: 'bash': No such file or directory

The real problem is external programs such as cat(1), rm(1), sed(1). Are those being found (on current master)? If so, how?

They are. The problem is only with 'env' itself behaving very strangely (hence the focus on the shebang of the scripts containing it). Sorry, if that was not clear from the beginning, I didn't fully understand it myself up to now.

Those are trivial to solve: just prepend «$ZSH» to the call.

You are right, that does work. (Just need to pass ZSH via env -i ... ZSH=${ZSH} ... in the Makefile).

I have pushed the branch again. Tests rune fine on Cygwin using that approach. What do you think?

@danielshahaf
Copy link
Member

They are. The problem is only with 'env' itself behaving very strangely (hence the focus on the shebang of the scripts containing it). Sorry, if that was not clear from the beginning, I didn't fully understand it myself up to now.

Wait, so you're saying that within env -i zsh, $PATH is unset, and plain sed works but literally env sed fails? Why does the former work? What's type -w sed at that point? Might the difference be a Cygwin bug?

'env -i' clears the complete environment, including PATH. In that
case, on most platforms, when excuting commands without PATH being
set, /usr/bin and /bin are searched, e.g. on Linux:

  $ strace env -i asdf |&  grep asdf
  execve("/usr/bin/env", ["env", "-i", "asdf"], 0x7ffc3e3c0890 /* 27 vars */) = 0
  execve("/bin/asdf", ["asdf"], 0x55be2da090d0 /* 0 vars */) = -1 ENOENT (No such file or directory)
  execve("/usr/bin/asdf", ["asdf"], 0x55be2da090d0 /* 0 vars */) = -1 ENOENT (No such file or directory)
  write(2, "\342\200\230asdf\342\200\231", 10‘asdf’) = 10

Howver, this does not hold on every platform. E.g. on Cygwin, so
such fallback paths are searched:

  $ strace env -i asdf |&  grep asdf
     37   25736 [main] env 3516 build_argv: argv[2] = 'asdf'
    643   30373 [main] env 3516 find_exec: find_exec (asdf)
     35   30408 [main] env 3516 find_exec: (null) = find_exec (asdf)
     36   30444 [main] env 3516 spawnve: spawnve (, asdf, 0x10040B000)
  ‘asdf’  199   53601 [main] env 3516 write: 10 = write(2, 0x10040B040, 10)

  $ env -i zsh
  env: ‘zsh’: No such file or directory

Therefore, we need to make sure that the default PATH is exported
from tests/test-highlighting.zsh.
@m0vie
Copy link
Contributor Author

m0vie commented Aug 15, 2020

Wait, so you're saying that within env -i zsh, $PATH is unset, and plain sed works but literally env sed fails?

Yes, it seems to be due to the fact that

  1. env -i removes PATH from the environment
  2. When invoking, in /etc/zshenv PATH at least gets set to some sane default again
  3. But that sane default is not exported, so while zsh is able to directly invoke commands again, when invoking env again from the shell it is again fully unset and no commands are found

So this has to do with the default lookup fallback after all.

One could argue that this is a bug in Cygwin, indeed. However, this appears to be know for over 17 years:
-https://cygwin.cygwin.narkive.com/aMNM4qT7/env-xargs-no-such-file-or-directory
-https://sourceware.org/legacy-ml/cygwin/2004-09/msg00128.html
-http://mailman.uclinux.org/pipermail/uclinux-dev/2005-May/032483.html

We can work around that by making sure PATH is exported from tests/test-highlighting.zsh for command invocations from there. See updated commit.

@danielshahaf
Copy link
Member

Point (1) isn't a bug; it's a POSIX-compliant behaviour.

Point (3) sounds like it might be a bug, and doesn't seem to be covered by the references you listed. Why don't you raise it with the Cygwin zsh package maintainers?

Could you make the export conditional on running on Cygwin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants