funny arg splitting for cmd.exe in some cases but not others

[zooko]

Why does "C:\WINDOWS\system32\cmd.exe /c" work in this buildstep:

 http://buildbot.darcs.net/builders/zooko%20allmydata%20virtual2%20Windows-XP%20i386/builds/186/steps/darcs/logs/stdio

but fail in this one:

 http://buildbot.darcs.net/builders/zooko%20allmydata%20virtual2%20Windows-XP%20i386/builds/185/steps/darcs/logs/stdio

With the error message "'C:\Program' is not recognized as an internal or external command, operable program or batch file."

The "argv" that the buildbot thinks it is passing to cmd.exe is ['C:
WINDOWS
system32
cmd.exe', '/c', 'C:
Program Files
darcsdir-w32
darcs.EXE', 'get', '--verbose', '--repo-name', 'build', '--partial', ' http://darcs.net/unstable']

in the first, working case, and ['C:
WINDOWS
system32
cmd.exe', '/c', 'C:
Program Files
darcsdir-w32
darcs.EXE', 'get', '--verbose', '--repo-name', 'build', '--partial', '--context', 'c:
Documents and Settings
buildslave
windows-darcs2
zooko allmydata virtual2
.darcs-context', ' http://darcs.net/unstable']

in the second, failing, case.

[bhearsum]

cmd.exe has some pretty crazy rules for arguments. I don't think it supports spaces like you want it to. You should try quoting or escaping that path (if you can), if not, the only solution is probably to move your slave to c:\slave, or something like that.

(fyi, here are the rules about quoting things with cmd.exe:  http://thesystemguard.com/TheGuardBook/CCS-Ext/helptext/Cmd-K3.txt.htm)

[greg]

I have this problem too. You could get around your particular instance of this problem by changing your PATH environment variable to say 'C:\Progra~1\Darcs...' instead of Program Files. It should work then. To be specific -- you can have spaces in the program, or in the arguments, but not both (as far as I can tell)

Greg.

[greg]

This may be related to python issue2304

 http://bugs.python.org/issue2304

[greg]

Actually, the condition above isn't quite right, because

['C:\\WINDOWS\\system32\\cmd.exe', '/c', 'C:\\Program Files\\Microsoft Visual Studio 9\\Common7\\IDE\\devenv.com', 'Clock.sln', '/Build', 'Release']

fails. Maybe it's "no more than one space (total)". hmm.

[greg]

"what works for me"(TM) fix for 377

[greg]

this is what has got things working for me. It will still fail if you are relying on PATH, and you have >1 space within your arguments.

[greg]

also, it gives rubbish error messages. if you've tried to run a exe it can't find, you get no output, and 'AbandonChain?' error thrown.

[greg]

gah. I've just come across a case where it doesn't work. Trying to rely on shell paths etc. when you've got spaces. but putting cmd /c back in works. rehack...

[dustin]

see also #322

[ddunbar]

After bumping into this problem repeatedly, and being unable to figure out how to solve it, I eventually just ended up putting the commands I wanted to run in .bat files and sending them to the slave.

What about adding a DotBatShellCommand? to buildbot which would simply put the command in a temporary .bat file, then run that with cmd /c. This appears to be very similar to what vcbuild does itself, for example.

[Nicolas]

I had trouble running Visual C++ 'vcbuild', because the command line contains a pipe character that is part of an argument, not interpreted as command piping. In an interactive cmd.exe, I use "foo|bar", but this didn't work in buildbot.

In the end, I had to edit the process-spawning code from twisted; I couldn't fix it from buildbot.

[Nicolas]

See  Twisted #3933

[zooko]

Man, this sucks. I think bhearsum and greg hit the nail on the head in the first two comments, and neither Twisted #3933 nor Python issue2304 nor greg's hacks can fix this issue. I guess ddunbar's suggestion in #comment:10 is the only way forward. Meanwhile, I guess the work-around is to rename all relevant paths on your Windows buildslaves to not have spaces in them. Argh. If anyone else can ponder  http://thesystemguard.com/TheGuardBook/CCS-Ext/helptext/Cmd-K3.txt.htm and figure out a fix to this, that would be great.

[ddunbar]

FWIW, I've been using a variant of my idea for quite a while on the LLVM buildbot and it works great. Basically I just put the commands in a file and use FileDownload? to send them to the slave. It's inconvenient but works -- I'd like to integrate the functionality into a proper shell command step (to improve the log, for example) but haven't had time to work on it.

[exarkun]

For what it's worth, the Twisted ticket is now closed (invalid). spawnProcess will not take responsibility for knowing that the process being executed is cmd.exe and account for this by adjusting its quoting rules. The proper place for this logic is in the code which is selecting cmd.exe as the process to run.

[dustin]

Discussion from #buildbot:

14:54 < ddunbar> at the risk of being boringly repetitive, the simple solution is to make ShellCommand create a .bat file on the slave
14:54 < ddunbar> imho, trying to understand cmd.exe is a lost cause
14:54 < djmitche> ddunbar: hmm
14:54 < djmitche> that just seems too simple
14:55 < ddunbar> this is what msbuild etc do, and there is a reason
14:55 < djmitche> does that introduce other metacharacters? %-thingies?
14:55 < maruel> ddunbar: not a bad idea per se
14:55 < djmitche> I'm not saying it's *wrong*, I'm just amazed the solution is that simple .. I kinda thought cmd.exe basically "executed" each line of a batch file?
14:56 < ddunbar> see #377 (and #595) if thats not whats already talked about
14:56 < exarkun> djmitche: I think it works because it makes it really obvious which things you need to do cmd.exe quoting for and which things you need to do CreateProcess quoting for.
14:56 < exarkun> djmitche: Instead of mixing them together and making it really hard for everyone to understand.
14:56 < ddunbar> I think its actually impossible to do the right escaping
14:57 < exarkun> ddunbar: Surely not in any particular case.
14:57 < exarkun> ddunbar: Just in general.
14:57 < ddunbar> that doesn't make sense
14:57 < ddunbar> if its possible in every particular case it is possible in general :)
14:57 < exarkun> mmm.  no.
14:57 < exarkun> If you know what program you're running, and you know what its quoting rules are, then you can do it right.
14:57 < exarkun> If you're running an arbitrary program with arbitrary quoting rules, then you can't.
14:57 < ddunbar> thats not true
14:57 < exarkun> Sure it is.
14:57 < ddunbar> or rather, thats not what I am saying
14:58 < djmitche> woah, before we go all metaphysical here.. can someone with access to a windows box hack up the slave side of ShellCommand to do this?
14:58 < ddunbar> I think there are arguments (strings of characters) which you cannot pass to programs, if you call createprocess which calls cmd.exe
14:58 < exarkun> ddunbar: Ah, I see.
14:58 < djmitche> ddunbar: that's what it looks like is going on here
14:58 < ddunbar> I could be wrong, I just think that their quoting schemes aren't strong enough
14:58 < djmitche> in particular, |
14:58 < exarkun> ddunbar: I'm not sure if that's true.
14:58 < ddunbar> right
14:58 < ddunbar> I tried really hard to quote | correctly and failed
14:58 < exarkun> Did you try ^?
14:59 < ddunbar> I don't remember
14:59 < exarkun> ^| should work for that.
14:59 < djmitche> so it seems like ShellCommand on win32 should *always* write to a .bat and run it?
14:59 < ddunbar> giving up and moving to .bat files made everything much simpler
14:59 < djmitche> or is that appreciably slower?
14:59 < maruel> djmitche: it wouldn't
14:59 < catlee> seems wrong somehow
14:59 < exarkun> djmitche: everything on windows is already so slow you probably won't notice.
14:59 < catlee> lol
14:59 < djmitche> hah
14:59 < ddunbar> true
14:59 < djmitche> anyway, I'd like this to be a part of ShellCommand .. the question is which way should the default go?
15:00 < exarkun> the default should be to work correctly
15:00 < djmitche> .. a *optional* part of ..
15:00 < exarkun> which is probably closer to the .bat thing than the current behavior
15:00 < djmitche> which means .bat all the time? or does that just introduce some other edge cases?
15:00 < exarkun> beats me :)
15:00 < djmitche> let's do .bat as the default in 0.8.0, and change it back if we find problems
15:00 < exarkun> someone should try and see
15:00 < djmitche> sound like a plan?
15:00 < ddunbar> I would heart that
15:01 < exarkun> it shouldn't be very hard to write a comprehensive set of tests

The plan was subsequently revised to not make this the *default* in 0.8.0, but we should have optional support available, at least. Something like ShellCommand(.., useBatch=True). The NEWS should state that this will become the default in 0.8.1, and those who do *not* want to use a batch file for certain commands should include useBatch=False now, even though it's the current default.

Now, I can put some code together, but I have no capacity to execute it, which means I'm not so helpful. So who's going to write this?

[fcrestois]

Could i add some basic test case that failed actually on Win32 Windows XP SP3, Buidbot 7.11, Twisted 8.2, Python 2.6

factory.addStep(ShellCommand?(command=r'echo "Win32|Release"')) stdio = \"Win32|Release\" must be "Win32|Release" factory.addStep(ShellCommand?(command=r'echo "Release"')) stdio = \"Release\" must be "Release" factory.addStep(ShellCommand?(command=['echo','Win32 Release'])) stdio = "Win32 Release" must be Win32 Release factory.addStep(ShellCommand?(command=['echo','"Release"'])) stdio = \"Release\"" must be "Release"

[fcrestois]

Could i add some basic test case that failed actually on Win32

Windows XP SP3, Buidbot 7.11, Twisted 8.2, Python 2.6

factory.addStep(ShellCommand??(command=r'echo "Win32|Release"'))

stdio = \"Win32|Release\" must be "Win32|Release"

factory.addStep(ShellCommand??(command=r'echo "Release"'))

stdio = \"Release\" must be "Release"

factory.addStep(ShellCommand??(command=['echo','Win32 Release']))

stdio = "Win32 Release" must be Win32 Release

factory.addStep(ShellCommand??(command=['echo','"Release"']))

stdio = \"Release\"" must be "Release"

[dustin]

Yes, test cases would be great - can you put that together?

[dustin]

Nobody's stepped up to work on this yet, so .. not in 0.8.0

[fcrestois]

a workaround is to disable the "buggy" twisted part ( http://twistedmatrix.com/trac/ticket/1123) in twisted\internet\_dumbwin32proc.py

Let the user make the correct quoting by changing

  cmdline = quoteArguments(args)

by

  cmdline = " ".join(args)