1064952 - Update Cpp unit tests for new OSX v2 bundle structure

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Stephen A Pohl [:spohl]]

We need to update our Cpp unit tests for the new OSX v2 bundle structure.

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 2

[Stephen A Pohl [:spohl]]

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

Comment 3

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/projects/oak/rev/641355770161

Comment 4

[(not currently active) Ted Mielczarek]

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

Comment 6

[Stephen A Pohl [:spohl]]

Attachment: Patch

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).

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Patch

Updated commit message to r=jmaher.

Comment 8

[Stephen A Pohl [:spohl]]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=e131c2875bfa

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

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])

Comment 10

[Stephen A Pohl [:spohl]]

Attachment: Patch

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.

Comment 11

[Stephen A Pohl [:spohl]]

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

Comment 12

[Stephen A Pohl [:spohl]]

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.

Comment 13

[Stephen A Pohl [:spohl]]

New try push and things are looking green so far:
https://tbpl.mozilla.org/?tree=Try&rev=24416e6484ca

Comment 14

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

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

Comment 15

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

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/5af85a6db219

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5af85a6db219

Comment 17

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

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