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.

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?

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.

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 on attachment 703099 [details] [diff] [review] install extra headers I'm not qualified to review makefile changes.

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.

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 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 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...

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.

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 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.

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.

Er, sorry, ignore the second sentence there. :-) We do still want it for 17-based release, just not for those reasons.

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 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.