Standalone SpiderMonkey should not install shared library twice

RESOLVED FIXED in Firefox -esr52

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: ptomato, Assigned: ptomato)

Tracking

52 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox-esr52 fixed, firefox57 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 months ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36

Steps to reproduce:

Looks like the libmozjs-52 shared library is getting installed twice when building standalone SpiderMonkey on macOS. It is getting installed once as $(SHARED_LIBRARY) and once as $(IMPORT_LIBRARY). It's my understanding that $(IMPORT_LIBRARY) should be empty on non-Windows systems, but it is instead set to the same value as $(SHARED_LIBRARY).


Actual results:

Relevant log from "make install":

../../config/nsinstall -t libmozjs-52.dylib /Users/ptomato/jhbuild/install/_jhbuild/root-mozjs52/Users/ptomato/jhbuild/install/lib
../../config/nsinstall -t libmozjs-52.dylib /Users/ptomato/jhbuild/install/_jhbuild/root-mozjs52/Users/ptomato/jhbuild/install/lib


Expected results:

The library should only have been installed once.
(Assignee)

Updated

11 months ago
Blocks: 1379536
(Assignee)

Comment 1

11 months ago
Would the correct approach be to change the makefile code generation at http://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/backend/recursivemake.py#1232 to emit, say, "ifeq ($(OS_ARCH),WINNT)" around the IMPORT_LIBRARY definition and otherwise leave the variable blank?

It seems like BaseLibrary.import_name is used for a lot of other things so I don't really want to mess with it.
Flags: needinfo?(mh+mozilla)

Comment 2

11 months ago
I think a simpler approach would be to skip the import lib install when it's the same as the shared library, in js/src/build/Makefile.in.
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 3

10 months ago
Created attachment 8902522 [details] [diff] [review]
Don't install import library if same as shared library

When backporting to esr52, the change should be applied to
js/src/Makefile.in instead of js/src/build/Makefile.in.
(Assignee)

Updated

10 months ago
Attachment #8902522 - Flags: review?(mh+mozilla)

Updated

10 months ago
Attachment #8902522 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 4

10 months ago
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a32dc1aabb43410697bad6699840a2feeb3c91c9&selectedJob=127641575
All failures accounted for in other bugs.
Keywords: checkin-needed
Assignee: nobody → philip.chimento

Comment 5

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec954e9a0e55
Don't install import library if same as shared library. r=glandium
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ec954e9a0e55
Status: UNCONFIRMED → RESOLVED
Last Resolved: 10 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Comment 7

10 months ago
Comment on attachment 8902522 [details] [diff] [review]
Don't install import library if same as shared library

[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.
Attachment #8902522 - Flags: approval-mozilla-esr52?
Comment on attachment 8902522 [details] [diff] [review]
Don't install import library if same as shared library

packaging fix for spidermonkey, esr52.4+
Attachment #8902522 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Comment 9

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/1cacc7c362b3
status-firefox-esr52: --- → fixed
You need to log in before you can comment on or make changes to this bug.