Install additional internal headers, required for standalone build.

RESOLVED FIXED in Firefox 21

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: darkxst, Assigned: darkxst)

Tracking

(Blocks: 1 bug)

unspecified
mozilla21
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox20 wontfix, firefox21 fixed, firefox-esr17 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 703099 [details] [diff] [review]
install extra headers
Attachment #703082 - Attachment is obsolete: true
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #703099 - Flags: review?(luke)

Comment 2

5 years ago
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?
(Assignee)

Comment 3

5 years ago
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 5

5 years ago
Comment on attachment 703099 [details] [diff] [review]
install extra headers

I'm not qualified to review makefile changes.
Attachment #703099 - Flags: review?(luke)

Comment 6

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: Install additional public headers. → Install additional internal headers, required for standalone build.
Flags: needinfo?(jwalden+bmo)
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?
Attachment #703099 - Attachment is obsolete: true
Attachment #708906 - Flags: review?(jimb)
Attachment #708906 - Flags: review?(darkxst)
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 8

5 years ago
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.
Blocks: 837921
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...
Attachment #708906 - Flags: review?(ted)

Updated

4 years ago
Attachment #708906 - Flags: review?(jimb) → review+

Comment 10

4 years ago
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.
Attachment #708906 - Flags: review?(ted) → review+
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.
Assignee: general → darkxst
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla21
Er, sorry, ignore the second sentence there.  :-)  We do still want it for 17-based release, just not for those reasons.
https://hg.mozilla.org/mozilla-central/rev/c06650ce0770
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
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.
Attachment #708906 - Flags: approval-mozilla-esr17?
(Assignee)

Updated

4 years ago
Attachment #708906 - Flags: review?(darkxst) → review+
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.
Attachment #708906 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
https://hg.mozilla.org/releases/mozilla-esr17/rev/441b8aa4bc66
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
status-firefox-esr17: --- → fixed
You need to log in before you can comment on or make changes to this bug.