Colour output processor chokes on multi-part Actions

[jondo2010]

Cuppa's colour output processor chokes on multi-command actions such as:

Action("cd $DIR && $COMMAND")

The problem is that subprocess.Popen doesn't work when this command string is split() into an array. Discussed here: http://stackoverflow.com/questions/17742789/running-multiple-bash-commands-with-subprocess.

[ja11sop]

Yes - good link. I'm not sure what's the best approach here though. Is this something we fix or advise a work-around for as a "better" practice. For instance the example you provide could presumably be accomplished using two separate steps, possibly with commands?

[jondo2010]

I spent a few hours looking at this more thoroughly today.

Multipart actions, e.g.

env.Builder(action=[
    SCons.Action.Action(change_to_dir()),
    SCons.Action.Action("$COMMAND"),
    SCons.Action.Action(return_to_dir())
])

won't work with a parallel build because any action executed by another builder has the chance of getting executed while the pwd is changed from the build root.

SCons.Builder actually has support for a chdir parameter which is supposed to execute the actions in a different directory. This suffers the same problem as above, and is even warned about in the doc:

WARNING: Python only keeps one current directory location for all of the threads. This means that use of the chdir argument will not work with the SCons -j option, because individual worker threads spawned by SCons interfere with each other when they start changing directory.

Another possible solution lies in subprocess.Popen(), which supports a cwd parameter, and will execute the sub process in that specific directory. The problem then becomes "how the heck do I trickle down that parameter from the SConscript or Tool files that use the Builder/Action interface?" The only way I could see at doing this (and allow seamless compatibility with other build tools) would be to monkey-patch SCons.Action.CommandAction.execute() and SCons.Action._ActionAction.__init__() to pass the chdir parameter from the builder down to the spawn() method instead of uselessly calling os.chdir(chdir)

[jondo2010]

An simpler alternative which works is to modify IncrementalSubProcess.Popen2():

...
process = subprocess.Popen(
    " ".join(args_list),
    **dict( kwargs, close_fds=close_fds, shell=True )
)
...

[ja11sop]

I don't have a lot of experience with using shell=True - are the warnings around the use of this argument relevant to our use-case here? Should this be configurable or a default assuming this (as it appears) is the best option?

[jondo2010]

I think typically the warnings against using shell=True are about running untrusted code. Since this is not a webservice, I think this is not a problem.

I've also found another case where the current invocation of Popen results in unexpected behaviour: Passing the linker an rpath argument: -Wl,-rpath='$ORIGIN'. Without shell=True, the single quotes are not evaluated, and the final RPATH in the executable becomes '$ORIGIN', which doesn't work.

[ja11sop]

Sorry for the long delay in responding - I haven't gone ahead with this merge yet as adding shell=True seems to break on Linux - at least with my test setups. I'm still looking into how I might be able to address this...