1205012 - Allow Rust sources to be used in SpiderMonkey

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

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

Comment 1

[Ralph Giles (:rillian) | needinfo me]

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?

Comment 2

[(not currently active) Ted Mielczarek]

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 3

[(not currently active) Ted Mielczarek]

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.

Comment 4

[Terrence Cole [:terrence]]

Attachment: allow_rust_sources_in_spidermonkey-v1.diff

Thanks, Ted: that's much, much nicer!

Comment 5

[Ralph Giles (:rillian) | needinfo me]

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!

Comment 6

[Ralph Giles (:rillian) | needinfo me]

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.

Comment 7

[Michael Shal [: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).

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15d9f056935d

Comment 9

[Terrence Cole [:terrence]]

Looks like I landed first. Sorry to bit rot you!

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/15d9f056935d