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

interp: Regression on $PATH handling on Windows #768

Closed
andreynering opened this issue Nov 30, 2021 · 6 comments · Fixed by #769
Closed

interp: Regression on $PATH handling on Windows #768

andreynering opened this issue Nov 30, 2021 · 6 comments · Fixed by #769
Milestone

Comments

@andreynering
Copy link
Collaborator

Originally reported at:

Didn't had the time to try it myself yet (sorry), but wanted to report this sooner than later.

Apparently the latest release is messing with how executables see $PATH on Windows. For example, when running go build foo.go Gosh has no problems finding go, but the go command has problems finding other executables like gcc.

@mvdan
Copy link
Owner

mvdan commented Nov 30, 2021

If anyone can bisect to a single commit, or produce a small reproducer, that would certainly help :)

@glumia
Copy link

glumia commented Dec 1, 2021

Hi @mvdan! (and thanks @andreynering for reporting the issue here)

Here is a minimal reproducer:

.
└── main.go

main.go:

package main

import (
	"fmt"
	"os"
)

func main(){
	fmt.Println(os.Getenv("PATH"))
}

Commands (Powershell):

> go install mvdan.cc/sh/v3/cmd/gosh@v3.4.0
> gosh -c 'go run main.go'
C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Windows\System32\OpenSSH\;C:\Program Files\Go\bin;C:\TDM-GCC-64\bin;C:\Program Files\Git\cmd;C:\Users\gius\AppData\Local\Microsoft\WindowsApps;C:\Users\gius\go\bin
> go install mvdan.cc/sh/v3/cmd/gosh@v3.4.1
> gosh -c 'go run main.go'
C:\Windows\system32:C:\Windows:C:\Windows\System32\Wbem:C:\Windows\System32\WindowsPowerShell\v1.0\:C:\Windows\System32\OpenSSH\:C:\Program Files\Go\bin:C:\TDM-GCC-64\bin:C:\Program Files\Git\cmd:C:\Users\gius\AppData\Local\Microsoft\WindowsApps:C:\Users\gius\go\bin

It seems like the issue are those : used as separators instead of ;.


Update: the issue was introduced by commit 1eb295c.

@mvdan
Copy link
Owner

mvdan commented Dec 1, 2021

Thank you both for the detailed report.

In a way, 1eb295c isn't to blame - it simply made the bug surface where it was hidden before.

The source of the bug is very old; see edb75bb. In there, I wrote:

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.

I was kind enough to write this message for my future self, but I'm not sure what I was even thinking with that. I think the point was that scripts using stuff like export PATH="${PATH}:${PWD}" would work as usual?

The reason it's surfacing now is that such a "unix-ified" PATH used to be kept as an internal detail, whereas the recent change fixed a bit of the environment logic that now means we re-export the modified PATH, thus breaking any Windows programs we spawn.

I think I'm just going to undo that path list separator conversion. None of our tests break if I change the code accordingly, and your example should then work again. Apologies for the trouble.

mvdan added a commit that referenced this issue Dec 1, 2021
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.
@glumia
Copy link

glumia commented Dec 2, 2021

Uh, interesting!

Apologies for the trouble.

You don't have to apologize, you don't owe us anything.
Thanks for your work on this project and for looking into the issue in such a short time :)

@mvdan mvdan closed this as completed in #769 Dec 2, 2021
mvdan added a commit that referenced this issue Dec 2, 2021
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.
mvdan added a commit that referenced this issue Dec 2, 2021
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.
@mvdan
Copy link
Owner

mvdan commented Dec 2, 2021

I have cherry-picked the fix into the v3.4 branch: 5ae9d64

I'll do a v3.4.2 release sometime in the next week, to wait and see if any other bugs are spotted. In the meantime, you can use the HEAD of that branch, since master may include v3.5 feature work.

@mvdan mvdan added this to the 3.4.2 milestone Dec 2, 2021
@andreynering
Copy link
Collaborator Author

Thanks @mvdan!

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

Successfully merging a pull request may close this issue.

3 participants