1247630 - Try OSX SM(arm) builds are busted

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/logviewer.html#?job_id=16612122&repo=try

 Undefined symbols for architecture i386:
   "_CFRelease", referenced from:
       _PR_UnloadLibrary in prlink.o
       _pr_FindSymbolInLib in prlink.o
       _pr_LoadCFBundle in prlink.o
   "_c2pstrcpy", referenced from:
       _pr_FindSymbolInLib in prlink.o
   "_CFBundleGetFunctionPointerForName", referenced from:
       _pr_FindSymbolInLib in prlink.o
   "_CFStringCreateWithCString", referenced from:
       _pr_FindSymbolInLib in prlink.o
       _pr_LoadCFBundle in prlink.o
   "_CFURLCreateWithFileSystemPath", referenced from:
       _pr_LoadCFBundle in prlink.o
   "_CloseConnection", referenced from:
       _PR_UnloadLibrary in prlink.o
   "_FindSymbol", referenced from:
       _pr_FindSymbolInLib in prlink.o
      (maybe you meant: _PR_FindSymbol, _PR_FindSymbolAndLibrary )
   "_CFBundleCreate", referenced from:
       _pr_LoadCFBundle in prlink.o
 ld: symbol(s) not found for architecture i386
 clang-3.8: error: linker command failed with exit code 1 (use -v to see invocation)
 make[3]: *** [libnspr4.dylib] Error 1
 make[3]: Leaving directory `/builds/slave/try_m64-d_sm-arm-sim-osx-00000/src/obj-spider/config/external/nspr/pr'
 make[2]: *** [config/external/nspr/pr/target] Error 2
 make[2]: *** Waiting for unfinished jobs....

Steve, these were enabled on Try nearly a year ago. Do we intend to enable these on production branches at some point or should we turn them off? Seems silly to leave them running on Try only on an indefinite basis.

Comment 1

[Steve Fink [:sfink] [:s:]]

Hey, I didn't want to turn off the ni?.

I know some people want these, but I haven't done anything at all with them for ages. I hadn't realized that they were broken, since my try pushes never get them -- they are off by default and I always push with either -p linux64, -p all, or occasionally -p win32,win64. You have to push with -p macosx64 to get them.

But yes, there's no point in keeping them if they're broken, and it's annoying to have permafail builds cluttering up try results in the first place. I'm going to ni? :jandem, since I think he is the one most likely to find these builds useful, to see if he knows how to fix cross-compiling nspr to 32 bit on osx.

Comment 2

[Steve Fink [:sfink] [:s:]]

Jan, another question -- would these builds still be useful if they were generated by cross-compiling to osx on linux64? If I want to move these builds to taskcluster, that's probably what we'll need to do. But I'm not sure if that would defeat the purpose of these builds, given that (AIUI) they exist to make failures maximally likely to be reproducible on developers' local osx machines.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Steve Fink [:sfink, :s:] from comment #1)
> I'm going to ni? :jandem, since I think he is the one most likely to
> find these builds useful, to see if he knows how to fix cross-compiling nspr
> to 32 bit on osx.

I get the same build failure when I use autospider locally on OS X. I don't know how to fix it. I wrote a patch to turn off --enable-nspr-build for the OS X arm sim builds; the POSIX NSPR emulation should be good enough for this, and it avoids a lot of pain. Let's see if that fixes things:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa4d75308b87

(In reply to Steve Fink [:sfink, :s:] from comment #2)
> Jan, another question -- would these builds still be useful if they were
> generated by cross-compiling to osx on linux64?

Do the cross-compiled builds use gcc? What I like about using Clang is that it keeps us from depending on GCC features/bugs in the JIT's ARM backend...

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

Not using --enable-nspr-build for the arm-sim-osx variant indeed fixes this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa4d75308b87

What do you think?

Comment 5

[Steve Fink [:sfink] [:s:]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> (In reply to Steve Fink [:sfink, :s:] from comment #1)
> > I'm going to ni? :jandem, since I think he is the one most likely to
> > find these builds useful, to see if he knows how to fix cross-compiling nspr
> > to 32 bit on osx.
> 
> I get the same build failure when I use autospider locally on OS X. I don't
> know how to fix it. I wrote a patch to turn off --enable-nspr-build for the
> OS X arm sim builds; the POSIX NSPR emulation should be good enough for
> this, and it avoids a lot of pain. Let's see if that fixes things:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa4d75308b87

Given that that's what you're using locally, and these builds are mostly meant to match what osx developers are using, I'd say I'm fine with that approach.

Though I'd prefer to also fix --enable-nspr-build. It's a good thing to keep working.

> (In reply to Steve Fink [:sfink, :s:] from comment #2)
> > Jan, another question -- would these builds still be useful if they were
> > generated by cross-compiling to osx on linux64?
> 
> Do the cross-compiled builds use gcc? What I like about using Clang is that
> it keeps us from depending on GCC features/bugs in the JIT's ARM backend...

The current cross-compiled builds use clang, so the path of least resistance is to continue doing so. Do we use gcc on osx for anything? I assumed it had rotted away.

ted: just for my information - I recall that you recently changed something about the nspr build. Did that touch the JS shell build with --enable-nspr-build in any way? (No action needed, at least as far as I know.)

Comment 6

[Steve Fink [:sfink] [:s:]]

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

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

So I'm ok with this patch, but given that we've already had to split out the osx build into a separate variants/ file, I'd prefer to avoid the special case by removing --enable-nspr-build entirely from autospider.sh and instead putting it in variants/* (except for variants/arm-sim-osx). I know that touches a bunch of files, but I'm already feeling like autospider.sh is getting a little too complex with all of its special casing.

I really should go through and prune out the dead variants, and the now-redundant configure flags within the variants.

Comment 7

[Steve Fink [:sfink] [:s:]]

I stuck a patch on bug 1248502 to clean up the variants a bit. One of us will end up having to rebase.

Comment 8

[(not currently active) Ted Mielczarek]

(In reply to Steve Fink [:sfink, :s:] from comment #5)
> ted: just for my information - I recall that you recently changed something
> about the nspr build. Did that touch the JS shell build with
> --enable-nspr-build in any way? (No action needed, at least as far as I
> know.)

I changed the NSPR build to use in-tree moz.build files instead of NSPR's configure+Makefiles:
https://dxr.mozilla.org/mozilla-central/source/config/external/nspr/moz.build

From the error in comment 0, it sounds like maybe this is just some missing OS_LIBS in a moz.build file:
https://dxr.mozilla.org/mozilla-central/source/config/external/nspr/pr/moz.build

Firefox builds with MOZ_FOLD_LIBS on OS X, so it doesn't actually build NSPR as shared libs, so it wouldn't hit this error.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Patch

Thanks for the pointer!

I did some searching and apparently you have to use |-framework CoreServices| for these symbols. Some websites mention CoreFoundation as well, but that wasn't necessary to get NSPR to compile (using just CoreFoundation didn't work).

Note that we add frameworks to OS_LIBS in other moz.build files so I assume that's fine: https://dxr.mozilla.org/mozilla-central/source/media/libcubeb/tests/moz.build#47-55

Comment 10

[Jan de Mooij [:jandem]]

Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04c977e1127d

Comment 11

[(not currently active) Ted Mielczarek]

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

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

::: config/external/nspr/pr/moz.build
@@ +36,5 @@
>      )
>      DEFINES[CONFIG['OS_TARGET'].upper()] = True
>      SOURCES += ['/nsprpub/pr/src/md/unix/%s.c' % CONFIG['OS_TARGET'].lower()]
>  elif CONFIG['OS_TARGET'] == 'Darwin':
> +    OS_LIBS += ["-framework CoreServices"]

nit: single quotes, not double

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a281e177105

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a281e177105