Closed Bug 938289 Opened 6 years ago Closed 6 years ago

add support for running with DMD during mochitests

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

I needed this today and wanted to package it all up so nobody has to think about
it again.  Just the automation side of things, some DMD dumping needs to be added
elsewhere.
DMD requires some environment variables to be setup and this is the right place
to put all that.

I'm not totally crazy about specifying the directory of the path to the DMD libraries
to this function and in the forthcoming part 2, but I think it is the most flexible
way to make sure this part doesn't know about $OBJDIR/dist/lib and other parts don't
have to care what the particular libraries are named on which platforms.
Attachment #831715 - Flags: review?(jmaher)
I don't really expect people to use --dmd-path with runtests.py, but I need a good
way of passing in an appropriate option via mach.  This is as good as any.
Attachment #831716 - Flags: review?(jmaher)
And finally the piece that makes everything Just Work.
Attachment #831717 - Flags: review?(gps)
Comment on attachment 831715 [details] [diff] [review]
part 1 - add automation support for adding DMD environment variables

Nick should probably look at this too, just to confirm I got all the DMD magic right.
Attachment #831715 - Flags: review?(n.nethercote)
Comment on attachment 831715 [details] [diff] [review]
part 1 - add automation support for adding DMD environment variables

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

straightforward and solves a problem.  Are there needs for Android or b2g which are not addressed here?
Attachment #831715 - Flags: review?(jmaher) → review+
Comment on attachment 831716 [details] [diff] [review]
part 2 - add mochitest support for specifying DMD paths

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

all of this looks good, nice and simple.
Attachment #831716 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #5)
> straightforward and solves a problem.  Are there needs for Android or b2g
> which are not addressed here?

There probably are.  I don't have enough experience with the process for running those to offer an opinion or patch at the moment.
Comment on attachment 831717 [details] [diff] [review]
part 3 - add mach support for running DMD during mochitests

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

You don't need my review on mach commands for testing pieces - members of A*Team will gladly handle this review. Remember, mach commands belong mostly to the module they interact with. The individual mach_commands.py files living next to the code they interact with enforces this.
Attachment #831717 - Flags: review?(gps) → review+
Comment on attachment 831715 [details] [diff] [review]
part 1 - add automation support for adding DMD environment variables

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

I just read the instructions at https://wiki.mozilla.org/Performance/MemShrink/DMD to check this because I can't actually remember how to do it!

::: build/automationutils.py
@@ +422,2 @@
>    elif mozinfo.isMac:
>      envVar = "DYLD_LIBRARY_PATH"

According to the DMD docs you need to modify LD_LIBRARY_PATH just like Linux, but maybe DYLD_LIBRARY_PATH works as well?  Have you tried this on Mac?
Attachment #831715 - Flags: review?(n.nethercote) → review+
(In reply to Gregory Szorc [:gps] from comment #8)
> You don't need my review on mach commands for testing pieces - members of
> A*Team will gladly handle this review. Remember, mach commands belong mostly
> to the module they interact with. The individual mach_commands.py files
> living next to the code they interact with enforces this.

Doh, sorry about that!  Lesson learned.

(In reply to Nicholas Nethercote [:njn] from comment #9)
> ::: build/automationutils.py
> @@ +422,2 @@
> >    elif mozinfo.isMac:
> >      envVar = "DYLD_LIBRARY_PATH"
> 
> According to the DMD docs you need to modify LD_LIBRARY_PATH just like
> Linux, but maybe DYLD_LIBRARY_PATH works as well?  Have you tried this on
> Mac?

I haven't tried it on a Mac, but I've never seen anything that does dynamic loading on the Mac use LD_LIBRARY_PATH; it's always been DYLD_LIBRARY_PATH.  I can check before landing this, I suppose.
> I haven't tried it on a Mac, but I've never seen anything that does dynamic
> loading on the Mac use LD_LIBRARY_PATH; it's always been DYLD_LIBRARY_PATH. 
> I can check before landing this, I suppose.

The bash alias I use to invoke DMD-enabled builds uses LD_LIBRARY_PATH on both Mac and Linux and always has...
(In reply to Nicholas Nethercote [:njn] from comment #11)
> > I haven't tried it on a Mac, but I've never seen anything that does dynamic
> > loading on the Mac use LD_LIBRARY_PATH; it's always been DYLD_LIBRARY_PATH. 
> > I can check before landing this, I suppose.
> 
> The bash alias I use to invoke DMD-enabled builds uses LD_LIBRARY_PATH on
> both Mac and Linux and always has...

Huh.  The Mac dynamic linker supports both:

http://opensource.apple.com/source/dyld/dyld-210.2.3/src/dyld.cpp

Wonder when that code got introduced.

I'll change it to use LD_LIBRARY_PATH, I guess.
Blocks: MochiMem
(In reply to Joel Maher (:jmaher) from comment #5)
> straightforward and solves a problem.  Are there needs for Android or b2g
> which are not addressed here?

Turns out that there are a number of changes required to support Android, since non-remote and remote mochitests have slightly different codepaths for modifying the environment.  The main problem right now is that there's no support in automation.py.in or automationutils.py for deciding whether the target is Android.  (Android has a slightly different way of preloading libraries, closer to what Windows does.)

I *think* the automation.py.in changes are simple enough (adding another #expand directive, assuming that the necessary symbols are already defined...).  But the automationutils.py changes would require updates to mozinfo, which look less than trivial.  Especially since mozinfo looks like it only provides reliable information about the host machine for the tests, not the target machine.  Joel, you mentioned this morning in #ateam that Android was blocking things; is this part of the blockage, or is it something completely different?  (Or am I totally off track?)
Flags: needinfo?(jmaher)
Depends on: 939914
in general you could overload function from automation.py.in in remoteautomation.py:
http://mxr.mozilla.org/mozilla-central/source/build/mobile/remoteautomation.py

otherwise it looks like you need to somehow gain access to the .so required and push it to the device.  We do this for profile creation by using the general profile and then tweaking it as needed and pushing:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtestsremote.py#336

I suspect this would solve your main problems.  Right now android tests use automation.py.in and desktop uses mozprocess/mozprofile.  Not an ideal situation- a day of work and a few patches could get most of that confusion removed.
Flags: needinfo?(jmaher)
OK, I think I got all the places that need to be touched for android
tests to be happy about this.  Needs a try run, though.
Attachment #831715 - Attachment is obsolete: true
Attachment #8334780 - Flags: review?(jmaher)
The only change here is to push libdmd.so onto the device and fixup
where we expect to find libdmd.
Attachment #831716 - Attachment is obsolete: true
Attachment #8334782 - Flags: review?(jmaher)
Comment on attachment 8334780 [details] [diff] [review]
part 1 - add automation support for adding DMD environment variables

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

just one concern, although I assume you have taken care of this.

::: build/mobile/remoteautomation.py
@@ +55,5 @@
>              env = {}
>  
> +        if dmdPath:
> +            env['DMD'] = '1'
> +            env['MOZ_REPLACE_MALLOC_LIB'] = os.path.join(dmdPath, 'libdmd.so')

this is in the remote wrapper library, is dmdpath the path on the device?
Attachment #8334780 - Flags: review?(jmaher) → review+
Comment on attachment 8334782 [details] [diff] [review]
part 2 - add mochitest support for specifying DMD paths

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

::: testing/mochitest/runtestsremote.py
@@ +585,5 @@
> +        dmdLibrary = "libdmd.so"
> +        dmdPathOnDevice = os.path.join(deviceRoot, dmdLibrary)
> +        dm.removeFile(dmdPathOnDevice)
> +        dm.pushFile(os.path.join(options.dmdPath, dmdLibrary), dmdPathOnDevice)
> +        options.dmdPath = deviceRoot

great!
Attachment #8334782 - Flags: review?(jmaher) → review+
Assignee: nobody → nfroyd
You need to log in before you can comment on or make changes to this bug.