738310 - Talos does not run when there are spaces in browser path

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Fahim Zahur]

Talos does not open when there are spaces in Windows path for Firefox.

Comment 1

[Fahim Zahur]

Attachment: Added quotes around formatting string in ffprocess_win32

I've added quotes around the formatting string in the GenerateBrowserCommandLine function in ffprocess_win32.py. Working with Windows browser paths containing spaces in my machine now.

Comment 2

[Burak Yiğit Kaya [:BYK]]

Comment on attachment 608581 [details] [diff] [review]
Added quotes around formatting string in ffprocess_win32

Review of attachment 608581 [details] [diff] [review]:
-----------------------------------------------------------------

Why using triple quotes? This wraps the whole command line in single quotes which tend to have problems.

Comment 3

[Fahim Zahur]

(In reply to Burak Yiğit Kaya [:BYK] from comment #2)
> Comment on attachment 608581 [details] [diff] [review]
> Added quotes around formatting string in ffprocess_win32
> 
> Review of attachment 608581 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why using triple quotes? This wraps the whole command line in single quotes
> which tend to have problems.

Assuming that the double quotes are necessary around the browser_path for it to work with spaces, I am thinking there are 2 ways to fix this. One is to change the GenerateBrowserCommandLine function in ffprocess_win32.py, and the other is to change the bcontroller.py file so that after it reads in bcontroller.yml it adds double quotes to the browser_path inside command. 

Regarding the first approach I couldn't make it work any way other than with triple quotes. Anything else along with double quotes around browser_path seems to cause errors when reading in the bcontroller.yml file.

Do you think I should go for the second approach?

Comment 4

[Burak Yiğit Kaya [:BYK]]

(In reply to Fahim Zahur from comment #3)
> Assuming that the double quotes are necessary around the browser_path for it
> to work with spaces, I am thinking there are 2 ways to fix this. One is to
> change the GenerateBrowserCommandLine function in ffprocess_win32.py, and
> the other is to change the bcontroller.py file so that after it reads in
> bcontroller.yml it adds double quotes to the browser_path inside command. 
That's because writeConfigFile function in utils.py does not do proper quote escaping while writing the values to the yml file. Triple quotes include the single quotes which prevents the problem though instead of using triple quotes I would prefer escaped quotes like '\'"%s" %s ... \'' and of course a comment explaining all this in short for people not to get confused in the future.

> Do you think I should go for the second approach?
Nope, I looked at it before even the bug was opened and it is way more complicated that it looks.

Comment 5

[Fahim Zahur]

Attachment: Added escape quotes around formatting string and double quotes around browser_path

Added escape quotes around formatting string and double quotes around browser_path.

Comment 6

[Burak Yiğit Kaya [:BYK]]

Comment on attachment 608859 [details] [diff] [review]
Added escape quotes around formatting string and double quotes around browser_path

Review of attachment 608859 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good. The lines below the changed line needs a little styling though you got yourself your first r+ =)

Comment 7

[Jeff Hammel]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a1db31a6806f

Comment 8

[Mozilla RelEng Bot]

Try run for a1db31a6806f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a1db31a6806f
Results (out of 74 total builds):
    success: 72
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a1db31a6806f

Comment 9

[Jeff Hammel]

Looks green.  Needs to be tested on android (try does not yet test Talos changes on android)

Comment 10

[Mozilla RelEng Bot]

Try run for 024c9636b51a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=024c9636b51a
Results (out of 76 total builds):
    success: 75
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-024c9636b51a

Comment 11

[Jeff Hammel]

Looks green, pushing

Comment 12

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/77b823710ff6

Comment 13

[Jeff Hammel]

Thanks for the patch!