Closed
Bug 1384308
Opened 7 years ago
Closed 7 years ago
Standalone SpiderMonkey should not install shared library twice
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: ptomato, Assigned: ptomato)
References
Details
Attachments
(1 file)
808 bytes,
patch
|
glandium
:
review+
jcristau
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•7 years 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•7 years 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•7 years ago
|
||
When backporting to esr52, the change should be applied to js/src/Makefile.in instead of js/src/build/Makefile.in.
Assignee | ||
Updated•7 years ago
|
Attachment #8902522 -
Flags: review?(mh+mozilla)
Updated•7 years ago
|
Attachment #8902522 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 4•7 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a32dc1aabb43410697bad6699840a2feeb3c91c9&selectedJob=127641575 All failures accounted for in other bugs.
Keywords: checkin-needed
Updated•7 years ago
|
Assignee: nobody → philip.chimento
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
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ec954e9a0e55
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 7•7 years 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 8•7 years ago
|
||
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•7 years ago
|
||
bugherder uplift |
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.
Description
•