Closed Bug 952397 Opened 10 years ago Closed 10 years ago

Merge |mach valgrind-test| and valgrind_test.py

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(1 file, 1 obsolete file)

|mach valgrind-test| does some stuff and then calls build/valgrind/valgrind_test.py.  There's no good reason for the split.  Let's merge them!
2 files changed, 88 insertions(+), 115 deletions(-)
Comment on attachment 8350488 [details] [diff] [review]
Merge |mach valgrind-test| and valgrind_test.py.

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

I'm not sure who's the right person to ask about this, so I'll ask both ted and gps.

For now, I'd just like quick feedback on whether this is a reasonable thing to do.  It certainly feels nicer than what we currently have...
Attachment #8350488 - Flags: feedback?(ted)
Attachment #8350488 - Flags: feedback?(gps)
No longer blocks: valgrind-on-tbpl
Comment on attachment 8350488 [details] [diff] [review]
Merge |mach valgrind-test| and valgrind_test.py.

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

FWIW, I actually prefer as much code as possible live outside of mach_commands.py. My thought process is that mach is simply one of possibly many frontends. It follows that you should make every attempt to separate the "library" code into a standalone and reusable module and have code in mach_commands.py handle command line and argument processing, calling into the library code, and output processing. This usually results in testable library code for the core functionality (since our test coverage of mach commands is rather poor ATM, this is a very good thing). Essentially, divide the frontend from backend so you can have multiple frontends.

In practice, this distinction isn't made very well. So I'm not going to hold this against you. If we need to make the distinction going forward, we can cross that bridge.

::: build/valgrind/mach_commands.py
@@ +23,5 @@
> +from mozhttpd import MozHttpd
> +from mozprofile import FirefoxProfile, Preferences
> +from mozprofile.permissions import ServerLocations
> +from mozrunner import FirefoxRunner
> +from mozrunner.utils import findInPath

We should avoid non-critical imports at module-level because it slows down mach command dispatching. These imports should be inside the mach command function. This is another reason why keeping things as separate modules is easier - you typically only need to manage a single import from the mach command function if most of the logic is external.

@@ +46,3 @@
>              return 1
>  
> +        PORT = 8888

This seems like something that should be configurable via an argument. Also, I'm not sure about MozHttpd, but Python's built-in HTTP server (which I'm guessing MozHttpd is built upon) can auto-select an available port. Unless firewalls are getting in the way, you should almost always let the kernel select an available port for you.

@@ +79,2 @@
>  
> +            firefox_args = ['http://localhost:%d/index.html' % PORT]

Shouldn't this be using httpd.server_address?

@@ +108,5 @@
> +                valgrind_args.append('--suppressions=' + supps_file2)
> +
> +            valgrind = 'valgrind'
> +            if not os.path.exists(valgrind):
> +                valgrind = findInPath(valgrind)

Oh, I guess mozbase has its own implementation of which.which()!

@@ +115,5 @@
> +                                   binary=self.get_binary_path(),
> +                                   cmdargs=firefox_args,
> +                                   env=env)
> +            runner.start(debug_args=[valgrind] + valgrind_args)
> +            status = runner.wait()

I assume FirefoxRunner has some kind of intelligent __del__ behavior so ctrl+c will terminate the process? If not, you'll need a try..finally here.

@@ +122,5 @@
> +            if status != 0:
> +                status = 1      # normalize status, in case it's larger than 127
> +                print('TEST-UNEXPECTED-FAILURE | valgrind-test | non-zero exit code')
> +
> +            sys.exit(status)

return status

(mach does the right thing)
Attachment #8350488 - Flags: feedback?(gps) → feedback+
(In reply to Gregory Szorc [:gps] from comment #3)
> > +        PORT = 8888
> 
> This seems like something that should be configurable via an argument. Also,
> I'm not sure about MozHttpd, but Python's built-in HTTP server (which I'm
> guessing MozHttpd is built upon) can auto-select an available port. Unless
> firewalls are getting in the way, you should almost always let the kernel
> select an available port for you.

This is a leftover from profileserver.py that I never fixed. The sane way to do this now with mozhttpd is to just pass port=0, and then use mozhttpd.get_url.
Thanks for the feedback.  This revised version...

- Localizes the imports.

- Fixes the PORT stuff.

- Uses |return| instead of |sys.exit|.

Ctrl-C terminates execution without as-is, so no changes are required there.
Attachment #8355090 - Flags: review?(ted)
Attachment #8355090 - Flags: review?(gps)
Attachment #8350488 - Attachment is obsolete: true
Attachment #8350488 - Flags: feedback?(ted)
Comment on attachment 8355090 [details] [diff] [review]
Merge |mach valgrind-test| and valgrind_test.py.

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

::: build/valgrind/mach_commands.py
@@ +48,3 @@
>              return 1
>  
> +        PORT = 8888

You're not actually passing this to MozHttpd anymore, you don't need it.

@@ +71,5 @@
>  
> +            locations = ServerLocations()
> +            locations.add_host(host='127.0.0.1',
> +                               port=PORT,
> +                               options='primary,privileged')

This doesn't actually work right now since you're passing PORT (which is not the actual port in use). It probably doesn't matter that much since you don't need the privileged part (quitter takes care of that). You can pass in httpd.httpd.server_port for correctness.

@@ +81,2 @@
>  
> +            firefox_args = [httpd.get_url()]

You could just ditch the intermediate var here since you only use it in one place.

@@ +103,5 @@
> +            valgrind_args.append('--suppressions=' + supps_file1)
> +
> +            # MACHTYPE is an odd bash-only environment variable that doesn't
> +            # show up in os.environ, so we have to get it another way.
> +            machtype = subprocess.check_output(['bash', '-c', 'echo $MACHTYPE']).rstrip()

That's kinda gross, but we'll let it slide for now.

@@ +110,5 @@
> +                valgrind_args.append('--suppressions=' + supps_file2)
> +
> +            valgrind = 'valgrind'
> +            if not os.path.exists(valgrind):
> +                valgrind = findInPath(valgrind)

If you did this before setting valgrind_args above you could just construct debug_args directly with valgrind as the first entry.

@@ +119,5 @@
> +                                   env=env)
> +            runner.start(debug_args=[valgrind] + valgrind_args)
> +            status = runner.wait()
> +
> +            httpd.stop()

This could probably stand to be in a try/finally block.
Attachment #8355090 - Flags: review?(ted) → review+
Comment on attachment 8355090 [details] [diff] [review]
Merge |mach valgrind-test| and valgrind_test.py.

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

I'll defer to ted for r+.

::: build/valgrind/mach_commands.py
@@ +48,1 @@
>              return 1

mach has the ability to filter commands pre-dispatch. An advantage of this is that the command shows up as "inactive" during |mach help|. It also prints error messages for you.

This is accomplished by passing a conditions=fn in the @Command. See mozbuild.base.MachCommandConditions and testing/mochitest/mach_commands.py for an example.
Attachment #8355090 - Flags: review?(gps) → feedback+
Blocks: 952605
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
I also pushed a trivial change to valgrind.sh so that it no longer prints TEST-UNEXPECTED-FAIL when Valgrind fails, because |mach valgrind-test| now does that:

https://hg.mozilla.org/build/tools/rev/21696c5b424a
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: