code review 5937043: cmd/dist: don't fail when Mercurial is a batch file on ...

cmd/dist: don't fail when Mercurial is a batch file on Windows

On windows Mercurial installed with easy_install typically creates
an hg.bat batch file in Python Scripts directory, which cannot be used
with CreateProcess unless full path is specified. Work around by
launching hg via cmd.exe /c.

Additionally, fix a rare FormatMessageW crash.

Fixes issue 3093.

[snaury, 2012-03-27 17:29:13]

Hello golang-dev@googlegroups.com,

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

[rsc, 2012-03-27 18:14:18]

We can look into this, but only after Go 1.
The Go 1 release branch will contain a VERSION file,
so this won't matter nearly as much anyway.

Russ

[brainman, 2012-03-28 01:39:43]

Please, add "Fixes issue 3093." to CL description.

Alex

http://codereview.appspot.com/5937043/diff/2001/src/cmd/dist/windows.c
File src/cmd/dist/windows.c (right):

http://codereview.appspot.com/5937043/diff/2001/src/cmd/dist/windows.c#newcod...
src/cmd/dist/windows.c:289: if(i == 0 && streq(q, "hg")) {
I have one problem with your approach. How do we deal with hg exit code. We can
see when hg.exe fails by looking at its exit code. Would cmd.exe return "failed"
exit code when Mercurial fails when we use hg.bat?

http://codereview.appspot.com/5937043/diff/2001/src/cmd/dist/windows.c#newcod...
src/cmd/dist/windows.c:292: xgetenv(&comspec, "COMSPEC");
I do not think you should worry about COMSPEC environment variable. We know

cmd.exe /c hg

will work. Do we know if

%COMSPEC% /c hg

will too?

I think we should assume that cmd.exe is our command interpreter. When it fails,
we will deal with that.

[rsc, 2012-03-28 01:45:43]

Let's wait on the review discussion until after Go 1.  Thanks.

[snaury, 2012-03-29 19:12:21]

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

Please take another look.

[snaury, 2012-03-29 19:16:18]

On 2012/03/28 01:39:43, brainman wrote:
> Please, add "Fixes issue 3093." to CL description.

Done

> I have one problem with your approach. How do we deal with hg exit code. We
can
> see when hg.exe fails by looking at its exit code. Would cmd.exe return
"failed"
> exit code when Mercurial fails when we use hg.bat?

Yes, cmd.exe passes the response back to its caller.

> I do not think you should worry about COMSPEC environment variable. We know
> 
> cmd.exe /c hg
> 
> will work. Do we know if
> 
> %COMSPEC% /c hg
> 
> will too?

Well, I think it is generally advisable to use COMSPEC. Although unlikely, user
might not have cmd.exe on the path, but %COMSPEC% should always be correct. On
Windows it's similar to what's SHELL is on unix, where it might not always be a
good idea to hard-code /bin/bash or /bin/sh.

> I think we should assume that cmd.exe is our command interpreter. When it
fails,
> we will deal with that.

Well, if you really insist I could leave just cmd.exe, I only did it this way
for portability and correctness.

[aram, 2012-03-29 19:30:59]

> Well, I think it is generally advisable to use COMSPEC. Although unlikely,
user
> might not have cmd.exe on the path, but %COMSPEC% should always be correct.

%COMSPEC% should be fine. The command interpreter was changed from command.com
to cmd.exe during Windows' lifetime. While it's unlikely to change again it's
more about honoring user's preferences, I know a few folks who run alternate,
but compatible shells.

[brainman, 2012-03-30 01:29:26]

On 2012/03/29 19:16:18, snaury wrote:
> ...
> > I have one problem with your approach. How do we deal with hg exit code. We
> can
> > see when hg.exe fails by looking at its exit code. Would cmd.exe return
> "failed"
> > exit code when Mercurial fails when we use hg.bat?
> 
> Yes, cmd.exe passes the response back to its caller.
> 

I am not convinced. Could you, please, tell me your "Mercurial with hg.bat"
setup. I would like to recreate the issue, you are seeing, myself. I would like
to see if your change will handle situation where Mercurial fails for one reason
or the other. Thank you.

> > I do not think you should worry about COMSPEC environment variable. We know
> > 
> > cmd.exe /c hg
> > 
> > will work. Do we know if
> > 
> > %COMSPEC% /c hg
> > 
> > will too?
> 
> Well, I think it is generally advisable to use COMSPEC. ...

I have no problem with us using different command interpreter, the one pointed
by COMSPEC. I am concerned that it might have different (from cmd.exe) arguments
for executing batch files. We do know what "cmd.exe /c hg" does. But we can't be
sure that "shell.exe /c hg" will to, if COMPSEC=shell.exe. Can we? How can you
be sure if that "other shell program" even has "/c" option, if you do not know
what this program is? How can you be sure that "shell.exe /c hg" does not delete
all files on c:\, instead of running mercurial? :-)

Alex

[Joe Poirier, 2012-03-30 03:14:27]

On Thu, Mar 29, 2012 at 8:29 PM,  <alex.brainman@gmail.com> wrote:
> On 2012/03/29 19:16:18, snaury wrote:
>> > cmd.exe /c hg
>> >
>> > will work. Do we know if
>> >
>> > %COMSPEC% /c hg
>> >
>> > will too?
>
>
>> Well, I think it is generally advisable to use COMSPEC. ...
>
>
> I have no problem with us using different command interpreter, the one
> pointed by COMSPEC. I am concerned that it might have different (from
> cmd.exe) arguments for executing batch files. We do know what "cmd.exe
> /c hg" does. But we can't be sure that "shell.exe /c hg" will to, if
> COMPSEC=shell.exe. Can we? How can you be sure if that "other shell
> program" even has "/c" option, if you do not know what this program is?
> How can you be sure that "shell.exe /c hg" does not delete all files on
> c:\, instead of running mercurial? :-)
>

On a healthy system, ComSpec should return cmd.exe with the full path
to the system folder it resides in. AFAIK, true from Windows 2K and
up.

If ComSpec returns shell.exe, it's possible that said system is
infected with spyware\trojan\virus\malware.

-joe

[mattn, 2012-03-30 04:58:19]

http://codereview.appspot.com/5937043/diff/2001/src/cmd/dist/windows.c
File src/cmd/dist/windows.c (right):

http://codereview.appspot.com/5937043/diff/2001/src/cmd/dist/windows.c#newcod...
src/cmd/dist/windows.c:297: bwritestr(&cmd, " /c ");
Shoud be " /c call ". If "cmd /c hg" can't get exit code if "hg" is "hg.bat" in
your PATH.
"cmd /c call" works with hg.exe / hg.bat both.

[mattn, 2012-03-30 04:59:30]

Ah, typo.

> Shoud be " /c call ". "cmd /c hg" can't get exit code if "hg" is "hg.bat" 
in your PATH.


On Friday, March 30, 2012 1:58:19 PM UTC+9, mattn wrote:
>
>
> http://codereview.appspot.com/5937043/diff/2001/src/cmd/dist/windows.c
> File src/cmd/dist/windows.c (right):
>
>
>
http://codereview.appspot.com/5937043/diff/2001/src/cmd/dist/windows.c#newcod...
> src/cmd/dist/windows.c:297: bwritestr(&cmd, " /c ");
> Shoud be " /c call ". If "cmd /c hg" can't get exit code if "hg" is
> "hg.bat" in your PATH.
> "cmd /c call" works with hg.exe / hg.bat both.
>
> http://codereview.appspot.com/5937043/
>
>

[snaury, 2012-03-30 05:33:41]

On 2012/03/30 01:29:26, brainman wrote:
> I am not convinced. Could you, please, tell me your "Mercurial with hg.bat"
> setup. I would like to recreate the issue, you are seeing, myself. I would
like
> to see if your change will handle situation where Mercurial fails for one
reason
> or the other. Thank you.

Ah, sorry. I just checked and it does not. But that has nothing to do with
cmd.exe, it's the fault of Mercurial, because *hg.bat* always succeeds (they
don't bother passing errors back from python).

However, if I make hg.exe then cmd.exe /c hg does return non-zero exit status
when mercurial fails.

> I have no problem with us using different command interpreter, the one pointed
> by COMSPEC. I am concerned that it might have different (from cmd.exe)
arguments
> for executing batch files. We do know what "cmd.exe /c hg" does. But we can't
be
> sure that "shell.exe /c hg" will to, if COMPSEC=shell.exe. Can we? How can you
> be sure if that "other shell program" even has "/c" option, if you do not know
> what this program is? How can you be sure that "shell.exe /c hg" does not
delete
> all files on c:\, instead of running mercurial? :-)

But then everything would be breaking on a system like that, because when you
specify COMSPEC on Windows, it's expected to be compatible. It's also used by
the standard runtime, and recommended on msdn, see:

http://msdn.microsoft.com/en-us/library/277bwbdz.aspx
http://msdn.microsoft.com/en-us/library/ms404706.aspx

The advantage of COMSPEC is also that it is a full path to cmd.exe, and will
always execute the right one, instead of some D:\tools\cmd.exe that happened to
be in the PATH before system32.

[snaury, 2012-03-30 05:39:14]

On 2012/03/30 04:59:30, mattn wrote:
> Shoud be " /c call ". "cmd /c hg" can't get exit code if "hg" is "hg.bat" 
> in your PATH.

Oh, interesting. Might be some artifact to allow parent interpreters know exit
code of whatever was executed, instead of batch file itself. Unfortunately, the
batch file itself always succeeds. You can see it by e.g. trying:

cmd := exec.Cmd("hg", "should-be-an-error")
panic(cmd.Run())

You will see that since hg is a batch file, it will always succeed.

[brainman, 2012-03-30 05:51:00]

On 2012/03/30 05:33:41, snaury wrote:
> 
> ... I just checked and it does not. ...

I think it is big problem. We are executing program, and we can't determine if
it was successful or not. You are suggesting, we disregard the fact that it
fails and continue. But what are the implications?

I suggest, we leave everything as is. If someone wants to use cmd/dist, they
would have to install hg.exe. I do not think it is a big ask.

I am not dead against it. I just think it is silly to support stuff that is
broken. Leaving for others to decide.

Alex

[snaury, 2012-03-30 05:51:41]

On 2012/03/30 05:33:41, snaury wrote:
> http://msdn.microsoft.com/en-us/library/277bwbdz.aspx
> http://msdn.microsoft.com/en-us/library/ms404706.aspx

Hmm, but then CreateProcess recommends cmd.exe. Ok then, I'm gonna cave in.

[snaury, 2012-03-30 05:51:42]

Hello golang-dev@googlegroups.com, rsc@golang.org, alex.brainman@gmail.com,
aram@mgk.ro, jdpoirier@gmail.com, mattn.jp@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[snaury, 2012-03-30 06:04:54]

On 2012/03/30 05:51:00, brainman wrote:
> I think it is big problem. We are executing program, and we can't determine if
> it was successful or not. You are suggesting, we disregard the fact that it
> fails and continue. But what are the implications?

The implications are unchanged, I think. Exactly the same behavior happens with
go get, for example when I deliberatly introduce an error in mercurial, this
happens:

# cd C:\Alex\go\src\pkg\code.google.com\p\go.crypto; hg tags
package code.google.com/p/go.crypto: chdir
C:\Alex\go\src\pkg\code.google.com\p\go.crypto: The system cannot find the file
specified.

The error above is because hg clone silently failed. I'd say this is not our
problem if hg.bat is made to always succeed.

[snaury, 2012-03-30 06:13:37]

Btw, hg.bat can be easily fixed by adding:

if errorlevel 1 %COMSPEC% /c exit /b %errorlevel%

As the very last line.

[mattn, 2012-03-30 06:57:44]

This is similar that I discused in golang-dev.

https://groups.google.com/d/topic/golang-dev/ujtLlrp8e_c/discussion

On Friday, March 30, 2012 3:13:37 PM UTC+9, Alexey Borzenkov wrote:
>
> Btw, hg.bat can be easily fixed by adding:
>
> if errorlevel 1 %COMSPEC% /c exit /b %errorlevel%
>
> As the very last line.
>
> http://codereview.appspot.com/5937043/
>
>

[snaury, 2012-03-30 20:49:44]

On 2012/03/30 06:57:44, mattn wrote:
> This is similar that I discused in golang-dev.
> 
> https://groups.google.com/d/topic/golang-dev/ujtLlrp8e_c/discussion

Your patch was about working around a bug in third-party software, I would be
strongly against that, it's Selenic's job to fix this, perhaps all that's needed
is a proper bug report on their tracker.

My patch is about a fix for a bug in *go*, where dist fails to execute hg.bat.
Even if hg.bat worked perfectly, dist would still fail to execute it. There's a
difference between "go tip cannot be built on windows" vs "tomorrow selenic
might fix this bug and everything would be perfect".

[mattn, 2012-03-31 14:18:25]

> Oh, interesting. Might be some artifact to allow parent interpreters
> know exit code of whatever was executed, instead of batch file itself.
> Unfortunately, the batch file itself always succeeds. You can see it by
> e.g. trying:

Yes, I know. I did explain about this in the thread.
I don't think patching for hg.bat don't solve problem fundamentally.

> Your patch was about working around a bug in third-party software, I
> would be strongly against that, it's Selenic's job to fix this, perhaps
> all that's needed is a proper bug report on their tracker.
> My patch is about a fix for a bug in *go*, where dist fails to execute
> hg.bat. Even if hg.bat worked perfectly, dist would still fail to
> execute it. There's a difference between "go tip cannot be built on
> windows" vs "tomorrow selenic might fix this bug and everything would be
> perfect".

I'm not talking "My CL is better". :)
I'm talking that go must handle exit code. To handle exit code of batch 
file, we should call "cmd /c call".

[snaury, 2012-03-31 14:47:05]

On 2012/03/31 14:18:25, mattn wrote:
> I'm talking that go must handle exit code. To handle exit code of batch 
> file, we should call "cmd /c call".

No, because in that case you do another thing entirely: you fool cmd.exe into
thinking that the batch file is called from another batch file and errorlevel
needs to propagate. Yes, sure, it would work with hg.bat in this case, because
python is the last thing it executes that changes errorlevel (and it just
happens that endlocal does not). But it's a wrong concept on, because batch file
*does not* fail. Unlike bash, which returns exit code of the last invocation,
it's a job of batch file writer to exit with an appropriate exit code.
Unfortunately people rarely do that (and cmd.exe, especially on WinXP, has
horrible and ancients bugs/quirks).

Using "cmd.exe /c call" would be counter productive. While dist would start
seeing "errors", os.StartProcess consequently exec.Command do not use cmd.exe or
COMSPEC at all, instead searching PATH for a suitable PATHEXT file and using it
as a first parameter. That invocation does not use "/c call".

I'm thinking about reworking this patch by porting LookPath from package os to C
so that it's done uniformly by both dist and the rest of go, then I hope this
would finally stop being about cmd.exe and hg.bat bugs.

[brainman, 2012-04-01 04:26:35]

On 2012/03/30 06:04:54, snaury wrote:
> 
> ... Exactly the same behavior happens with
> go get, ...

I am under impression that "go get ..." will fail, if it is calling hg.exe and
hg.exe fails.

Are you saying, that if user do not have hg.exe installed, but uses hg.bat
instead, then "go get ..." will succeed, while Mercurial command inside hg.bat
fails? I am prepared to believe that. If that is the case, perhaps we should do
something to prevent this inconsistency. Maybe show some warning about us using
"batch" file, maybe refuse to execute it altogether.

Alex

[brainman, 2012-04-01 04:34:28]

On 2012/03/31 14:47:05, snaury wrote:
> 
> I'm thinking about reworking this patch by porting LookPath from package os to
C
> so that it's done uniformly by both dist and the rest of go, then I hope this
> would finally stop being about cmd.exe and hg.bat bugs.

But it is a hg issue at this moment. cmd/dist is bit specific in what it does
and who it calls. I am not sure all that extra code is worth the effort. But, as
I said earlier, I am OK either way.

Alex

[rsc, 2012-04-02 16:15:32]

I'm surprised that this works.  If Alex is okay with it, fine.
It looks like it is down to just a few lines.

Russ

[snaury, 2012-04-02 20:56:21]

Hello golang-dev@googlegroups.com, rsc@golang.org, alex.brainman@gmail.com,
aram@mgk.ro, jdpoirier@gmail.com, mattn.jp@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[snaury, 2012-04-02 20:59:48]

On 2012/04/02 16:15:32, rsc wrote:
> I'm surprised that this works.  If Alex is okay with it, fine.
> It looks like it is down to just a few lines.

As promised I've updated it with an implementation for lookpath. Now it
correctly searches for files using PATHEXT. There's also an important fix
regarding FormatMessage, as during testing I found that if the error message is
something like "%1 is not a valid Win32 application" dist would crash instead of
displaying said error message.

I hope it's not too complex, and I hope it's correct. One big advantage of this
approach is that now it would support batch files (and more) for anything, not
just mercurial.

[snaury, 2012-04-02 21:03:04]

Oops, looks like I forgot to fight myself over if/for parentheses in a couple
places. Will fix shortly.

[snaury, 2012-04-02 21:08:01]

Hello golang-dev@googlegroups.com, rsc@golang.org, alex.brainman@gmail.com,
aram@mgk.ro, jdpoirier@gmail.com, mattn.jp@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2012-04-03 01:18:54]

LGTM

Thank you.

Leaving for Russ.

Alex

[rsc, 2012-04-04 13:44:42]

What just happened?  We went from a 12-line patch (patch set 3)
to a 137-line patch (patch set 7).  I was happy with the 12-line patch.
It was ugly but it presumably worked, and we only care about
Mercurial, not about handling every possible general case.
The new code seems like it is solving a problem that misc/dist
doesn't have.

Russ

[brainman, 2012-04-05 04:33:37]

On 2012/04/04 13:44:42, rsc wrote:
> What just happened?  We went from a 12-line patch (patch set 3)
> to a 137-line patch (patch set 7). ...

I do not think this issue is important enough to spend so much time on it. I do
not use hg.bat myself.

I am OK to:
1) drop this CL and leave everything as is;
2) have short (patch set 3) change;
3) have long (current) change.

Either choice have positives and negatives:
1) no support of hg.bat, but everything works as expected;
2) supports hg.bat, but might cause confusion if Mercurial fails, but hg.bat
returns success;
3) same as 2), but does not rely on existence of cmd.exe; it is also similar to
what we do in os/exec package, so some consistency here.

You choose.

Alex

[rsc, 2012-04-05 16:12:43]

I would like to do patch set 3.

Russ

[snaury, 2012-04-09 18:59:27]

On 2012/04/05 16:12:43, rsc wrote:
> I would like to do patch set 3.
> 
> Russ

I'd like to clarify, do you want patch set 3 (long, with comspec), or patch set
5 (short, with cmd.exe only)? I (or codereview) might be counting patch sets
differently...

[rsc, 2012-04-09 19:01:29]

Either patch set 3 or patch set 5 would be fine.
Both are small enough to be justifiable for this situation.
The current patch set is, in contrast, enormous.

[snaury, 2012-04-09 19:07:22]

Hello golang-dev@googlegroups.com, rsc@golang.org, alex.brainman@gmail.com,
aram@mgk.ro, jdpoirier@gmail.com, mattn.jp@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[aram, 2012-04-09 19:14:07]

I still don't understand why this works, how do we detect
failure? Maybe we don't care about failure? The hg.bat
thing is broken anyway, this is a hack supporting a hack,
so if this *mostly* works, fine by me.

Code wise LGTM.

[snaury, 2012-04-09 19:26:25]

On 2012/04/09 19:14:07, aram wrote:
> I still don't understand why this works, how do we detect
> failure? Maybe we don't care about failure? The hg.bat
> thing is broken anyway, this is a hack supporting a hack,
> so if this *mostly* works, fine by me.

Oh come on, not this again! There's nothing to detect, because hg.bat that
Mercurial ships *never fails*. If it would ever fail (either when Mercurial
fixes it, or because cmd.exe cannot find and execute hg.bat, or whatever else),
then cmd.exe will return non-zero exit code, and then it would fail, and then
*that* would be detected. But since hg.bat is written in such a way that it
never fails: there's nothing to detect.

[rsc, 2012-04-09 19:39:44]

LGTM

[rsc, 2012-04-09 19:40:04]

*** Submitted as http://code.google.com/p/go/source/detail?r=9a55b03991e3 ***

cmd/dist: don't fail when Mercurial is a batch file on Windows

On windows Mercurial installed with easy_install typically creates
an hg.bat batch file in Python Scripts directory, which cannot be used
with CreateProcess unless full path is specified. Work around by
launching hg via cmd.exe /c.

Additionally, fix a rare FormatMessageW crash.

Fixes issue 3093.

R=golang-dev, rsc, alex.brainman, aram, jdpoirier, mattn.jp
CC=golang-dev
http://codereview.appspot.com/5937043

Committer: Russ Cox <rsc@golang.org>