Allow Rust sources to be used in SpiderMonkey

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript: GC
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox43 affected, firefox44 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Created attachment 8661395 [details] [diff] [review]
allow_rust_sources_in_spidermonkey-v0.diff

Adding Rust sources to SOURCES in js/src/moz.build does not work, currently. This is because js/src/ has a separate configure step and while --enable-rust is getting getting propagated to that configure, nothing is handling it there.

The attached patch just copies the MOZ_RUST stanza from configure.in to js/src/configure.in. This is suboptimal, but it seems to be how things are done right now? Is there some common place we could move this block to ensure things stay in sync?

In any case, local testing shows that adding Rust to SOURCES in js/src/moz.build works perfectly with this patch applied.
Attachment #8661395 - Flags: review?(giles)
Comment on attachment 8661395 [details] [diff] [review]
allow_rust_sources_in_spidermonkey-v0.diff

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

Looks fine to me, but you need build peer approval too, I think.

To avoid duplication we should move the details into build/autoconf/rust.m4 and instantiate it in each configure script with a macro. Can spidermonkey configure.in use files from the global m4 library?
Attachment #8661395 - Flags: review?(ted)
Attachment #8661395 - Flags: review?(giles)
Attachment #8661395 - Flags: review+
Yes, Spidermonkey's configure gets everything from build/autoconf.m4. The best way to do this would be to pull the entire MOZ_RUST block into build/autoconf.m4, and wrap it in
AC_DEFUN([MOZ_RUST_SUPPORT],
[
...
])

and then replace it with a bare `MOZ_RUST_SUPPORT` in both configure.in files.
Comment on attachment 8661395 [details] [diff] [review]
allow_rust_sources_in_spidermonkey-v0.diff

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

Please put this into build/autoconf/rust.m4 as discussed in the previous comment. You'll also need to add that file to $topsrcdir/aclocal.m4.
Attachment #8661395 - Flags: review?(ted) → review-
(Assignee)

Comment 4

2 years ago
Created attachment 8661875 [details] [diff] [review]
allow_rust_sources_in_spidermonkey-v1.diff

Thanks, Ted: that's much, much nicer!
Attachment #8661395 - Attachment is obsolete: true
Attachment #8661875 - Flags: review?(ted)
Comment on attachment 8661875 [details] [diff] [review]
allow_rust_sources_in_spidermonkey-v1.diff

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

Much better, indeed. Thanks!
Attachment #8661875 - Flags: review+
Comment on attachment 8661875 [details] [diff] [review]
allow_rust_sources_in_spidermonkey-v1.diff

Moving review to mshal who's also been looking at the rust stuff lately.
Attachment #8661875 - Flags: review?(ted) → review?(mshal)
Comment on attachment 8661875 [details] [diff] [review]
allow_rust_sources_in_spidermonkey-v1.diff

Looks good! But please see bug 1208566 for some minor bit rot (the rust version check will need to be updated).
Attachment #8661875 - Flags: review?(mshal) → review+

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/15d9f056935d
(Assignee)

Comment 9

2 years ago
Looks like I landed first. Sorry to bit rot you!
https://hg.mozilla.org/mozilla-central/rev/15d9f056935d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.