We need to update our xpcshell-tests for the new OSX v2 bundle structure. This applies to both the selftest in 'make check' as well as the actual xpcshell test harness.

I started a build on Oak to see where this gets us. This should at least get us past the selftest during 'make check'.

The previous patch fixed the xpcshell selftest during 'make check', but the actual xpcshell-tests still needed some tweaking. This should fix this as well. Kicked off a new test run on oak: https://tbpl.mozilla.org/?tree=Oak&rev=c91ee24525b9

More test fixes.

Forgot hg qrefresh.

Fixed b2g desktop OSX selftest.py failures during 'make check'.

Comment on attachment 8487475 [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 on attachment 8487475 [details] [diff] [review] Patch Review of attachment 8487475 [details] [diff] [review]: ----------------------------------------------------------------- There are a couple of changes in here that I don't think we need (selftest.py, base.py), so I'm r-ing. Other than that it looks good, although I think we could stand to do things in a nicer way. I'm fine with punting niceties to a followup bug though. ::: js/xpconnect/src/XPCShellImpl.cpp @@ +1367,5 @@ > + greDir->AppendNative(NS_LITERAL_CSTRING("Resources")); > + bool dirExists = false; > + greDir->Exists(&dirExists); > + if (dirExists) { > + dirprovider.SetGREDir(greDir); This could stand a comment explaining why this is so. ::: netwerk/test/unit/test_socks.js @@ +33,2 @@ > var ds = new DirectoryService(); > + var bin = ds.get("XREExeF", Ci.nsILocalFile); This change actually makes the tests nicer, good work! ::: python/mozbuild/mozbuild/base.py @@ +274,5 @@ > > @property > def bindir(self): > + if sys.platform.startswith('darwin'): > + return os.path.join(self.topobjdir, 'dist', self.substs['MOZ_MACBUNDLE_NAME'], 'Contents', 'MacOS') This seems pretty invasive, and I think it should be out of scope. This only has impact on people running tests locally, where the GateKeeper stuff doesn't really matter anyway, right? ::: security/manager/ssl/tests/unit/head_psm.js @@ +362,5 @@ > .getService(Ci.nsIEnvironment); > let greDir = directoryService.get("GreD", Ci.nsIFile); > + let macOSDir = greDir.parent; > + macOSDir.append("MacOS"); > + envSvc.set("DYLD_LIBRARY_PATH", macOSDir.path); It feels like maybe we'd be better served by adding another directory service location here, like "GreLibrariesD", that handed out this path, so then you could just fix callers to use that. Is there a reason you didn't go that route? Making all these callers do path manipulation like this is not fantastic. ::: testing/xpcshell/runxpcshelltests.py @@ +845,5 @@ > + # Check if we're run from an OSX app bundle and override > + # self.xrePath if we are. > + appBundlePath = os.path.join(os.path.dirname(os.path.dirname(self.xpcshell)), 'Resources') > + if os.path.exists(os.path.join(appBundlePath, 'application.ini')): > + self.xrePath = appBundlePath This is gross, but since we're not currently passing an xrePath in on the commandline I guess there's not much we can do. @@ +882,5 @@ > elif sys.platform in ('os2emx', 'os2knix'): > os.environ["BEGINLIBPATH"] = self.xrePath + ";" + self.env["BEGINLIBPATH"] > os.environ["LIBPATHSTRICT"] = "T" > elif sys.platform == 'osx' or sys.platform == "darwin": > + self.env["DYLD_LIBRARY_PATH"] = os.path.join(os.path.dirname(self.xrePath), 'MacOS') Similar to what I mentioned elsewhere, I wonder if it would make sense to just have the concept of an xreLibraryPath in our test harnesses, which would == xrePath on other platforms, but could be set specially on mac in a central location. ::: testing/xpcshell/selftest.py @@ +20,5 @@ > mozinfo.find_and_update_from_json() > > objdir = build_obj.topobjdir.encode("utf-8") > + > +if sys.platform.startswith('darwin'): mozinfo.os == 'mac': @@ +22,5 @@ > objdir = build_obj.topobjdir.encode("utf-8") > + > +if sys.platform.startswith('darwin'): > + from buildconfig import substs > + xpcshellBin = os.path.join(objdir, "dist", substs['MOZ_MACBUNDLE_NAME'], "Contents", "MacOS", "xpcshell") I don't think you should need this change. These tests only ever run on build machines or developer machines. Will running xpcshell from dist/bin no longer work after your changes?

xpcshell-tests are green on try with this patch (ignoring the update specific test failures, as those are handled in separate bugs). Still looking into Cpp failures: https://tbpl.mozilla.org/?tree=Try&rev=ec6821ee0c47 (In reply to Ted Mielczarek [:ted.mielczarek] from comment #8) > Comment on attachment 8487475 [details] [diff] [review] > Patch > > Review of attachment 8487475 [details] [diff] [review]: > ----------------------------------------------------------------- > > There are a couple of changes in here that I don't think we need > (selftest.py, base.py), so I'm r-ing. Other than that it looks good, > although I think we could stand to do things in a nicer way. I'm fine with > punting niceties to a followup bug though. > > ::: js/xpconnect/src/XPCShellImpl.cpp > @@ +1367,5 @@ > > + greDir->AppendNative(NS_LITERAL_CSTRING("Resources")); > > + bool dirExists = false; > > + greDir->Exists(&dirExists); > > + if (dirExists) { > > + dirprovider.SetGREDir(greDir); > > This could stand a comment explaining why this is so. Added a comment and cleaned this up a bit. > ::: python/mozbuild/mozbuild/base.py > @@ +274,5 @@ > > > > @property > > def bindir(self): > > + if sys.platform.startswith('darwin'): > > + return os.path.join(self.topobjdir, 'dist', self.substs['MOZ_MACBUNDLE_NAME'], 'Contents', 'MacOS') > > This seems pretty invasive, and I think it should be out of scope. This only > has impact on people running tests locally, where the GateKeeper stuff > doesn't really matter anyway, right? After our conversation on IRC I've changed this to point to Contents/Resources. If we need the path to Contents/MacOS, we can use os.path.dirname(get_binary_path()) instead, when available. > ::: security/manager/ssl/tests/unit/head_psm.js > @@ +362,5 @@ > > .getService(Ci.nsIEnvironment); > > let greDir = directoryService.get("GreD", Ci.nsIFile); > > + let macOSDir = greDir.parent; > > + macOSDir.append("MacOS"); > > + envSvc.set("DYLD_LIBRARY_PATH", macOSDir.path); > > It feels like maybe we'd be better served by adding another directory > service location here, like "GreLibrariesD", that handed out this path, so > then you could just fix callers to use that. Is there a reason you didn't go > that route? Making all these callers do path manipulation like this is not > fantastic. I shied away from it because this would be an OSX-only directory service location, but I'm fine going that route if that's better. > ::: testing/xpcshell/selftest.py > @@ +20,5 @@ > > mozinfo.find_and_update_from_json() > > > > objdir = build_obj.topobjdir.encode("utf-8") > > + > > +if sys.platform.startswith('darwin'): > > mozinfo.os == 'mac': Corrected. > @@ +22,5 @@ > > objdir = build_obj.topobjdir.encode("utf-8") > > + > > +if sys.platform.startswith('darwin'): > > + from buildconfig import substs > > + xpcshellBin = os.path.join(objdir, "dist", substs['MOZ_MACBUNDLE_NAME'], "Contents", "MacOS", "xpcshell") > > I don't think you should need this change. These tests only ever run on > build machines or developer machines. Will running xpcshell from dist/bin no > longer work after your changes? Right, dist/bin will no longer work.

Comment on attachment 8495941 [details] [diff] [review] Patch Review of attachment 8495941 [details] [diff] [review]: ----------------------------------------------------------------- please confirm/answer my questions below prior to a r+ ::: netwerk/test/unit/test_socks.js @@ +33,2 @@ > var ds = new DirectoryService(); > + var bin = ds.get("XREExeF", Ci.nsILocalFile); Where is XreExeF set? ::: security/manager/ssl/tests/unit/head_psm.js @@ +362,5 @@ > .getService(Ci.nsIEnvironment); > let greDir = directoryService.get("GreD", Ci.nsIFile); > + let macOSDir = greDir.parent; > + macOSDir.append("MacOS"); > + envSvc.set("DYLD_LIBRARY_PATH", macOSDir.path); and this will work on all flavors of OSX? ::: testing/xpcshell/runxpcshelltests.py @@ +844,5 @@ > self.xrePath = os.path.dirname(self.xpcshell) > + if mozinfo.isMac: > + # Check if we're run from an OSX app bundle and override > + # self.xrePath if we are. > + appBundlePath = os.path.join(os.path.dirname(os.path.dirname(self.xpcshell)), 'Resources') can we assume the app bundle is ..\..\resources? I believe so, but please confirm. @@ +883,5 @@ > elif sys.platform in ('os2emx', 'os2knix'): > os.environ["BEGINLIBPATH"] = self.xrePath + ";" + self.env["BEGINLIBPATH"] > os.environ["LIBPATHSTRICT"] = "T" > elif sys.platform == 'osx' or sys.platform == "darwin": > + self.env["DYLD_LIBRARY_PATH"] = os.path.join(os.path.dirname(self.xrePath), 'MacOS') and this works on 10.6, 10.8 and 10.10? ::: xpcom/tests/unit/head_xpcom.js @@ +17,5 @@ > getService(Components.interfaces.nsIProperties); > var greDir = dirSvc.get("GreD", Components.interfaces.nsIFile); > + var macOSDir = greDir.parent; > + macOSDir.append("MacOS"); > + envSvc.set("DYLD_LIBRARY_PATH", macOSDir.path); this seems specific to osx, what about windows/linux?

(In reply to Joel Maher (:jmaher) from comment #10) > ::: netwerk/test/unit/test_socks.js > @@ +33,2 @@ > > var ds = new DirectoryService(); > > + var bin = ds.get("XREExeF", Ci.nsILocalFile); > > Where is XreExeF set? http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#322 > ::: security/manager/ssl/tests/unit/head_psm.js > @@ +362,5 @@ > > .getService(Ci.nsIEnvironment); > > let greDir = directoryService.get("GreD", Ci.nsIFile); > > + let macOSDir = greDir.parent; > > + macOSDir.append("MacOS"); > > + envSvc.set("DYLD_LIBRARY_PATH", macOSDir.path); > > and this will work on all flavors of OSX? Yes. Setting the DYLD_LIBRARY_PATH is used all over the tree, including as a workaround for plugin-container.app for example. If we didn't set the library load path for plugin-container.app to Firefox.app/Contents/MacOS, it would need to carry its own copy of libmozglue, libmozalloc, XUL etc. > ::: testing/xpcshell/runxpcshelltests.py > @@ +844,5 @@ > > self.xrePath = os.path.dirname(self.xpcshell) > > + if mozinfo.isMac: > > + # Check if we're run from an OSX app bundle and override > > + # self.xrePath if we are. > > + appBundlePath = os.path.join(os.path.dirname(os.path.dirname(self.xpcshell)), 'Resources') > > can we assume the app bundle is ..\..\resources? I believe so, but please > confirm. So, the xpcshell binary is at: <bundleName>.app/Contents/MacOS/xpcshell We do a couple os.path.dirname to get to: <bundleName>.app/Contents Now, we join this path with 'Resources' to get to: <bundleName>.app/Contents/Resources If this directory exists, we want it to become our xrePath since we're run from a .app bundle. Does this answer your question? > @@ +883,5 @@ > > elif sys.platform in ('os2emx', 'os2knix'): > > os.environ["BEGINLIBPATH"] = self.xrePath + ";" + self.env["BEGINLIBPATH"] > > os.environ["LIBPATHSTRICT"] = "T" > > elif sys.platform == 'osx' or sys.platform == "darwin": > > + self.env["DYLD_LIBRARY_PATH"] = os.path.join(os.path.dirname(self.xrePath), 'MacOS') > > and this works on 10.6, 10.8 and 10.10? It does, yes. > ::: xpcom/tests/unit/head_xpcom.js > @@ +17,5 @@ > > getService(Components.interfaces.nsIProperties); > > var greDir = dirSvc.get("GreD", Components.interfaces.nsIFile); > > + var macOSDir = greDir.parent; > > + macOSDir.append("MacOS"); > > + envSvc.set("DYLD_LIBRARY_PATH", macOSDir.path); > > this seems specific to osx, what about windows/linux? DYLD_LIBRARY_PATH will only be picked up by osx. Windows ignores it and linux uses LD_LIBRARY_PATH instead. We could ifdef this, but it seems unnecessary under these circumstances. Let me know if you'd like me to ifdef this anyway.

thanks for clarifying my questions- all looks good.

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

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

Pushed to fx-team https://hg.mozilla.org/integration/fx-team/rev/33000c22f91f

Stephen, this patch will need to be updated for aurora

Now with proper hg qrefresh

Comment on attachment 8497271 [details] [diff] [review] Patch for Aurora If we're uplifting bug 928397 at the same time as this one, this patch will be obsolete and the regular patch for m-c should be used to uplift to aurora.

Comment on attachment 8497271 [details] [diff] [review] Patch for Aurora We will be uplifting bug 928397, so obsoleting this patch.

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