bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED WONTFIX

Status

Firefox Build System
General
RESOLVED WONTFIX
2 years ago
5 months ago

People

(Reporter: WG9s, Unassigned)

Tracking

Trunk

Firefox Tracking Flags

(firefox53 affected)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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'
(Reporter)

Comment 1

2 years ago
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.
(Reporter)

Comment 2

2 years ago
Hmm let me see what happens if i just escape the spaces in my other patch.
(Reporter)

Comment 3

2 years ago
Created attachment 8823443 [details] [diff] [review]
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.
(Reporter)

Comment 4

2 years ago
Created attachment 8823836 [details] [diff] [review]
Workaround
(Reporter)

Updated

2 years ago
Attachment #8823443 - Attachment is obsolete: true
(Reporter)

Comment 5

2 years ago
Created attachment 8824047 [details] [diff] [review]
Workaround
Attachment #8823836 - Attachment is obsolete: true
(Reporter)

Comment 6

2 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX

Updated

5 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.