interp: fix a PATH regression on Windows, fix tests on Wine #769
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A recent change added the use of "tr" to improve test portability, as some commands like "wc" print extra whitespace on Mac. Some versions of Windows, such as Wine, don't ship with "tr". Just like we do with other commands like "wc" or "sed", implement a very basic one as a builtin.
A recent commit, 1eb295c, introduced a regression on Windows. Programs spawned by interp no longer found other programs in PATH. The reason turns out to be because those programs saw a Unix-like PATH, with the list separator being ":" rather than ";", which breaks Windows programs as they see one single mangled directory. The actual source of the bug is much older, from eb75bb8f7d: For better compatibility with non-unix systems, convert $PATH to a unix path list when starting the interpreter. This should work with Windows, for example. The recent commit surfaced the bug by tweaking the environment logic. Before, converting PATH's list separators was an internal detail, but now we also export this change and it affects spawned programs. The added test case fails without the fix: --- FAIL: TestRunnerRun/#883 (0.19s) interp_test.go:3201: wrong output in "GOSH_CMD=lookpath $GOSH_PROG": want: "cmd found\n" got: "exec: \"cmd\": executable file not found in %PATH%\nexit status 1" The fix is relatively simple: stop converting PATH's list separators. I believe the reason I originally added that code was for compatibility, so that scripts using Unix-like path list separator, such as export PATH="${PATH}:${PWD}" would work as expected on Windows. However, I struggle to agree with my past self on that approach. We had no tests for this behavior, and any script using Unix-like paths in any non-trivial way would likely need to be adjusted to be compatible with Windows anyway. Fixes #768.
riacataquian
approved these changes
Dec 2, 2021
|
Oops sorry for the inconvenience !
Yes, handling PATH specifically breaks consistency with other PATH like variables |
cclerget
approved these changes
Dec 2, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(see commit messages - please do not squash)
Fixes #768.