Comment 1:

Labels changed: added priority-later, go1.1, removed priority-triage.

Status changed to Accepted.

Comment 2:

This is ridiculous (Windows's fault, not yours).
I guess we should call filepath.SplitPath to get the unquoted form.
Russ

Comment 3:

I am not convinced that there is an issue here. None of my environment variables have
quotes around them, some of them are paths with space in it.
Putting quotes around GOROOT environment variable is not needed - it achieves nothing. I
think it is a user mistake. I would keep things simple and do nothing.
Alternatively, if we want to be nice, and try and support this case, then, I think, the
change should be done in runtime.GOROOT, not in cmd/go. Just to be consistent. But, I am
concerned to introduce "unquoted" runtime.GOROOT that will not equal to
os.Getenv("GOROOT"). Also what are the rules of GOPATH "unquoting"? Should we follow
filepath.SplitPath? But it is not documented anywhere.
Just makes everything complicated.
Perhaps, we could just os.Stat(runtime.GOROOT) at cmd/go start, and fail with
appropriate message.
Alex

Comment 4 by jgodfrey@thelostsite.co.uk:

If you have a path with a '&' character in it you're essentially required to put ""
around it or it won't get passed at all (in BAT). 
I actually found another related bug while researching this one. If you put whitespace
after the GOROOT path with or without quotes it will blindly put that into GOTOOLDIR
e.g. 
REM The below line has an extra space at the end   
set GOTOOLDIR=C:\go 
Then if we run "go env":   
set GOTOOLDIR=C:\go \pkg\tool\windows_386
So seems like the problem is "larger" than just quotation marks. We aren't even trimming
GOROOT.   
Now I would accept that ALL of these are the user's fault. But the way BAT is designed
it makes these kind of mistakes fairly easy to do and fairly hard to detect once they're
done.    
I mean even if we ignore the quote issue (edge case?) then it is hard to ignore the
whitespace issue.

Comment 5 by jgodfrey@thelostsite.co.uk:

If you have a path with a '&' character in it you're essentially required to put ""
around it or it won't get passed at all (in BAT). 
I actually found another related bug while researching this one. If you put whitespace
after the GOROOT path with or without quotes it will blindly put that into GOTOOLDIR
e.g. 
REM The below line has an extra space at the end   
set GOROOT=C:\go 
Then if we run "go env":   
set GOTOOLDIR=C:\go \pkg\tool\windows_386
So seems like the problem is "larger" than just quotation marks. We aren't even trimming
GOROOT.   
Now I would accept that ALL of these are the user's fault. But the way BAT is designed
it makes these kind of mistakes fairly easy to do and fairly hard to detect once they're
done.   
All users get told is "fmt" and "runetime" packages not found (when building). Instead
of something like "WARNING: GOTOOLDIR not found."   
I mean even if we ignore the quote issue (edge case?) then it is hard to ignore the
whitespace issue.

Comment 6:

jgodfrey,
Your last discovery just proves my point - we should treat all these cases as user
mistake. Otherwise we'll be chasing our tail trying to do what cmd.exe does.
We should just display good error message, if os.Stat(runtime.GOROOT) fails.
Alex

Comment 7 by jgodfrey@thelostsite.co.uk:

While I would really like to see a clearer error message, I am struggling with the
concept that an extra space character can be a "user mistake."    
Also, GOROOT with an extra space is a valid path since the space is stripped out at a
later point. The issue is with the construction of GOTOOLDIR where the space is used
verbatim as in C:\go \pkg\tool\windows_386 and the path will never be valid.    
I guess it depends on how "forgiving" you want go to be. You're essentially arguing that
go should be so unforgiving that it doesn't even understand the quirks of the underlying
OS/environment it runs on.   
So we get into a situation of shifting the responsibility of understanding Windows and
its quirks from the go team onto the end users themselves. It really isn't that rare for
users to set environment variables in BAT or even using the dialog, both of which
support quotes and extra spaces/whitespace.   
Only Powershell is likely not going to introduce these issues...

Comment 8 by visan.ovidiu:

Tested on both windows 64 and 32 bit and the GOTOOLDIR it's correctly contructed if you
run the SET command without any quotes in path.
eg. SET GOROOT=C:\Program Files\go

Comment 9 by eric.hill:

Quotes in Windows were intended for the command interpreter to understand where to break
command line arguments to an application:
app.exe "First Argument" "Second Argument"
Placing them into environment variables is not generally a good idea, including the
system PATH.  Applications that use PATH will split it on a semicolon and then use the
strings as prefixes to the file name they're looking for.  Environment variables with a
single path should be just that, the path:
SOMEPATH=C:\Program Files\Long Path\bin
Inside of a program that reads the variable, that expands to a single string that should
be treated as a path segment.  Command scripts work the same way:
@echo off
"%SOMEPATH%\app.exe" "First Argument" "Second Argument"
I think the parsing or stripping quotes from environment variables may cause unintended
side effects.  For example, an application that computes folder size:
@echo off
set APPPATH=C:\Program Files\Folder Sizer
set APPARGS=folder name
rem Use case 1
"%APPPATH%\app.exe" %APPARGS%
rem Use case 2
"%APPPATH%\app.exe" "%APPARGS%"
Use case 1 is very different from use case 2.  In case 1, the application should compute
the space for two folders, one named "folder", and one named "name".  In the second
case, the application should compute the space for a single folder called "folder name".
 If the application performed quote stripping, it may not work properly for case 2.

Comment 10:

I agree with the many people who have said that we should not interpret quotes here.

Status changed to Unfortunate.

Comment 11 by JGodfrey16:

That's fine. But maybe an error then? You don't even need to do anything with the
quotes, just check if GOROOT exists. That way you catch quotes, whitespace (see above),
and simply invalid paths.    
Nobody has addressed the whitespace bug I posted above. Should I re-submit that one as a
new bug report?

Comment 12 by jgodfrey@thelostsite.co.uk:

That's fine. But maybe an error then? You don't even need to do anything with the
quotes, just check if GOROOT exists. That way you catch quotes, whitespace (see above),
and simply invalid paths.    
Nobody has addressed the whitespace bug I posted above. Should I re-submit that one as a
new bug report?

Comment 13:

I don't know enough about Windows to know what to reject and what to accept. My
inclination is to require users to set environment variables correctly.
Note that the GOROOT environment variable need not be set at all as long as the Go
installation does not move after it is built (or is installed into the same directory
where it was built on another machine). 
So maybe the real fix is to stop telling people to set %GOROOT%.

Comment 14 by jgodfrey@thelostsite.co.uk:

But a single space character after the environmental variable is "correct" from the
perspective of most users. Such a path can even be used by most system APIs to do
things. You just cannot concatenate it into the middle of a longer path with the space
still attached. Ditto with the quotes.   
Checking if the directory exists would be hugely helpful not just on Windows but also on
Linux and OSX. There is never a situation where invalid environmental variables won't
cause go to fail and fail in rather obscure ways.    
Plus you won't even have to handle every single edge case... Just one case - "is this
path valid? Yes|No" and almost every OS has functionality to check that for you.

Comment 15:

I suggest we check GOROOT as in https://golang.org/cl/7786047/.
Alex

Comment 16:

This issue was closed by revision d67e300.

Status changed to Fixed.