Fix test suite on Cygwin #757

m0vie · 2020-08-09T20:14:23Z

'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 · 2020-08-10T02:10:37Z

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

highlighters/main/test-data/parameter-to-global-alias.zsh

highlighters/main/test-data/precommand-killing1.zsh

danielshahaf · 2020-08-10T05:03:06Z

Makefile

@@ -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##*/}"; \


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 && \

?

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.

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?

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?

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.

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 · 2020-08-10T19:56:03Z

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

I am not aware of any.

danielshahaf · 2020-08-11T08:16:12Z

highlighters/main/test-data/precommand-killing1.zsh

+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


Cherry-picked to master: 79b6e7e.

…x' happens to be a valid external command name. Reported on issue #757 along with other issues.

danielshahaf · 2020-08-11T09:25:48Z

highlighters/main/test-data/parameter-to-global-alias.zsh

+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


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.

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.

danielshahaf · 2020-08-14T06:08:55Z

@m0vie Ping?

m0vie · 2020-08-14T20:16:52Z

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

What else is needed?

danielshahaf · 2020-08-14T22:53:27Z

Address https://github.com/zsh-users/zsh-syntax-highlighting/pull/757/files#r467688965.

m0vie · 2020-08-14T23:36:50Z

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 · 2020-08-14T23:55:03Z

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.

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

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

Why are you focussing on shebangs? Those are trivial to solve: just prepend «$ZSH» to the call (or skip the call entirely — tap-filter and tap-colorizer should both be optional). The real problem is external programs such as cat(1), rm(1), sed(1). Are those being found (on current master)? If so, how? If not, how did the test suite ever work on Cygwin?

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.

It's not our place to hardcode every platform's default $PATH, but perhaps we could derive it? Something along the lines of «PATH=`ZDOTDIR=$(mktemp -d) PATH= $(which zsh) -c 'print -r -- ${(q-)PATH}'`» might do the trick.

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.

The problem statement given was "'env -i zsh' doesn't find zsh". To fix that by passing $PATH through to all tests is to kill a fly with a cannon.

m0vie · 2020-08-15T00:20:56Z

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 · 2020-08-15T06:58:55Z

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 · 2020-08-15T11:38:13Z

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

env -i removes PATH from the environment
When invoking, in /etc/zshenv PATH at least gets set to some sane default again
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 · 2020-08-16T08:02:54Z

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.

m0vie force-pushed the p_cygwin branch from aba2f09 to 458bdf2 Compare August 9, 2020 20:15

danielshahaf reviewed Aug 10, 2020

View reviewed changes

highlighters/main/test-data/parameter-to-global-alias.zsh Outdated Show resolved Hide resolved

danielshahaf reviewed Aug 10, 2020

View reviewed changes

highlighters/main/test-data/precommand-killing1.zsh Outdated Show resolved Hide resolved

danielshahaf reviewed Aug 10, 2020

View reviewed changes

m0vie force-pushed the p_cygwin branch from 458bdf2 to 326afba Compare August 10, 2020 19:55

danielshahaf reviewed Aug 11, 2020

View reviewed changes

danielshahaf added a commit that referenced this pull request Aug 11, 2020

tests: parameter-to-global-alias: Fix a false positive failure when '…

6d5372a

…x' happens to be a valid external command name. Reported on issue #757 along with other issues.

danielshahaf reviewed Aug 11, 2020

View reviewed changes

m0vie force-pushed the p_cygwin branch from 326afba to 5268a95 Compare August 11, 2020 20:15

m0vie force-pushed the p_cygwin branch from 5268a95 to 3417298 Compare August 15, 2020 00:20

m0vie force-pushed the p_cygwin branch from 3417298 to d91164a Compare August 15, 2020 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test suite on Cygwin #757

Fix test suite on Cygwin #757

m0vie commented Aug 9, 2020

danielshahaf commented Aug 10, 2020

danielshahaf Aug 10, 2020

m0vie Aug 10, 2020

danielshahaf Aug 11, 2020

danielshahaf Aug 11, 2020 •

edited

m0vie Aug 11, 2020

danielshahaf Aug 11, 2020

m0vie commented Aug 10, 2020

danielshahaf Aug 11, 2020

danielshahaf Aug 11, 2020

m0vie Aug 11, 2020 •

edited

m0vie Aug 11, 2020

danielshahaf commented Aug 14, 2020

m0vie commented Aug 14, 2020

danielshahaf commented Aug 14, 2020

m0vie commented Aug 14, 2020

danielshahaf commented Aug 14, 2020 via email

m0vie commented Aug 15, 2020

danielshahaf commented Aug 15, 2020

m0vie commented Aug 15, 2020

danielshahaf commented Aug 16, 2020

		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

Fix test suite on Cygwin #757

Are you sure you want to change the base?

Fix test suite on Cygwin #757

Conversation

m0vie commented Aug 9, 2020

danielshahaf commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielshahaf Aug 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m0vie commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m0vie Aug 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielshahaf commented Aug 14, 2020

m0vie commented Aug 14, 2020

danielshahaf commented Aug 14, 2020

m0vie commented Aug 14, 2020

danielshahaf commented Aug 14, 2020 via email

m0vie commented Aug 15, 2020

danielshahaf commented Aug 15, 2020

m0vie commented Aug 15, 2020

danielshahaf commented Aug 16, 2020

danielshahaf Aug 11, 2020 •

edited

m0vie Aug 11, 2020 •

edited