1060652 - In the OAK build, running mochitest using the .dmg file and tests does not work because of missing dylibs

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Syd Polk :sydpolk]

1. Download firefox-firefox-33.0a1.en-US.mac.dmg and firefox-33.0a1.en-US.mac.tests.zip.
2. Mount the dmg.
3. Copy /Volumes/Nightly/FirefoxNightly.app to your drive. I copied it to ~/firefoxes.
4. mkdir tests
5. unzip firefox-33.0a1.en-US.mac.tests.zip
6. cd ..
7. Run the setup instructions in https://etherpad.mozilla.org/running-out-of-tests-zip (but don't run the runtests.py script yet)
8. From your vitualenv, run the following:

(mochitest)Mountain-Lion-Xcode-6:~ jazzman$ python tests/mochitest/runtests.py --total-chunks 5 --this-chunk 1 --appname=~/firefoxes/FirefoxNightly.app/Contents/MacOS/firefox --utility-path=tests/bin --extra-profile-file=tests/bin/plugins --certificate-path=tests/certs --autorun --close-when-done --console-level=INFO --quiet --log-file=raw.log --chunk-by-dir=4 > /tmp/log

You will get spew, but the last of the log says:

dyld: Library not loaded: @executable_path/libnss3.dylib
  Referenced from: /Users/jazzman/tests/bin/certutil
  Reason: image not found
TEST-UNEXPECTED-FAIL | runtests.py | Certificate integration failed
(END) 

To workaround, do:

export DYLD_LIBRARY_PATH=~/firefoxes/FirefoxNightly.app/Contents/MacOS

Run the command again. The tests proceed.

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks, Syd! All your work helped me come up with this patch. I kicked off a test run on Oak, which should tell us how we're looking. More work may still be necessary, but this should fix the immediate issue with the library load path:

https://tbpl.mozilla.org/?tree=Oak&rev=c91ee24525b9

Comment 2

[Syd Polk :sydpolk]

I actually don't agree with the approach of this patch. The necessary dylibs are included in the tests directory, where the xpcshell binaries are. If you have DYLD_LIBRARY_PATH there, then you can run these tests on future versions of Firefox if those versions change dylibs in the future.

Comment 3

[Syd Polk :sydpolk]

However, those dylibs are scattered all of the place in the tests directory. Somebody should clean this up:

sydpolkzillambp:tests spolk$ find . -name \*.dylib
./bin/plugins/JavaTest.plugin/Contents/MacOS/libnptestjava.dylib
./bin/plugins/SecondTest.plugin/Contents/MacOS/libnpsecondtest.dylib
./bin/plugins/Test.plugin/Contents/MacOS/libnptest.dylib
./mochitest/chrome/toolkit/components/ctypes/tests/chrome/libjsctypes-test.dylib
./xpcshell/tests/js/xpconnect/tests/components/native/libxpctest.dylib
./xpcshell/tests/modules/libmar/tests/unit/libnss3.dylib
./xpcshell/tests/toolkit/components/ctypes/tests/unit/libjsctypes-test.dylib
./xpcshell/tests/toolkit/crashreporter/test/unit/libtestcrasher.dylib
./xpcshell/tests/toolkit/crashreporter/test/unit_ipc/libtestcrasher.dylib
./xpcshell/tests/xpcom/tests/unit/libtest656331.dylib
./xpcshell/tests/xpcom/tests/unit/libtestcompnoaslr.dylib
./xpcshell/tests/xpcom/tests/unit/libtestcomponent.dylib
sydpolkzillambp:tests spolk$

Comment 4

[Stephen A Pohl [:spohl]]

The xpcshell binary is copied into the .app bundle before it's executed. So, this patch actually points the library load path to the dylibs sitting next to the xpcshell binary, which is most likely what we want and prefer.

Comment 5

[Stephen A Pohl [:spohl]]

Comment on attachment 8481890 [details] [diff] [review]
Patch

This turned out to be all we needed for mochitest. The remaining failures were due to a mistake in the patch for bug 1055774. Mochitest is now green:

https://tbpl.mozilla.org/?tree=Oak&rev=797d19721fee

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Patch

This also fixes the mochitest harness when run locally via mach.

Comment 8

[Stephen A Pohl [:spohl]]

Comment on attachment 8486544 [details] [diff] [review]
Patch

Try push is almost completely green, so requesting review (updater xpcshell test failures and gaia python integration test suite failures are handled in separate bugs):
https://tbpl.mozilla.org/?tree=Try&rev=7362867ad903

Comment 9

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/2aacb42e3dc5

Comment 10

[(not currently active) Ted Mielczarek]

(In reply to Syd Polk :sydpolk from comment #3)
> However, those dylibs are scattered all of the place in the tests directory.
> Somebody should clean this up:

This isn't a thing to be cleaned up, FWIW, these are individual tests that use shared libraries as part of the test.  (Or for the first few entries there, the test plugins we use.)

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 8486544 [details] [diff] [review]
Patch

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

I'm a little worried about scattering these path manipulations all over the tree. I think that's going to make it very hard to keep things from breaking in the future. I'm not r+ing this because I have a few issues with it, but I don't think there's anything insurmountable.

::: browser/app/macbuild/Contents/MacOS-files.in
@@ +4,3 @@
>  /firefox-bin
> +/pk12util
> +/ssltunnel

This is OK as a short-term fix, but the harness already supports running these files from a different directory, so it feels like we should just be using that instead. If you just made the mach command point its utilityPath at dist/bin instead of the MacOS dir, would that work?

::: testing/mochitest/runtests.py
@@ +293,5 @@
>  
>      # get testing environment
>      env = environment(xrePath=self._xrePath)
>      env["XPCOM_DEBUG_BREAK"] = "warn"
> +    env["DYLD_LIBRARY_PATH"] = os.path.join(os.path.dirname(self._xrePath), 'MacOS')

I think it'd make sense to push this down into the environment function. (Looks like you could drag the LD_LIBRARY_PATH bit with it, since all the callers seem to set that as well.) That would at least make it so you only set it once.

@@ +677,5 @@
>      # on the command line to select a particular version of httpd.js. If not
>      # specified, try to select the one from hostutils.zip, as required in bug 882932.
>      if not options.httpdPath:
>        options.httpdPath = os.path.join(options.utilityPath, "components")
> +      if not os.path.exists(options.httpdPath) and (sys.platform == 'osx' or sys.platform == "darwin"):

Use mozinfo.os == 'mac' here, please. Do we really hit this case in practice? This should only happen in packaged builds, and httpd.js should wind up in the test package's components dir because of the packaging bits here:
http://hg.mozilla.org/mozilla-central/annotate/5e704397529b/testing/xpcshell/Makefile.in#l48

Comment 12

[Stephen A Pohl [:spohl]]

Attachment: Patch

Mochitests are turning green on try with this patch:
https://tbpl.mozilla.org/?tree=Try&rev=55e45bb6a000

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11)
> ::: browser/app/macbuild/Contents/MacOS-files.in
> @@ +4,3 @@
> >  /firefox-bin
> > +/pk12util
> > +/ssltunnel
> 
> This is OK as a short-term fix, but the harness already supports running
> these files from a different directory, so it feels like we should just be
> using that instead. If you just made the mach command point its utilityPath
> at dist/bin instead of the MacOS dir, would that work?

Discussed this on IRC and it's my understanding that this is okay now, since these files won't be packaged into the final bundle.

> ::: testing/mochitest/runtests.py
> @@ +293,5 @@
> >  
> >      # get testing environment
> >      env = environment(xrePath=self._xrePath)
> >      env["XPCOM_DEBUG_BREAK"] = "warn"
> > +    env["DYLD_LIBRARY_PATH"] = os.path.join(os.path.dirname(self._xrePath), 'MacOS')
> 
> I think it'd make sense to push this down into the environment function.
> (Looks like you could drag the LD_LIBRARY_PATH bit with it, since all the
> callers seem to set that as well.) That would at least make it so you only
> set it once.

Moved this to automationutils.py. Interestingly, automationutils.py should already do the right thing for LD_LIBRARY_PATH, so I'm not sure why this is set here too. Would need to investigate before moving LD_LIBRARY_PATH, but this shouldn't stop this patch.
 
> @@ +677,5 @@
> >      # on the command line to select a particular version of httpd.js. If not
> >      # specified, try to select the one from hostutils.zip, as required in bug 882932.
> >      if not options.httpdPath:
> >        options.httpdPath = os.path.join(options.utilityPath, "components")
> > +      if not os.path.exists(options.httpdPath) and (sys.platform == 'osx' or sys.platform == "darwin"):
> 
> Use mozinfo.os == 'mac' here, please.

Is mozinfo.isMac okay?

> Do we really hit this case in
> practice? This should only happen in packaged builds, and httpd.js should
> wind up in the test package's components dir because of the packaging bits
> here:
> http://hg.mozilla.org/mozilla-central/annotate/5e704397529b/testing/xpcshell/
> Makefile.in#l48

You're right. Removed.

Comment 13

[Stephen A Pohl [:spohl]]

Attachment: Patch

Updated commit message with r=jmaher. Carrying over r+.

Comment 14

[Robert Strong (they/them - no direct email)]

Backed out old patch and relanded on oak
https://hg.mozilla.org/projects/oak/rev/dbf4069f434a

Comment 15

[Robert Strong (they/them - no direct email)]

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/427a562e16d4

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/427a562e16d4

Comment 17

[Robert Strong (they/them - no direct email)]

Landed on aurora in the Mac V2 signing combined patch in bug 1047584