705400 - pyflakes analysis of talos reveals problems

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[William Lachance (:wlach)]

So I experiment with running pyflakes (a static analyzer for python) on talos today, and it revealed some problems. Most of them are just undefined variables, but there's a few that concern me:

./ffprocess.py:69: local variable 'terminate_result' is assigned to but never used
./ffprocess.py:113: undefined name 'terminate_result'

It appears that the method where this problem appears (cleanupProcesses) was meant to return a list of processes it cleaned up, but somehow its return value got put into another method, leaving the function returning nothing. We probably didn't notice this yet because the method is only used when a crash occurs.

./remotePerfConfigurator.py:143: undefined name 'fileContents'

This method is called when we were supposed to retrieve the remote manifest. It's supposed to return a variable called "filecontents". The fact that it was misnamed indicates that copying a remote application manifest and reading its results doesn't work.

In addition to these major problems, there was a bunch of stuff that was broken but was never called. Notably:

* cmanager_*'s "getRegisteredCounters" function
* ffprocess_*'s launchProcess and poll functions (with the exception of ffprocess_remote's launchProcess function)

Finally, there were a bunch of unused variables, imports, etc. These are less important but we might as well tidy things up. I'll attach a patch to fix all these issues. I haven't tested it myself, but I will try to do so next week (on Linux, mobile, and Windows).

Comment 1

[William Lachance (:wlach)]

Attachment: Pyflakes analysis of talos

Note this does not include devicemanager*.py

Comment 2

[William Lachance (:wlach)]

Attachment: Fixes for problems revealed by pyflakes

Comment 3

[William Lachance (:wlach)]

CCing bear just to keep releng in the loop on some of this stuff

Comment 4

[Jeff Hammel]

Comment on attachment 577033 [details] [diff] [review]
Fixes for problems revealed by pyflakes

You know one of my favorite things in the world is deleting lines of code.  Assuming this all tests out, a big f+ from me

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 577033 [details] [diff] [review]
Fixes for problems revealed by pyflakes

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

r=me as long as we test this in staging before landing.

::: talos/cmanager_win32.py
@@ -37,5 @@
>  
>  
>  import win32pdh
> -import win32pdhutil
> -

for some reason I thought we needed this, have you tested on win32 while collecting counters?

::: talos/ffprocess_win32.py
@@ +39,5 @@
>  import os
>  import shutil
>  import utils
>  import subprocess
> +import time

not sure we need this?

::: talos/ffsetup.py
@@ -292,5 @@
>                                                                   browser_config["init_url"])
>  
>          log = browser_config['browser_log']
> -        if (browser_config['webserver'] != 'localhost'):
> -            b_log = browser_config['deviceroot'] + '/' + browser_config['browser_log']

why is this removed?

::: talos/remotePerfConfigurator.py
@@ +138,5 @@
>                                        "device - either it doesn't exist yet "
>                                        "(have you run fennec at least once?) "
>                                        "or you don't have permissions to get it "
>                                        "(workaround: extract it from apk locally)")
> +              return filecontents.split('\n')

nice!

Comment 6

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #5)

> r=me as long as we test this in staging before landing.

For sure.

> ::: talos/cmanager_win32.py
> @@ -37,5 @@
> >  
> >  
> >  import win32pdh
> > -import win32pdhutil
> > -
> 
> for some reason I thought we needed this, have you tested on win32 while
> collecting counters?

No, I have not. The import isn't used anywhere, so the only reason this could be needed was if importing had some sort of side effect that we're implicitly depending on. I doubt it. I will test this to make sure it still works.

> ::: talos/ffprocess_win32.py
> @@ +39,5 @@
> >  import os
> >  import shutil
> >  import utils
> >  import subprocess
> > +import time
> 
> not sure we need this?

You're right, this was used later in the file (inside launchProcess). But that method was unused (hence we didn't see the problem) and I removed it elsewhere in the patch. I'll take this out.

> ::: talos/ffsetup.py
> @@ -292,5 @@
> >                                                                   browser_config["init_url"])
> >  
> >          log = browser_config['browser_log']
> > -        if (browser_config['webserver'] != 'localhost'):
> > -            b_log = browser_config['deviceroot'] + '/' + browser_config['browser_log']
> 
> why is this removed?

The b_log variable is never used.

Comment 7

[Mozilla RelEng Bot]

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

Comment 8

[William Lachance (:wlach)]

Attachment: Fixes for problems revealed by pyflakes (take 2)

This is essentially the same as the previous patch, updated to address the comments, bitrot and removing a few more imports that were spuriously added (ffsetup.py: shutil, ffprocess.py: devicemanager)

Comment 9

[William Lachance (:wlach)]

(In reply to Mozilla RelEng Bot from comment #7)
> Try run for 1f415ffe0b7e is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=1f415ffe0b7e
> Results (out of 79 total builds):
>     success: 78
>     failure: 1
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla.
> com-1f415ffe0b7e

This was a try run of the just-attached patch. The only failure here is a known problem (timeout when running Talos).

Comment 10

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #9)
> (In reply to Mozilla RelEng Bot from comment #7)
> This was a try run of the just-attached patch. The only failure here is a
> known problem (timeout when running Talos).

"timeout when running Talos on Android", that should say.

Comment 11

[William Lachance (:wlach)]

Pushed: http://hg.mozilla.org/build/talos/rev/5f202f20f7d8