Last Comment Bug 831552 - Install additional internal headers, required for standalone build.
: Install additional internal headers, required for standalone build.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla21
Assigned To: darkxst
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: sm.embedding
  Show dependency treegraph
 
Reported: 2013-01-16 16:07 PST by darkxst
Modified: 2013-03-06 08:06 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed
fixed


Attachments
install extra headers (637 bytes, patch)
2013-01-16 16:07 PST, darkxst
no flags Details | Diff | Splinter Review
install extra headers (838 bytes, patch)
2013-01-16 16:35 PST, darkxst
no flags Details | Diff | Splinter Review
Slightly updated patch, with a bit more commentary, that does what we discussed on IRC (3.28 KB, patch)
2013-01-31 19:01 PST, Jeff Walden [:Waldo] (remove +bmo to email)
darkxst: review+
jimb: review+
ted: review+
akeybl: approval‑mozilla‑esr17+
Details | Diff | Splinter Review

Description darkxst 2013-01-16 16:07:22 PST
Created attachment 703082 [details] [diff] [review]
install extra headers

For the standalone spidermonkey engine from ESR10 onwards there are a bunch of extra public headers that are required by jsapi, however these do not get installed currently.

We have been using the following patch, to install these headers in our test tarballs of ESR17.
Comment 1 darkxst 2013-01-16 16:35:21 PST
Created attachment 703099 [details] [diff] [review]
install extra headers
Comment 2 Luke Wagner [:luke] 2013-01-16 22:19:08 PST
I have no clue about any of this; this patch looks like the wrong direction as all of those directories we want to keep private... anyone else know?
Comment 3 darkxst 2013-01-16 23:32:03 PST
So they are actually internal headers, but they are getting pulled in by the public headers and are needed to build any app that embeds spidermonkey without these headers installed.
Comment 4 Bill McCloskey (:billm) 2013-01-17 11:12:40 PST
I think this only installs the stuff that we've specifically approved, so it's not a problem in that sense. However, I don't know enough about the makefile to know why this is needed. Why do these things get installed during Firefox builds but not in other embeddings?
Comment 5 Luke Wagner [:luke] 2013-01-17 13:48:50 PST
Comment on attachment 703099 [details] [diff] [review]
install extra headers

I'm not qualified to review makefile changes.
Comment 6 Jim Blandy :jimb 2013-01-17 15:38:03 PST
The effect of this change would be cause 'install' to install, in addition to the files listed in INSTALLED_HEADERS, which it already does, those listed in:

- EXPORTS_ds
- EXPORTS_gc
- EXPORTS_js
- EXPORTS_mozilla

Comment 4 suggests that we've already decided that those should go all the places INSTALLED_HEADERS go; if that's the case, then this patch is fine.

I don't *think* our build system uses the 'install' make target, though. It uses 'libs' and 'export' and some other stuff, but 'install' isn't part of the process as far as I know. So I don't think it affects the Firefox build what we do with those targets.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2013-01-31 19:01:34 PST
Created attachment 708906 [details] [diff] [review]
Slightly updated patch, with a bit more commentary, that does what we discussed on IRC

The answer to the question in comment 4 is that includes show up to SpiderMonkey because |make export| exposes them.  But this is specifically about |make install|, used to install SpiderMonkey alone onto a system.

The install target used by Firefox/Gecko/etc. is here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk

It's basically oriented around installing the stuff to *run* Firefox.  There's really nothing here that makes sense for SpiderMonkey embedders, or people who want a system installation of SpiderMonkey.  So it makes sense that we'd have to install all of these headers ourselves.

After some talking on IRC and me wrapping my head around things, I think the patch posted here is pretty much good, except for adding a /js to the end of one of the lines, and alphabetizing them for simplicity.  And also -- and perhaps most important to me -- I wanted some comments explaining the what/why of this, so that it's not all just locked up in the minds of embedders and/or me.

Here's the resulting patch.  What do people think?
Comment 8 darkxst 2013-01-31 19:36:17 PST
Comment on attachment 708906 [details] [diff] [review]
Slightly updated patch, with a bit more commentary, that does what we discussed on IRC

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

Looks good.
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-06 17:44:21 PST
Comment on attachment 708906 [details] [diff] [review]
Slightly updated patch, with a bit more commentary, that does what we discussed on IRC

Sounds like jimb's a bit busy, forwarding another place as well...
Comment 10 Jim Blandy :jimb 2013-02-12 14:47:16 PST
I don't know if we'll run afoul of gps's efforts to make Makefiles purely declarative. Hopefully not, since these targets aren't actually part of Mozilla's build process proper.
Comment 11 Ted Mielczarek [:ted.mielczarek] 2013-02-13 05:49:06 PST
Our long-term goal is to get rid of Makefile.in files entirely, so we'll have to sort this all out, but since Spidermonkey already has install targets this isn't really making anything worse.
Comment 12 Ted Mielczarek [:ted.mielczarek] 2013-02-13 11:38:06 PST
Comment on attachment 708906 [details] [diff] [review]
Slightly updated patch, with a bit more commentary, that does what we discussed on IRC

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

We'll probably just replace this with a Python script invoked by some moz.build magic at some point, but this is fine for now.
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-13 14:48:32 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/c06650ce0770

This is one of the patches we'll want to backport for a 17-based release, Sean.  We'll also need to tell embedders to add js/RequiredDefines.h to their command lines, although the plan is for pkgconfig or whatever to spit it out as part of js-cflags somehow, so hopefully many won't need to take special steps here.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2013-02-13 14:49:53 PST
Er, sorry, ignore the second sentence there.  :-)  We do still want it for 17-based release, just not for those reasons.
Comment 15 Ed Morley [:emorley] 2013-02-14 02:58:18 PST
https://hg.mozilla.org/mozilla-central/rev/c06650ce0770
Comment 16 Sean Stangl [:sstangl] 2013-02-14 14:47:43 PST
Comment on attachment 708906 [details] [diff] [review]
Slightly updated patch, with a bit more commentary, that does what we discussed on IRC

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

Necessary for the mozjs17 standalone release of the JS engine. Correctly installs header files into the necessary subdirectories. Does not affect non-standalone builds.

User impact if declined:

The patch is necessary for the upcoming JS standalone release based on esr17. If declined, the patch will have to be carried separately from the main repository for the lifetime of mozjs17 support.

Fix Landed on Version: 21
Risk to taking this patch (and alternatives if risky): None: only affects JS standalone builds.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Comment 17 Alex Keybl [:akeybl] 2013-03-04 10:49:31 PST
Comment on attachment 708906 [details] [diff] [review]
Slightly updated patch, with a bit more commentary, that does what we discussed on IRC

NPOTB change in support of external packages. Approving.
Comment 18 Sean Stangl [:sstangl] 2013-03-04 16:58:08 PST
https://hg.mozilla.org/releases/mozilla-esr17/rev/441b8aa4bc66

Note You need to log in before you can comment on or make changes to this bug.