os/exec: windows LookPath is broken #6224
Labels
Milestone
Comments
nope, it's not fixed: c:\>go.exe version go version devel +037a28ab0725 Thu Aug 22 12:13:54 2013 +0900 windows/amd64 c:\>go.exe run -x hello.go WORK=C:\Temp\go-build062400227 mkdir -p $WORK\command-line-arguments\_obj\ mkdir -p $WORK\command-line-arguments\_obj\exe\ cd c:\ "s:\\workspace.go.2013\\go64\\pkg\\tool\\windows_amd64\\6g.exe" -o "C:\\Temp\\go-build062400227\\co mmand-line-arguments\\_obj\\_go_.6" -p command-line-arguments -complete -D _/c_ -I "C:\\Temp\\go-bu ild062400227" "c:\\hello.go" go build command-line-arguments: exec: "s:\\workspace.go.2013\\go64\\pkg\\tool\\windows_amd64\\6g.e xe": file does not exist |
The problem is PATHEXT, not absolute filename. This is how Windows works. C:\>find FIND: Parameter format not correct C:\>set PATHEXT=.BAT C:\>find 'find' is not recognized as an internal or external command, operable program or batch file. C:\> Go is doing the same. Please, feel free to argue this point. Alex Status changed to WorkingAsIntended. |
(excuse my $d $t $p$g prompt) Ok, but if I'm using your example it should be like this: Fri 08/23/2013 10:25:16.88 C:\Users\user>find.exe FIND: Parameter format not correct Fri 08/23/2013 10:25:18.98 C:\Users\user>set PATHEXT=.BAT Fri 08/23/2013 10:25:24.37 C:\Users\user>find.exe FIND: Parameter format not correct Fri 08/23/2013 10:26:56.61 C:\Users\user>c:\windows\system32\find 'c:\windows\system32\find' is not recognized as an internal or external command, operable program or batch file. Fri 08/23/2013 10:27:03.66 C:\Users\user>c:\windows\system32\find.exe FIND: Parameter format not correct Because Go is using this filename: "s:\\workspace.go.2013\\go64\\pkg\\tool\\windows_amd64\\6g.exe" which is equivalent to c:\windows\system32\find.exe above, if it didn't have .exe already, then it would make sense to not find it(just as above) Fri 08/23/2013 10:34:04.41 C:\Users\user>set PATHEXT=.EXE Fri 08/23/2013 10:34:27.72 C:\Users\user>c:\windows\system32\find FIND: Parameter format not correct Fri 08/23/2013 10:34:30.75 C:\Users\user>set PATHEXT=.BAT Fri 08/23/2013 10:34:56.68 C:\Users\user>c:\windows\system32\find 'c:\windows\system32\find' is not recognized as an internal or external command, operable program or batch file. (this makes sense, to me) The thing is, currently, even if the file name is absolute (path+name+extension) LookPath is still using PATHEXT to validate the extension, so if it's not ending in .bat (if PATHEXT=.BAT) then it says it doesn't find it, but I say that if the file exists (possibly regardless of the extension?) it should be found... Maybe the extension should be limited(to ie. exe, com, dll, whichever), but in windows command prompt if the file is absolute it will be executed as if double clicked, so something like file.jpg would open default program for .jpg files: Fri 08/23/2013 10:27:08.94 C:\Users\user>"Documents\!screenshots\applications ffox.png" Fri 08/23/2013 10:31:14.73 C:\Users\user>"Documents\!screenshots\applications ffox" '"Documents\!screenshots\applications ffox"' is not recognized as an internal or external command, operable program or batch file. (the first one just worked, opened file in default viewer) Maybe Go doesn't want to have this behaviour though, if it tries to execute the file in some other way (as if it were an .exe) ... but still the point is that if the file is absolute/exists it gets "executed"(think: as if double clicked) on Windows |
also, in case anyone was wondering at this point, this works too (it opens the .png file, even though its extension wasn't specified, just like it does for .exe files for example) Fri 08/23/2013 10:36:51.06 C:\Users\user>set PATHEXT=.PNG Fri 08/23/2013 10:38:47.79 C:\Users\user>"Documents\!screenshots\applications ffox" Fri 08/23/2013 10:38:52.32 C:\Users\user> but PATHEXT shouldn't interfere with absolute filenames, or well, rather when the filename already has its extension specified (just like above when I used just find.exe [extension was specified]) So in other words, as I am just realizing this as I write, if the filename already has the extension specified, it should be searched in path, or if it already contains a path ie. c:\windows\system32\find.exe then it shouldn't be searched in path but since it does contain .exe it should be stat-ed to see if it exists, regardless of PATHEXT's value |
That's a great idea, I kinda wanted to do that, thank you for this opportunity. http://play.golang.org/p/g4MD29_6JT or pasting it here just in case: package main import "os" import "os/exec" func main() { testEXE() testBAT() } func setup(extensions string) { setEnv("PATHEXT", extensions) setEnv("PATH", "C:\\windows\\system32") } func findWhere() { lookPath("where") lookPath("where.exe") lookPath("c:\\windows\\system32\\where.exe") } func testEXE() { //all work setup(".EXE") findWhere() } func testBAT() { //all fail setup(".BAT") findWhere() } func lookPath(file string) { if ffile, err := exec.LookPath(file); err != nil { println("Not found:", err.Error()) } else { println(" Found:", ffile) } } func setEnv(key, value string) { os.Setenv(key, value) if now_val := os.Getenv(key); value != now_val { println("Couldn't set `", key, "` env var, current value `", now_val, "`, wanted value `", value, "`") } } |
output:
Found: C:\windows\system32\where.exe
Found: C:\windows\system32\where.exe
Found: c:\windows\system32\where.exe
Not found: exec: "where": executable file not found in %PATH%
Not found: exec: "where.exe": executable file not found in %PATH%
Not found: exec: "c:\\windows\\system32\\where.exe": file does not exist
|
atkaaz, You convinced me there is a problem here. I think the problem is the way we interpret one of the rules in http://technet.microsoft.com/en-us/library/cc723564.aspx, in particular, it says: "... If the command name includes a file extension, the shell searches each directory for the exact file name specified by the command name. ...". We only consider etxtensions listed in PATHEXT https://code.google.com/p/go/source/browse/src/pkg/os/exec/lp_windows.go#32, but, I think, we need to allow for ANY extension. Also, it continues to say: "... If the command name does not include a file extension, the shell adds the extensions listed in the PATHEXT environment variable, one by one, and searches the directory for that file name.... " but we go and try extensions in PATHEXT https://code.google.com/p/go/source/browse/src/pkg/os/exec/lp_windows.go#37 regardles of whether we have extension in the name already. I think the fix should be easy enough to make, but, given that we made mistake already, I think we should write new test that will verify some of these rules. Perhaps something similar to TestWinSplitListTestsAreValid from path/filepath package. Would you like to have a go and make this change? If not, I will do it myself. Just le me know. Thank you. Alex Labels changed: added go1.2, packagebug, removed priority-triage. Owner changed to @alexbrainman. Status changed to Accepted. |
You put it so eloquently. I'd love to take a stab at it, but considering that I am new to Go and I've no idea(currently) how to use the CL stuff, it might be more work for people reviewing the code(that I'll write) than writing it yourself(?) but if time is not an issue (ie. 1-2 days max) then I'll proceed reading http://golang.org/doc/contribute.html (I'll still read this anyway) and submit this change[exactly as you described it], I just hope it won't be more work for the people reviewing the code having to suggest changes to it than actually having someone write it who already knows how to write it. |
I'll do it. Codereview seems to work after doing this https://code.google.com/p/go-wiki/wiki/CodeReview , unless you're referring to something else, I'll see. I can also try it on linux (virtualbox) if I really have to. Thanks. |
since 48 hours passed and I'm still yet not done, I felt like I should report that I am still currently working on it, not stuck it's just that I'm trying to reorganize the code while at the same time attempting to cover all/most test cases for this lookpath and haven't yet implemented it but i think i can do it, just need few more hours |
what I have so far is currently a mess(and this is only the test part): https://github.com/DeMLinkS/demlinks/blob/master/poc/lookpathtest/lp_windows_test.go but I'm expecting it to be clearer soon, I am also learning concepts as I go along... (sleep mode now,laterz) |
This is how I would approach this:
diff --git a/src/pkg/os/exec/lp_windows_test.go b/src/pkg/os/exec/lp_windows_test.go
new file mode 100644
--- /dev/null
+++ b/src/pkg/os/exec/lp_windows_test.go
@@ -0,0 +1,45 @@
+// Copyright 2013 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package exec
+
+import (
+ "testing"
+)
+
+type lookPathTest struct {
+ PATH string
+ PATHEXT string
+ files []string
+ searchFor string
+ // TODO: maybe try and work out resultExpected on the fly
+ // by executing some system command.
+ resultExpected string
+}
+
+var lookPathTests = []lookPathTest{
+ {
+ PATHEXT: `.COM;.EXE;.BAT`,
+ PATH: `p2;p1`,
+ files: []string{`p1\a.exe`, `p2\a.exe`},
+ searchFor: `a`,
+ resultExpected: `p2\a.exe`,
+ },
+}
+
+func testLookPath(t *testing.T, lpt lookPathTest) {
+ // Create tmp directory.
+ // Update lpt.PATH, lpt.files and lpt.resultExpected to include our tmp directory.
+ // Create files listed in lpt.files in tmp directory.
+ // Create a Go program that uses LookPath to find lpt.searchFor and print result to
stdout (maybe pass lpt.searchFor as process parameter).
+ // Build the Go program.
+ // Run the Go program with lpt.PATH and lpt.PATHEXT set.
+ // Capture output of execution and compare it with lpt.resultExpected.
+}
+
+func TestLookPath(t *testing.T) {
+ for _, lpt := range lookPathTests {
+ testLookPath(t, lpt)
+ }
+}
But, I could be wrong.
Alex
|
I think maybe you should do it..., because I will fail at keeping it as simple as such, so it will look complicated at least. I just can't help but add lots of crap which makes it complicated... I'd definitely not go the "create Go program" way, it seems overkill vs just using LookPath directly from within the test, also files don't have to be .exe . I think I will continue working on it anyway see where it goes, but don't let this stop you from implementing it your way... I mean, I don't mind at all because I've learned a lot by trying to do this so far, and I don't mind if you chose not to use or wait for my variant. |
https://golang.org/cl/13410045/ Status changed to Started. |
|
This issue was closed by revision a6149da. Status changed to Fixed. |
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
by atkaaz:
The text was updated successfully, but these errors were encountered: