1328393 - mach mixin run_process does not necessarily log commands that will actually parse or execute correctly

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Bill Gianopoulos [:WG9s]]

I found this while developing the patch for bug 1328153.

The run_process python function pr0vided as part of the mach mixins does not properly quote and/or escape the command arguments such that the logged command is non-parsable either by humans reading the log or the OS if you try to cut-and-paste the logged command into an interactive shell.

So this call:

self.run_process(['/usr/bin/notify-send', '--app-name=Mozilla Build System', 'Mozilla Build System', 'Build completed'])

results in logging:

/usr/bin/notify-send --app-name=Mozilla Build System Mozilla Build System Build complete

which is a totally ill-formatted command.

I would suggest modifying run+process such that all of the command arguments are scanned replacing all occurrences of the character "'" with "\'" and then surrounding the resultant string with the character "'" so that this log message would end up being:

/usr/bin/notify-send '--app-name=Mozilla Build System' 'Mozilla Build System' 'Build complete'

Comment 1

[Bill Gianopoulos [:WG9s]]

Hmm,but I bet doing that universally would slow the build down.  Perhaps add an option to espace-args that could be set to true in cases where it might be necessary? Or just change the messages about build completion status so that the msg string is either 'Build-completed' or 'Build-failed' and change the string 'Mozilla Build System' to be 'MozillaBuildSystem' in the run-process call so that all of this is unnecessary.

Comment 2

[Bill Gianopoulos [:WG9s]]

Hmm let me see what happens if i just escape the spaces in my other patch.

Comment 3

[Bill Gianopoulos [:WG9s]]

Attachment: Workaround

This avoids the issue where possible by not passing spaces in string constants to run_process where possible.  This patch is dependent on the patch for bug 1328153.

Comment 4

[Bill Gianopoulos [:WG9s]]

Attachment: Workaround

Comment 5

[Bill Gianopoulos [:WG9s]]

Attachment: Workaround

Comment 6

[Bill Gianopoulos [:WG9s]]

I am closing this.  I will open a new bug if I can come up with a better definition of the problem and.or a valid fix.