1379536 - Standalone SpiderMonkey should not use @executable_path in install_name on macOS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Philip Chimento [:ptomato]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

Building SpiderMonkey as a standalone library on macOS, the built .dylib's install_name should be an absolute path. If it uses @executable_path which is currently the case, then you can't run any programs that link to the .dylib unless they are installed into the same directory as the .dylib.


Actual results:

Running any program that links to the built SpiderMonkey .dylib crashes with 'Library not loaded: (incorrect path here) Reason: image not found'

Comment 1

[Philip Chimento [:ptomato]]

Attachment: 0002-build-Fix-library-install-name-on-macOS.patch

This patch makes it work for me, but it probably needs some magic to only have the change take effect when building standalone.

Comment 2

[Till Schneidereit [:till]]

Comment on attachment 8884710 [details] [diff] [review]
0002-build-Fix-library-install-name-on-macOS.patch

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

Mike, can you comment on whether this is the right approach or, if not, what would be the right approach?

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8884710 [details] [diff] [review]
0002-build-Fix-library-install-name-on-macOS.patch

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

The install name needs to stay as it is. Otherwise, you just break e.g. running the shell from the build directory. Plus, that obviously totally breaks Firefox.

Anyways, the right thing to do is to change the install name at `make install` time, with install_name_tool.

Comment 4

[Philip Chimento [:ptomato]]

Attachment: Fix library install_name on macOS

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8889009 [details] [diff] [review]
Fix library install_name on macOS

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

::: js/src/Makefile.in
@@ +220,5 @@
>  endif
>  ifneq (,$(SHARED_LIBRARY))
>  	$(SYSINSTALL) $(SHARED_LIBRARY) $(DESTDIR)$(libdir)
> +ifeq ($(OS_ARCH),Darwin)
> +	install_name_tool -id $(abspath $(libdir)/$(SHARED_LIBRARY)) $(DESTDIR)$(libdir)/$(SHARED_LIBRARY)

That is not the full story. This is useful for anything that would link against that system-installed library, but as long as js is still installed, it needs to be adjusted too, presumably with install_name_tool -change.

@@ +226,5 @@
>  endif
>  ifneq (,$(IMPORT_LIBRARY))
>  	$(SYSINSTALL) $(IMPORT_LIBRARY) $(DESTDIR)$(libdir)
> +ifeq ($(OS_ARCH),Darwin)
> +	install_name_tool -id $(abspath $(libdir)/$(IMPORT_LIBRARY)) $(DESTDIR)$(libdir)/$(IMPORT_LIBRARY)

This is effectively dead code, as import libraries are a windows-only thing.

Comment 6

[Philip Chimento [:ptomato]]

(In reply to Mike Hommey [:glandium] from comment #5)
> Comment on attachment 8889009 [details] [diff] [review]
> Fix library install_name on macOS
> 
> Review of attachment 8889009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/Makefile.in
> @@ +220,5 @@
> >  endif
> >  ifneq (,$(SHARED_LIBRARY))
> >  	$(SYSINSTALL) $(SHARED_LIBRARY) $(DESTDIR)$(libdir)
> > +ifeq ($(OS_ARCH),Darwin)
> > +	install_name_tool -id $(abspath $(libdir)/$(SHARED_LIBRARY)) $(DESTDIR)$(libdir)/$(SHARED_LIBRARY)
> 
> That is not the full story. This is useful for anything that would link
> against that system-installed library, but as long as js is still installed,
> it needs to be adjusted too, presumably with install_name_tool -change.

Since I have a patch pending on bug 1339931 to stop installing the js shell, would you be OK leaving this as-is and landing this patch after that one?
 
> @@ +226,5 @@
> >  endif
> >  ifneq (,$(IMPORT_LIBRARY))
> >  	$(SYSINSTALL) $(IMPORT_LIBRARY) $(DESTDIR)$(libdir)
> > +ifeq ($(OS_ARCH),Darwin)
> > +	install_name_tool -id $(abspath $(libdir)/$(IMPORT_LIBRARY)) $(DESTDIR)$(libdir)/$(IMPORT_LIBRARY)
> 
> This is effectively dead code, as import libraries are a windows-only thing.

Not so dead as you might think, maybe - $(IMPORT_LIBRARY) is apparently set on macOS as well to the same thing as $(SHARED_LIBRARY). So without this, the import library install rule overwrote the installed library that had its install name adjusted, with another copy of the unadjusted one.

Maybe the solution is to adjust the install name before installing, I'll change the patch to do that instead.

Comment 7

[Philip Chimento [:ptomato]]

Attachment: Fix library install_name on macOS

Note that for backport to esr52, the patch must be applied to
js/src/Makefile.in instead of js/src/build/Makefile.in.

Comment 8

[Mike Hommey [:glandium]]

(In reply to Philip Chimento [:ptomato] from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #5)
> > Comment on attachment 8889009 [details] [diff] [review]
> > Fix library install_name on macOS
> > 
> > Review of attachment 8889009 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/Makefile.in
> > @@ +220,5 @@
> > >  endif
> > >  ifneq (,$(SHARED_LIBRARY))
> > >  	$(SYSINSTALL) $(SHARED_LIBRARY) $(DESTDIR)$(libdir)
> > > +ifeq ($(OS_ARCH),Darwin)
> > > +	install_name_tool -id $(abspath $(libdir)/$(SHARED_LIBRARY)) $(DESTDIR)$(libdir)/$(SHARED_LIBRARY)
> > 
> > That is not the full story. This is useful for anything that would link
> > against that system-installed library, but as long as js is still installed,
> > it needs to be adjusted too, presumably with install_name_tool -change.
> 
> Since I have a patch pending on bug 1339931 to stop installing the js shell,
> would you be OK leaving this as-is and landing this patch after that one?

Fair enough.

> > @@ +226,5 @@
> > >  endif
> > >  ifneq (,$(IMPORT_LIBRARY))
> > >  	$(SYSINSTALL) $(IMPORT_LIBRARY) $(DESTDIR)$(libdir)
> > > +ifeq ($(OS_ARCH),Darwin)
> > > +	install_name_tool -id $(abspath $(libdir)/$(IMPORT_LIBRARY)) $(DESTDIR)$(libdir)/$(IMPORT_LIBRARY)
> > 
> > This is effectively dead code, as import libraries are a windows-only thing.
> 
> Not so dead as you might think, maybe - $(IMPORT_LIBRARY) is apparently set
> on macOS as well to the same thing as $(SHARED_LIBRARY). So without this,
> the import library install rule overwrote the installed library that had its
> install name adjusted, with another copy of the unadjusted one.
> 
> Maybe the solution is to adjust the install name before installing, I'll
> change the patch to do that instead.

Huh, please don't. File a separate bug for the above paragraph instead, we shouldn't do that in the first place.

Comment 9

[Philip Chimento [:ptomato]]

Attachment: Fix library install_name on macOS

Note that for backport to esr52, the patch must be applied to
js/src/Makefile.in instead of js/src/build/Makefile.in.

Comment 10

[Philip Chimento [:ptomato]]

Comment on attachment 8890101 [details] [diff] [review]
Fix library install_name on macOS

OK, this one should do the trick, but needs to land after bug 1339931.

Comment 11

[Philip Chimento [:ptomato]]

Comment on attachment 8890101 [details] [diff] [review]
Fix library install_name on macOS

Going to leave the js shell installed after all, so I'll redo this patch to work the install_name magic on it as well.

Comment 12

[Philip Chimento [:ptomato]]

Comment on attachment 8890101 [details] [diff] [review]
Fix library install_name on macOS

Actually, the js shell appears to link statically to the js library. In any case, otool -L shows no libmozjs to change with install_name_tool. So, I think the previous version of this patch is good after all.

Comment 13

[Philip Chimento [:ptomato]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a32dc1aabb43410697bad6699840a2feeb3c91c9&selectedJob=127641575
All failures accounted for in other bugs.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0959abdc7a87
Fix library install_name on macOS. r=glandium

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0959abdc7a87

Comment 16

[Philip Chimento [:ptomato]]

Comment on attachment 8890101 [details] [diff] [review]
Fix library install_name on macOS

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a build fix for standalone SpiderMonkey on macOS.
User impact if declined: None to Firefox users, but embedders will need to patch the ESR tarball for compiling on macOS.
Fix Landed on Version: 57
Risk to taking this patch (and alternatives if risky): Very low risk, only affects "make install"
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8890101 [details] [diff] [review]
Fix library install_name on macOS

packaging fix for spidermonkey, esr52.4+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/67e885027a23