This ticket is a migrated Trac ticket 2878
People contributed to the original ticket: @djmitche, caktux@...
, @tardyp, @unknown_contributor, @unknown_contributor, @unknown_contributor, @unknown_contributor, @sa2ajj
Ticket created on: Aug 22 2014
Ticket last modified on: Apr 22 2016
I am not sure for how long those paste's will be available:
Error message:
'%VCENV_BAT%' is not recognized as an internal or external command,
operable program or batch file.
Discussion on IRC starts at http://irclogs.jackgrigg.com/irc.freenode.net/buildbot/2014-08-22#i_3429967
BAT file that produces the error: http://paste.debian.net/116918
The problem seems to be
The question is why &
were escaped this way.
Comment from: @sa2ajj
Date: Aug 22 2014
It seems that it needs to be escaped when passed as a parameter, which does not seem to be our case.
Comment from: @dustin
Date: Aug 24 2014
The pastes are gone already :(
Comment from: @unknown_contributor
Date: Sep 09 2014
If you run the command as a parameter to cmd.exe, then it becomes a parameter and the escaping behaviour is correct.
Try prefixing the list of command line arguments with [["/c"|"cmd",]] (buildbot-0.8.9/buildbot/steps/vstudio.py line 418). I use this in my configuration.
Comment from: @sa2ajj
Date: Sep 09 2014
@otterdam, do you have the code you mentioned modified, or you use some external parameters?
I've been looking at this problem (unfortunately, no more information came my way) and it seems that Buildslave on Windows platform always runs commands using "cmd /c"...
Comment from: @unknown_contributor
Date: Dec 14 2014
otterdam's solution works here(tm), seems "cmd", "/c" should be added to vstudio.py
Comment from: @sa2ajj
Date: Dec 14 2014
Looks like [[logic|http://trac.buildbot.net/browser/slave/buildslave/runprocess.py?rev=7b9fb2d23e9ea07e1757fb31b5c309ab4e51bbf7#L437]] needs to be reviewed.
Comment from: @unknown_contributor
Date: Mar 17 2015
Can anybody express why buildbot doesn't simply use subprocess module for spawning stuff?..
Comment from: @tardyp
Date: Mar 17 2015
buildbot is based on asynchrounous library called twisted. subprocess is synchronous, which means we could only spawn one process at a time. You can however use threads with twisted. If it happens to be proven more stable, I think a patch that go in that way for windows would be gladly accepted.
We are lacking good windows expertise contributions.
Comment from: @unknown_contributor
Date: Mar 18 2015
That is completely untrue :) subprocess could spawn as much processes at a time as you need - just don't do .call() or .wait() unconditionally after subprocess.Popen().
Why I'm asking this is that subprocess handles quoting and stuff internally and is widely-tested given it's included in Python standard library, so IMHO it's much better to rely on this functionality rather than write some code that you cannot even test good enough since you lack Windows resources...
I see some more bugs that I think all could be resolved by switching to subprocess (at least on Windows).
P.S. Moving to Job Objects is a bad choice since there could be no nested Job Objects, and you cannot guarantee that a program you launch isn't using Job Objects inside.
Comment from: @tardyp
Date: Mar 18 2015
just don't do .call() or .wait() unconditionally after subprocess.Popen().
JustAMan, I would advice you make some experiment on the buildslave code to convince your self that this is not as simple as you say.
- How would you watch for subprocess stdios, and stream the results in realtime to the master?
- How would you wait for the processes to finish?
The parallelism here is completely independant. The same slave runs arbitrary number of builder at the same time with arbitrary timing on sub process spawn and termination.
For the rest, I agree that subprocess is probably much more reliable on windows than twisted's process management
http://twistedmatrix.com/documents/13.2.0/core/howto/process.html
How can we reuse the windows workaround methods of subprocess inside twisted is the question
Comment from: @unknown_contributor
Date: Mar 19 2015
I suggest we move Buildbot process spawning code to subprocess.
Both issues (watching for stdio and waiting for finish) are almost easy.
We have SCons-based build system where I worked on refactoring how processes are spawned, and this system "tee's" process output to a separate file and to console without much problem. The same goes to waiting for process to finish or killing the whole tree.
I can talk to our system architect who's right now working on open-sourcing it if we can move this process handling code to a subpackage. Or maybe I can just copy-paste it to Buildbot as it's about 15K overall, and is regularly tested on Windows, Linux and MacOS (and now it works on FreeBSD as well).
If that won't suit Buildbot community I suggest we at least move to subprocess.list2cmdline() function instead of ugly manual quoting.
Comment from: @sa2ajj
Date: Mar 19 2015
Any solution to the problem is very welcome.
15K as in "15K lines"?
If you think you could provide a PR, we could take it from there.
Comment from: @unknown_contributor
Date: Mar 20 2015
15K is "15 Kilobytes" of code.
I'll talk to our open-sourcing guy then.
Comment from: @unknown_contributor
Date: Mar 25 2015
I have a general "let's publish this code" approval, will work on that.
On a related note, does anyone have original "pastes" that could be used to reproduce the issue?
Comment from: @unknown_contributor
Date: Mar 09 2016
This is still a problem in Buildbot 0.8.12. I worked around it by modifying the "win32_batch_quote" function in buildslave\runprocess.py. I made two changes:
- Added a special case so "&&" is not escaped.
- Allowed environment variable expansion by not changing "%" characters to "%%".
I'm not saying this is the right way to do it but it's working for me. Here is the complete function I'm using now:
def win32_batch_quote(cmd_list):
def escape_arg(arg):
if arg == '|':
return arg
if arg == '&&':
return arg
arg = quoteArguments([arg])
# escape shell special characters
arg = re.sub(r'[@()^"<>&|]', r'^\g<0>', arg)
return arg
return ' '.join(map(escape_arg, cmd_list))
Comment from: @unknown_contributor
Date: Apr 19 2016
Since I'm the one who implemented the current escaping logic, I think I need to weigh in.
Unfortunately, the pastes in the description are no longer available, so I don't know for sure, but I'm going to guess that the string %VCENV_BAT%
was used as the first element of a list passed as the command to [[ShellCommand]]
. When the command is a list, Buildbot runs the process in such a way that the program's argv
are exactly the elements of the list. On Windows, that involves escaping all characters that are meaningful to the shell, including %
and &
.
This is nothing new. As far as I understand, on Unix-likes it has always been that way. On Windows, the escaping has been rather lax (only handling the pipe character) until I tightened it around 0.8.9. I did this to ensure consistency between Windows and non-Windows, and to solve issues like [[these|https://lists.buildbot.net/pipermail/devel/2013-August/009978.html]] (I had my own problem similar to that one).
To summarize, I don't think there is a problem here. If you want shell processing, you need to tell Buildbot to use the shell by passing the command as a single string. No escaping happens then.
Comment from: @unknown_contributor
Date: Apr 20 2016
Whether dpb is right or wrong, all Visual C++ steps are broken as-is.
http://docs.buildbot.net/current/manual/cfg-buildsteps.html#visual-c
They all use a shell command with a list and expect %VCENV_BAT%
to be expanded to a path. So either win32_batch_quote
needs to change or the Visual C++ steps need an update to pass the command as a single string.
Comment from: @unknown_contributor
Date: Apr 20 2016
Ah, now I get it. Yeah, that is a problem. I'll see if I can do anything about that.
Comment from: @unknown_contributor
Date: Apr 22 2016
#2159