Closed
Bug 1205012
Opened 6 years ago
Closed 6 years ago
Allow Rust sources to be used in SpiderMonkey
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file, 1 obsolete file)
6.12 KB,
patch
|
mshal
:
review+
rillian
:
review+
|
Details | Diff | Splinter Review |
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 1•6 years ago
|
||
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+
Comment 2•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
Thanks, Ted: that's much, much nicer!
Attachment #8661395 -
Attachment is obsolete: true
Attachment #8661875 -
Flags: review?(ted)
Comment 5•6 years ago
|
||
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 6•6 years ago
|
||
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 7•6 years ago
|
||
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+
Assignee | ||
Comment 9•6 years ago
|
||
Looks like I landed first. Sorry to bit rot you!
Comment 10•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/15d9f056935d
Status: ASSIGNED → RESOLVED
Closed: 6 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.
Description
•