Closed Bug 1064952 Opened 6 years ago Closed 6 years ago

Update Cpp unit tests for new OSX v2 bundle structure

Categories

(Testing :: General, 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, 3 obsolete files)

We need to update our Cpp unit tests for the new OSX v2 bundle structure.
Attached patch Patch (obsolete) — Splinter Review
Comment on attachment 8486539 [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 #8486539 - Flags: review?(ted)
Comment on attachment 8486539 [details] [diff] [review]
Patch

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

::: startupcache/test/TestStartupCache.cpp
@@ +418,5 @@
> +  if (NS_SUCCEEDED(scrv)) {
> +    nsCOMPtr<nsIFile> parent;
> +    manifest->GetParent(getter_AddRefs(parent));
> +    parent->AppendNative(NS_LITERAL_CSTRING("MacOS"));
> +    manifest = parent.forget();

I don't think this should be necessary if we continue to run the tests from dist/bin or the test package.

::: testing/mach_commands.py
@@ +273,5 @@
>  
>          tester = cppunittests.CPPUnitTests()
> +        xre_path = self.bindir
> +        if sys.platform == 'osx' or sys.platform == "darwin":
> +            xre_path = os.path.join(os.path.dirname(xre_path), 'Resources')

I think you're running yourself in circles here. dist/bin should work fine here, but since in that other patch you changed MozbuildObject.bindir to return something other than dist/bin now you have to work around it here.

::: testing/runcppunittests.py
@@ +101,4 @@
>          if mozinfo.os == "linux":
>              pathvar = "LD_LIBRARY_PATH"
>          elif mozinfo.os == "mac":
> +            libpath = os.path.join(os.path.dirname(libpath), 'MacOS')

Why are you manipulating xre_path in two places in this file? You're doing it in main as well as here. There are only two scenarios for this file:
1) Run from mach_commands.py, the path that's passed in here should work fine
2) Run on TBPL via mozharness, the dir will be passed in to --xre-path.

I actually think it might be simpler to just fix the argument that mozharness passes in, that's here:
http://dxr.mozilla.org/mozilla-central/source/testing/config/mozharness/mac_config.py#30
Attachment #8486539 - Flags: review?(ted) → review-
Duplicate of this bug: 1073009
Attached patch Patch (obsolete) — Splinter Review
Will kick off a try run shortly, but this passes both ./mach cppunittest and running via mozharness locally.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> ::: startupcache/test/TestStartupCache.cpp
> @@ +418,5 @@
> > +  if (NS_SUCCEEDED(scrv)) {
> > +    nsCOMPtr<nsIFile> parent;
> > +    manifest->GetParent(getter_AddRefs(parent));
> > +    parent->AppendNative(NS_LITERAL_CSTRING("MacOS"));
> > +    manifest = parent.forget();
> 
> I don't think this should be necessary if we continue to run the tests from
> dist/bin or the test package.

As discussed on IRC with Ted, running tests out of dist/bin will no longer work. XPCOM had to change the way it loads libraries it depends on. It used to simply look for dependentlibs.list in the same directory as the actual libraries (Contents/MacOS or dist/bin). Now, it will read the dependentlibs.list out of Contents/Resources, then load the libraries from a relative path of ../MacOS. in the case of dist/bin, this would have forced us to either create a symlink of dist/MacOS -> dist/bin or change client code to detect when we're run as part of unittests and load libraries out of the same directory as dependentlibs.list. Both of these solutions seemed much uglier than simply running the tests out of the .app bundle where the folder structure already had the proper form.

> ::: testing/mach_commands.py
> @@ +273,5 @@
> >  
> >          tester = cppunittests.CPPUnitTests()
> > +        xre_path = self.bindir
> > +        if sys.platform == 'osx' or sys.platform == "darwin":
> > +            xre_path = os.path.join(os.path.dirname(xre_path), 'Resources')
> 
> I think you're running yourself in circles here. dist/bin should work fine
> here, but since in that other patch you changed MozbuildObject.bindir to
> return something other than dist/bin now you have to work around it here.

Removed this as it was no longer necessary. self.bindir correctly points at Contents/Resources already due to changes to base.py in bug 1060562.

> ::: testing/runcppunittests.py
> @@ +101,4 @@
> >          if mozinfo.os == "linux":
> >              pathvar = "LD_LIBRARY_PATH"
> >          elif mozinfo.os == "mac":
> > +            libpath = os.path.join(os.path.dirname(libpath), 'MacOS')
> 
> Why are you manipulating xre_path in two places in this file? You're doing
> it in main as well as here. There are only two scenarios for this file:
> 1) Run from mach_commands.py, the path that's passed in here should work fine
> 2) Run on TBPL via mozharness, the dir will be passed in to --xre-path.

The first manipulation is to properly set xre_path to Contents/Resources. The second manipulation here is just to get the proper library path, which should point to Contents/MacOS (where the libraries reside).
Attachment #8486539 - Attachment is obsolete: true
Attachment #8496227 - Flags: review?(jmaher)
Attached patch PatchSplinter Review
Updated commit message to r=jmaher.
Attachment #8496227 - Attachment is obsolete: true
Attachment #8496227 - Flags: review?(jmaher)
Attachment #8496238 - Flags: review?(jmaher)
Comment on attachment 8496238 [details] [diff] [review]
Patch

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

only one comment

::: testing/runcppunittests.py
@@ +110,5 @@
>          elif mozinfo.os == "win":
>              pathvar = "PATH"
>          if pathvar:
>              if pathvar in env:
> +                env[pathvar] = "%s%s%s" % (libpath, os.pathsep, env[pathvar])

why not os.path.join(libpath, env[pathvar])
Attachment #8496238 - Flags: review?(jmaher) → review+
Attached patch Patch (obsolete) — Splinter Review
Thank you for the fast review! Addressed feedback. Carrying over r+.

(In reply to Joel Maher (:jmaher) from comment #9)
> ::: testing/runcppunittests.py
> @@ +110,5 @@
> >          elif mozinfo.os == "win":
> >              pathvar = "PATH"
> >          if pathvar:
> >              if pathvar in env:
> > +                env[pathvar] = "%s%s%s" % (libpath, os.pathsep, env[pathvar])
> 
> why not os.path.join(libpath, env[pathvar])

Good question. This was existing code. Changed it to os.path.join.
Attachment #8496238 - Attachment is obsolete: true
Attachment #8496420 - Flags: review+
Looks like OSX had trouble getting signed on the build servers yesterday. Trying again with a new push and updated patch:
https://tbpl.mozilla.org/?tree=Try&rev=a1c56fec3936
Ah, we didn't use os.path.join here because it's seemingly doing the wrong thing on Windows (and the section of code in question applies to all platforms). It's probably using '/' instead of '\' as path separator on Windows. See the Cpp failures on Windows in this try push:
https://tbpl.mozilla.org/?tree=Try&rev=62d4cc9ce99c

Will obsolete the patch with os.path.join and revert to the previous one.
Attachment #8496420 - Attachment is obsolete: true
Attachment #8496238 - Attachment is obsolete: false
New try push and things are looking green so far:
https://tbpl.mozilla.org/?tree=Try&rev=24416e6484ca
https://hg.mozilla.org/mozilla-central/rev/5af85a6db219
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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.