Closed Bug 1205012 Opened 4 years ago Closed 4 years ago

Allow Rust sources to be used in SpiderMonkey

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file, 1 obsolete file)

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-
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+
Looks like I landed first. Sorry to bit rot you!
https://hg.mozilla.org/mozilla-central/rev/15d9f056935d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.