Closed Bug 1060562 Opened 5 years ago Closed 5 years ago

Update xpcshell-tests for new OSX v2 bundle structure

Categories

(Testing :: XPCShell Harness, defect)

x86
macOS
defect
Not set

Tracking

(firefox34 fixed, firefox35 fixed)

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: spohl, Assigned: spohl)

References

Details

Attachments

(1 file, 8 obsolete files)

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.
Attached patch Patch (wip) (obsolete) — Splinter Review
I started a build on Oak to see where this gets us. This should at least get us past the selftest during 'make check'.
Attached patch Patch (wip) (obsolete) — Splinter Review
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
Attachment #8481561 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
More test fixes.
Attachment #8481889 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Forgot hg qrefresh.
Attachment #8486546 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Fixed b2g desktop OSX selftest.py failures during 'make check'.
Attachment #8486550 - Attachment is obsolete: true
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
Attachment #8487475 - Flags: review?(ted)
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?
Attachment #8487475 - Flags: review?(ted) → review-
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8487475 - Attachment is obsolete: true
Attachment #8495941 - Flags: review?(ted)
Attachment #8495941 - Flags: review?(ted) → review?(jmaher)
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?
Attachment #8495941 - Flags: review?(jmaher) → review-
(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.
Flags: needinfo?(jmaher)
Attachment #8495941 - Flags: review- → review+
thanks for clarifying my questions- all looks good.
Flags: needinfo?(jmaher)
Attached patch PatchSplinter Review
Updated commit message with r=jmaher. Carrying over r+.
Attachment #8495941 - Attachment is obsolete: true
Attachment #8496039 - Flags: review+
Stephen, this patch will need to be updated for aurora
Attached patch Patch for Aurora (obsolete) — Splinter Review
Attached patch Patch for Aurora (obsolete) — Splinter Review
Now with proper hg qrefresh
Attachment #8497270 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/33000c22f91f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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.
Attachment #8497271 - Attachment is obsolete: true
Landed on aurora in the Mac V2 signing combined patch in bug 1047584
You need to log in before you can comment on or make changes to this bug.