code review 4627064: gotest: clean up GOOS= pollution

gotest: clean up GOOS= pollution

Clean up gotest by eliminating GOOS=windows.

[peterGo, 2011-06-26 05:07:19]

Hello r@golang.org, vcc.163@gmail.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[brainman, 2011-06-26 07:36:12]

I don't see what are you trying to fix? Do you have an issue to demonstrate your
problem.

If you're trying to simplify the code, then I can't see how adding all that code
makes things simpler. Especially in the light that gomake bash script will get
replaced by a binary gomake program soon, and existing

if runtime.GOOS == "windows" && command == "gomake" {

will goo away anyway.

Alex

http://codereview.appspot.com/4627064/diff/5001/src/cmd/gotest/gotest.go
File src/cmd/gotest/gotest.go (right):

http://codereview.appspot.com/4627064/diff/5001/src/cmd/gotest/gotest.go#newc...
src/cmd/gotest/gotest.go:89: runTestWithArgs("." + string(os.PathSeparator) + O
+ ".out")
s/os.PathSeparator/filepath.Separator/

filepath.Separator is used everywhere else, not os.PathSeparator.

[peterGo, 2011-06-26 23:20:35]

Alex,

On 2011/06/26 07:36:12, brainman wrote:
> I don't see what are you trying to fix? Do you have an issue to demonstrate
your
> problem.

Reading the core Go code, it's clear that the the core Go developers have
experience writing portable code. They provided several mechanisms to help make
writing portable Go code easier. They avoided sprinkling Go code with 'if GOOS='
statements, a well-known pitfall. Otherwise, Go code readability and
maintainability would quickly crumble.

If you step back from the gotest code and ask what the real problem is, it's
that Unix-like OSs have a more complex model for executable files than Windows.
Files can be marked as executable and the shell peeks into executable files to
determine how to run them. Therefore, for Windows, portable programs need to be
more specific about what they want to execute. For example, distinguishing
between a binary executable program and a shell executable script.

This CL incorporates a fix for the bug identified by vcc, which prevents gotest
from running a binary executable file on Windows. His original fix added another
two 'if GOOS=' statements.

This CL also replaces all the 'if GOOS=' statements with standard Go mechanisms
for OS portability.

Whatever goinstall does, which is still speculative, this CL is necessary and
useful. The gotest program still needs to execute binary executable files on
Windows.

You should always leave code better than you found it. It would be a shame to
squander Rob's excellent code.
 
Peter

http://codereview.appspot.com/4627064/diff/5001/src/cmd/gotest/gotest.go
File src/cmd/gotest/gotest.go (right):

http://codereview.appspot.com/4627064/diff/5001/src/cmd/gotest/gotest.go#newc...
src/cmd/gotest/gotest.go:89: runTestWithArgs("." + string(os.PathSeparator) + O
+ ".out")
On 2011/06/26 07:36:12, brainman wrote:
> s/os.PathSeparator/filepath.Separator/
> 
> filepath.Separator is used everywhere else, not os.PathSeparator.

Done.

[peterGo, 2011-06-26 23:24:15]

Hello r@golang.org, vcc.163@gmail.com, alex.brainman@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2011-06-26 23:33:58]

On 2011/06/26 23:20:35, peterGo wrote:
> 
> This CL incorporates a fix for the bug identified by vcc, which prevents
gotest
> from running a binary executable file on Windows. ...

Do you have an issue to demonstrate the problem? I'm not aware of any problem.
I'm happy to fix, but I would like to know what we're fixing.

Thank you.

Alex

[Joe Poirier, 2011-06-27 03:01:04]

On Jun 26, 2011, at Jun 26 - 6:20 PM, go.peter.90@gmail.com wrote:
> 
> If you step back from the gotest code and ask what the real problem is,
> it's that Unix-like OSs have a more complex model for executable files
> than Windows. Files can be marked as executable and the shell peeks into
> executable files to determine how to run them. Therefore, for Windows,
> portable programs need to be more specific about what they want to
> execute. For example, distinguishing between a binary executable program
> and a shell executable script.
> 
> 
> Peter
> 

You can't call a script file directly, e.g. within an executable,
under Windows because it doesn't understand the "#!" magic number,
nor does the Windows cmd shell. The workaround on Windows is to call
bash, passing the script file as an argument.    

I'm guessing that when gomake becomes an executable the shell will
become less, or not at all, relevant.

-joe 

   

[vcc, 2011-06-27 04:29:13]

2011/6/27  <alex.brainman@gmail.com>:
> On 2011/06/26 23:20:35, peterGo wrote:
>
>> This CL incorporates a fix for the bug identified by vcc, which
>
> prevents gotest
>>
>> from running a binary executable file on Windows. ...
>
> Do you have an issue to demonstrate the problem? I'm not aware of any
> problem. I'm happy to fix, but I would like to know what we're fixing.

I try to fixes the problem is gotest run fail on my working windows amd64 port.

--
Wei guangjing

[r, 2011-06-27 04:32:55]

with the renaming, LGTM.  i'll leave it to alex.brainman to decide whether it
solves a real problem and, if so, to check it in.

http://codereview.appspot.com/4627064/diff/10001/src/cmd/gotest/gotest.go
File src/cmd/gotest/gotest.go (right):

http://codereview.appspot.com/4627064/diff/10001/src/cmd/gotest/gotest.go#new...
src/cmd/gotest/gotest.go:250: s := doRun(runScriptArgv(argv), true)
the 'run' makes it sound like it runs the command, but it's just building
arguments.
scriptArgv and binaryArgv would be better names

[brainman, 2011-06-27 04:36:42]

On 2011/06/27 04:29:13, vcc wrote:
> 
> I try to fixes the problem is gotest run fail on my working windows amd64
port.
> 

Well, you should fix it in your "working windows amd64 port", not in main go
repo. Unless we could reproduce it here, of course.

I think all that extra code is not needed. Please, convince me otherwise.

Alex

[rsc, 2011-06-27 20:04:23]

Please keep the filepath change but drop the extra files.

http://codereview.appspot.com/4627064/diff/10001/src/cmd/gotest/Makefile
File src/cmd/gotest/Makefile (right):

http://codereview.appspot.com/4627064/diff/10001/src/cmd/gotest/Makefile#newc...
src/cmd/gotest/Makefile:12: GOFILES_darwin=\
This is a lot of work to avoid one if statement.

http://codereview.appspot.com/4627064/diff/10001/src/cmd/gotest/gotest.go
File src/cmd/gotest/gotest.go (left):

http://codereview.appspot.com/4627064/diff/10001/src/cmd/gotest/gotest.go#old...
src/cmd/gotest/gotest.go:279: if runtime.GOOS == "windows" && command ==
"gomake" {
I think this is fine to leave here.  It will go away eventually on its own
(once we stop invoking make to do our work).

http://codereview.appspot.com/4627064/diff/10001/src/cmd/gotest/gotest.go
File src/cmd/gotest/gotest.go (right):

http://codereview.appspot.com/4627064/diff/10001/src/cmd/gotest/gotest.go#new...
src/cmd/gotest/gotest.go:89: runTestWithArgs("." + string(filepath.Separator) +
O + ".out")
Nice change.

[vcc, 2011-07-14 06:09:53]

2011/6/27  <alex.brainman@gmail.com>:
> On 2011/06/27 04:29:13, vcc wrote:
>
>> I try to fixes the problem is gotest run fail on my working windows
>
> amd64 port.
>
>
> Well, you should fix it in your "working windows amd64 port", not in
> main go repo. Unless we could reproduce it here, of course.
>
> I think all that extra code is not needed. Please, convince me
> otherwise.

Don't need this change now, all test in pkg is PASS for windows amd64
port now. sorry to waste your time.

Wei guangjing

>
> http://codereview.appspot.com/4627064/
>

[brainman, 2011-07-14 06:19:34]

On 2011/07/14 06:09:53, vcc wrote:
> 
> Don't need this change now, all test in pkg is PASS for windows amd64
> port now. ...
> 

Very nice. Congrats. I am shocked how few changes were needed!

Alex

[remyoudompheng, 2013-07-20 07:16:21]

R=close