Opened 3 years ago

Last modified 5 months ago

#2878 new defect

Windows escaping seem to produce problems

Reported by: sa2ajj Owned by:
Priority: major Milestone: 0.9.+
Version: 0.8.9 Keywords: windows
Cc:

Description

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.

Change History (21)

comment:1 Changed 3 years ago by sa2ajj

It seems that it needs to be escaped when passed as a parameter, which does not seem to be our case.

comment:2 Changed 3 years ago by dustin

  • Keywords windows added
  • Milestone changed from undecided to 0.9.0

The pastes are gone already :(

comment:3 Changed 3 years ago by otterdam

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 ["cmd", "/c"] (buildbot-0.8.9/buildbot/steps/vstudio.py line 418). I use this in my configuration.

comment:4 Changed 3 years ago by sa2ajj

@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:5 Changed 3 years ago by caktux

otterdam's solution works here(tm), seems "cmd", "/c" should be added to vstudio.py

comment:6 Changed 3 years ago by sa2ajj

Looks like logic needs to be reviewed.

comment:7 Changed 3 years ago by dustin

  • Milestone changed from 0.9.0 to 0.9.+

comment:8 Changed 2 years ago by JustAMan

Can anybody express why buildbot doesn't simply use subprocess module for spawning stuff?..

comment:9 Changed 2 years ago by tardyp

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:10 Changed 2 years ago by JustAMan

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:11 Changed 2 years ago by tardyp

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:12 Changed 2 years ago by JustAMan

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:13 Changed 2 years ago by sa2ajj

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:14 Changed 2 years ago by JustAMan

15K is "15 Kilobytes" of code.

I'll talk to our open-sourcing guy then.

comment:15 Changed 2 years ago by JustAMan

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:16 Changed 17 months ago by shoelzer

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:

  1. Added a special case so "&&" is not escaped.
  2. 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:17 Changed 16 months ago by dpb

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 (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:18 Changed 16 months ago by shoelzer

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:19 Changed 16 months ago by dpb

Ah, now I get it. Yeah, that is a problem. I'll see if I can do anything about that.

Note: See TracTickets for help on using tickets.