Closed
Bug 1161339
Opened 9 years ago
Closed 9 years ago
Support rust files in moz.build SOURCES
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox40 fixed)
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: rillian, Assigned: rillian)
References
Details
Attachments
(3 files, 7 obsolete files)
1.39 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
1.45 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
3.86 KB,
patch
|
Details | Diff | Splinter Review |
We'd like to be able to test gecko code written in rust. This bug is about getting basic rustc support in the build system.
Assignee | ||
Comment 1•9 years ago
|
||
As a first step, let's just check for a rustc already installed on the system in the configure script. That can serve as a conditional for later code.
Attachment #8601214 -
Flags: review?(ted)
Assignee | ||
Comment 2•9 years ago
|
||
The adds support for .rs files in moz.build SOURCES and corresponding rules for the makefile back end. 'rustc --crate-type staticlib --emit obj' may not be the correct invocation. This seems to give me an stand-alone object file, but I get unresolved symbol error on ___morestack when I link my test code into gecko.
Attachment #8601216 -
Flags: review?(ted)
Assignee | ||
Comment 3•9 years ago
|
||
WIP test code I'm using the verify the build system changes. This shouldn't land as is, but could be turned into an optional cpp unit test.
Comment 4•9 years ago
|
||
Please don't make the presence of a rust compiler magically trigger a build change. IMHO, building the rust parts should be a conscious choice (and the build should fail if that choice was made and a compiler isn't there)
Comment 5•9 years ago
|
||
Comment on attachment 8601216 [details] [diff] [review] Part 2 - mozbuild support for rustc under Makefiles Review of attachment 8601216 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/rules.mk @@ +928,5 @@ > +ifdef RUSTC > +$(RSOBJS): > + $(REPORT_BUILD) > + $(RUSTC) --crate-type staticlib --emit obj $(_VPATH_SRCS) > +endif I'd like these rules to be emitted by backend.py. By putting the logic for compiling Rust files one step above Makefiles, we make it easier to do cooler stuff in the future. e.g. if we had C++ compilation logic in Python, we would be building with Ninja or Tup already. As it stands, this logic is intractable with make files and our flexibility to easily use "not make" is limited. I think touching rules.mk for e.g. _OBJS and $(call src_objdep) is be OK. But if we can do that in backend.mk files, that is preferred. Don't bend over backwards though to do this though.
Comment 6•9 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #5) > I'd like these rules to be emitted by backend.py. I don't agree. Each backend would still need their own way of dealing with those command lines, and for the recursive make backend, that it's emitted by the python code or shared in rules.mk makes no practical difference.
Comment 7•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #6) > (In reply to Gregory Szorc [:gps] from comment #5) > > I'd like these rules to be emitted by backend.py. > > I don't agree. Each backend would still need their own way of dealing with > those command lines, and for the recursive make backend, that it's emitted > by the python code or shared in rules.mk makes no practical difference. It is the calculation of what exactly the command line is that I care about. By separating the logic for "what to do" (the commands and their arguments) from "how to do it" (make syntax), we make it easier to introduce new build backends that invoke those commands in different ways. Yes, we can get the "how to do it" from make rules (as Ehsan recently implemented in the YouCompleteMe feature), but it a hacky workaround to get around the fact we don't have an easier way. If nothing else, I would rather we code logic for defining command invocations in Python instead of make. One of those is easily readable and testable :)
Comment 8•9 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #7) > By separating the logic for "what to do" (the commands and their arguments) > from "how to do it" (make syntax), we make it easier to introduce new build > backends that invoke those commands in different ways. The "what to do" is in most cases "run the compiler", with some "default flags". Everything else is entirely specific to the backend (and yes, to compile C++ files, it's a lot of complicated things, some of which is "those things should be in the default flags but currently aren't and we sometimes rely on being able to override them"). That's hell of a scope creep to ask the people who do the rust integration to deal with more than setting variables for their compiler and the default flags it needs, which can be done in configure.in anyways.
Comment 9•9 years ago
|
||
Given that rust builds a complete library at a time, I'm not sure that SOURCES is the right abstraction; I don't know the library parts of the build system well enough to come up with a sensible alternative, though.
Comment 10•9 years ago
|
||
(In reply to :Ms2ger from comment #9) > Given that rust builds a complete library at a time, I'm not sure that > SOURCES is the right abstraction; I don't know the library parts of the > build system well enough to come up with a sensible alternative, though. The starting point is still a source file, though, and the library name might be different(?). BTW, another requirement to take into account: we're often cross-compiling, meaning the build target is different from the host system. That's true for android, but that's also true for linux and osx, where we build 32-bits binaries on 64-bits systems (that is, in fact, also true on windows). The rust command line we use needs to handle this, otherwise we'd likely get 64-bits objects to link to other 32-bits objects...
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #5) > I'd like these rules to be emitted by backend.py. That's scope creep. A fine idea, but since that's not how the current build rules are specified it should be a separate bug. (In reply to :Ms2ger from comment #9) > Given that rust builds a complete library at a time, I'm not sure that > SOURCES is the right abstraction; I don't know the library parts of the > build system well enough to come up with a sensible alternative, though. Not a fan of unified build then? :) This is based on the assertion in https://bugzilla.mozilla.org/show_bug.cgi?id=1135640#c20 that cargo isn't ready. The next simplest thing is to call rustc the same way we do the c, c++ and asm compilers.
Comment 12•9 years ago
|
||
No, not a fan of unified builds, but that's not quite what happens here. Note that it's not a cargo feature; rustc does the including itself.
Comment 13•9 years ago
|
||
(In reply to Ralph Giles (:rillian) from comment #11) > (In reply to Gregory Szorc [:gps] from comment #5) > > > I'd like these rules to be emitted by backend.py. > > That's scope creep. A fine idea, but since that's not how the current build > rules are specified it should be a separate bug. That's not how the current *C++ build rules* are specified. Rust != C++. From my perspective, we are starting from a clean slate and should do things properly. There is precedent for emitting make rules from Python. See WebIDL: https://hg.mozilla.org/mozilla-central/file/754579ec0e68/python/mozbuild/mozbuild/backend/recursivemake.py#l1386 That being said, I'm a proponent of perfect is the enemy of done. I understand writing the Python is scope bloat for you and not your area of expertise. You have my blessing to contribute something that works (i.e. the approach in the current patches). We can always refactor it later.
Assignee | ||
Comment 14•9 years ago
|
||
Updated configure patch to address #4. One must now 'ac_add_options --enable-rust' to compile rust code. This option will trigger an error if set when rustc isn't in path.
Attachment #8601214 -
Attachment is obsolete: true
Attachment #8601214 -
Flags: review?(ted)
Attachment #8601613 -
Flags: review?(ted)
Assignee | ||
Comment 15•9 years ago
|
||
Update build rules to propagate the new MOZ_RUST conditional in place of RUSTC.
Attachment #8601216 -
Attachment is obsolete: true
Attachment #8601216 -
Flags: review?(ted)
Attachment #8601614 -
Flags: review?(ted)
Assignee | ||
Comment 16•9 years ago
|
||
Same for build-test patch.
Attachment #8601217 -
Attachment is obsolete: true
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #13) > I understand writing the Python is scope bloat for you and not your area of > expertise. You have my blessing to contribute something that works (i.e. the > approach in the current patches). We can always refactor it later. Appreciated, thanks.
Assignee | ||
Comment 18•9 years ago
|
||
Switch to building static libraries with rustc. This is hackier than what we had before but I now think it's a better choice. When asked to output a object file, rustc doesn't include all the bits of the compiler-specific runtime code needed to resolve everything, but there's no clear way to find an equivalent of libgcc.a for resolving those links. What is better supported is to generate a static library which only depends on public system libraries. This is a better match for rust's native multi-file compilation modules, but this patch doesn't take advantage of that. Putting everything in a single source file is sufficient as a starting point. Gecko happens to already link all the system libraries rustc output depends on, so this patch also ignores that issue. It does write what it's linking against to stderr during the output stage, so we can parse that later if we need to, or add them manually with platform-specific code in configure.in or moz.build.
Attachment #8601614 -
Attachment is obsolete: true
Attachment #8601614 -
Flags: review?(ted)
Attachment #8602255 -
Flags: review?(ted)
Assignee | ||
Comment 19•9 years ago
|
||
Import rust code as 'extern "C"'. This resolves the remaining linking problem I was having. Thanks to mbrubeck for help with this issue.
Attachment #8601615 -
Attachment is obsolete: true
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8602917 -
Flags: review?(cajbir.bugzilla)
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8602257 [details] [diff] [review] Part 3 - test code WIP v3 To verify, apply all three patches. Add 'ac_add_options --enable-rust' to mozconfig, ./mach build and then: ./mach gtest rust.*
Attachment #8602257 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8602917 -
Flags: review?(cajbir.bugzilla) → review+
Comment 22•9 years ago
|
||
Comment on attachment 8601613 [details] [diff] [review] Part 1 - Check for rustc in configure v2 Review of attachment 8601613 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +433,5 @@ > +MOZ_ARG_ENABLE_BOOL([rust], > + [ --enable-rust Include rust language sources], > + [MOZ_RUST=1], > + [MOZ_RUST= ]) > +if test -z "$RUSTC" -a -n "$MOZ_RUST"; then Do we want to do some basic version sanity check to ensure we're using a new-enough rustc? (We probably want at least a 1.0.0 prerelease, right?) It's not infeasible that people might have an ancient rustc laying around earlier in $PATH. I assume we'll wind up needing a specific rustc version pretty soon anyway, so putting the plumbing in for it wouldn't be a waste.
Attachment #8601613 -
Flags: review?(ted) → review+
Comment 23•9 years ago
|
||
Comment on attachment 8602255 [details] [diff] [review] Part 2 - mozbuild support for rustc under Makefiles v3 Review of attachment 8602255 [details] [diff] [review]: ----------------------------------------------------------------- It doesn't thrill me that we can't convince rustc to just produce object files that we can link. Intermediate static libs aren't great for a number of reasons, which is why we stopped generating them some time ago. It's even worse that we're going to produce a static lib per-Rust-sourcefile, that seems extremely suboptimal. Can you file a followup on making this less bad? We'll make that block turning this on by default, certainly, and maybe even turning it on in production if it has a bad impact on our build times. I also noticed (trying out rustc commandlines locally) that `--crate-type staticlib` sticks all the deps into the archive it produces, so taking the sample program from rust-lang.org and compiling it: `rustc --crate-type staticlib -o librusttest.a test.rs` produces: $ ar t librusttest.a __.SYMDEF SORTED librusttest.o r-morestack-morestack.o r-compiler-rt-absvdi2.o r-compiler-rt-absvsi2.o r-compiler-rt-absvti2.o r-compiler-rt-adddf3.o r-compiler-rt-addsf3.o r-compiler-rt-addvdi3.o r-compiler-rt-addvsi3.o r-compiler-rt-addvti3.o r-compiler-rt-apple_versioning.o r-compiler-rt-ashldi3.o r-compiler-rt-ashlti3.o r-compiler-rt-ashrdi3.o r-compiler-rt-ashrti3.o r-compiler-rt-clear_cache.o r-compiler-rt-clzdi2.o r-compiler-rt-clzsi2.o r-compiler-rt-clzti2.o r-compiler-rt-cmpdi2.o r-compiler-rt-cmpti2.o r-compiler-rt-comparedf2.o r-compiler-rt-comparesf2.o r-compiler-rt-comparetf2.o r-compiler-rt-ctzdi2.o r-compiler-rt-ctzsi2.o r-compiler-rt-ctzti2.o r-compiler-rt-divdc3.o r-compiler-rt-divdf3.o r-compiler-rt-divdi3.o r-compiler-rt-divmoddi4.o r-compiler-rt-divmodsi4.o r-compiler-rt-divsc3.o r-compiler-rt-divsf3.o r-compiler-rt-divsi3.o r-compiler-rt-divti3.o r-compiler-rt-divxc3.o r-compiler-rt-eprintf.o r-compiler-rt-extendsfdf2.o r-compiler-rt-ffsdi2.o r-compiler-rt-ffsti2.o r-compiler-rt-fixdfdi.o r-compiler-rt-fixdfsi.o r-compiler-rt-fixdfti.o r-compiler-rt-fixsfdi.o r-compiler-rt-fixsfsi.o r-compiler-rt-fixsfti.o r-compiler-rt-fixunsdfdi.o r-compiler-rt-fixunsdfsi.o r-compiler-rt-fixunsdfti.o r-compiler-rt-fixunssfdi.o r-compiler-rt-fixunssfsi.o r-compiler-rt-fixunssfti.o r-compiler-rt-fixunsxfdi.o r-compiler-rt-fixunsxfsi.o r-compiler-rt-fixunsxfti.o r-compiler-rt-fixxfdi.o r-compiler-rt-fixxfti.o r-compiler-rt-floatdidf.o r-compiler-rt-floatdisf.o r-compiler-rt-floatdixf.o r-compiler-rt-floatsidf.o r-compiler-rt-floatsisf.o r-compiler-rt-floattidf.o r-compiler-rt-floattisf.o r-compiler-rt-floattixf.o r-compiler-rt-floatundidf.o r-compiler-rt-floatundisf.o r-compiler-rt-floatundixf.o r-compiler-rt-floatunsidf.o r-compiler-rt-floatunsisf.o r-compiler-rt-floatuntidf.o r-compiler-rt-floatuntisf.o r-compiler-rt-floatuntixf.o r-compiler-rt-gcc_personality_v0.o r-compiler-rt-int_util.o r-compiler-rt-lshrdi3.o r-compiler-rt-lshrti3.o r-compiler-rt-moddi3.o r-compiler-rt-modsi3.o r-compiler-rt-modti3.o r-compiler-rt-muldc3.o r-compiler-rt-muldf3.o r-compiler-rt-muldi3.o r-compiler-rt-mulodi4.o r-compiler-rt-mulosi4.o r-compiler-rt-muloti4.o r-compiler-rt-mulsc3.o r-compiler-rt-mulsf3.o r-compiler-rt-multi3.o r-compiler-rt-mulvdi3.o r-compiler-rt-mulvsi3.o r-compiler-rt-mulvti3.o r-compiler-rt-mulxc3.o r-compiler-rt-negdf2.o r-compiler-rt-negdi2.o r-compiler-rt-negsf2.o r-compiler-rt-negti2.o r-compiler-rt-negvdi2.o r-compiler-rt-negvsi2.o r-compiler-rt-negvti2.o r-compiler-rt-paritydi2.o r-compiler-rt-paritysi2.o r-compiler-rt-parityti2.o r-compiler-rt-popcountdi2.o r-compiler-rt-popcountsi2.o r-compiler-rt-popcountti2.o r-compiler-rt-powidf2.o r-compiler-rt-powisf2.o r-compiler-rt-powitf2.o r-compiler-rt-powixf2.o r-compiler-rt-subdf3.o r-compiler-rt-subsf3.o r-compiler-rt-subvdi3.o r-compiler-rt-subvsi3.o r-compiler-rt-subvti3.o r-compiler-rt-trampoline_setup.o r-compiler-rt-truncdfsf2.o r-compiler-rt-ucmpdi2.o r-compiler-rt-ucmpti2.o r-compiler-rt-udivdi3.o r-compiler-rt-udivmoddi4.o r-compiler-rt-udivmodsi4.o r-compiler-rt-udivmodti4.o r-compiler-rt-udivsi3.o r-compiler-rt-udivti3.o r-compiler-rt-umoddi3.o r-compiler-rt-umodsi3.o r-compiler-rt-umodti3.o r-std-r-rust_builtin-rust_android_dummy.o r-std-r-rust_builtin-rust_builtin.o r-std-r-rustrt_native-record_sp.o r-std-r-rustrt_native-rust_try.o r-std-std-4e7c5e5c.o r-collections-collections-4e7c5e5c.o r-unicode-unicode-4e7c5e5c.o r-rand-rand-4e7c5e5c.o r-alloc-alloc-4e7c5e5c.o r-alloc-r-jemalloc-arena.pic.o r-alloc-r-jemalloc-atomic.pic.o r-alloc-r-jemalloc-base.pic.o r-alloc-r-jemalloc-bitmap.pic.o r-alloc-r-jemalloc-chunk.pic.o r-alloc-r-jemalloc-chunk_dss.pic.o r-alloc-r-jemalloc-chunk_mmap.pic.o r-alloc-r-jemalloc-ckh.pic.o r-alloc-r-jemalloc-ctl.pic.o r-alloc-r-jemalloc-extent.pic.o r-alloc-r-jemalloc-hash.pic.o r-alloc-r-jemalloc-huge.pic.o r-alloc-r-jemalloc-jemalloc.pic.o r-alloc-r-jemalloc-mb.pic.o r-alloc-r-jemalloc-mutex.pic.o r-alloc-r-jemalloc-prof.pic.o r-alloc-r-jemalloc-quarantine.pic.o r-alloc-r-jemalloc-rtree.pic.o r-alloc-r-jemalloc-stats.pic.o r-alloc-r-jemalloc-tcache.pic.o r-alloc-r-jemalloc-tsd.pic.o r-alloc-r-jemalloc-util.pic.o r-alloc-r-jemalloc-valgrind.pic.o r-alloc-r-jemalloc-zone.pic.o r-libc-libc-4e7c5e5c.o r-core-core-4e7c5e5c.o That's a lot of objects for a single rust file that doesn't explicitly import anything! Are we going to wind up with a staticlib like this for *every* rust file we compile? I feel like rustc could be doing better for us. ::: config/rules.mk @@ +935,5 @@ > +# Assume any system libraries rustc links against are already > +# in the target's LIBS. > +$(RSOBJS): > + $(REPORT_BUILD) > + $(RUSTC) --crate-type staticlib $(_VPATH_SRCS) I'd prefer if we were more explicit about the output filename with -o here. You could pull that $(basename) bit out of src_libdep as a separate function and use it here.
Attachment #8602255 -
Flags: review?(ted) → review+
Comment 24•9 years ago
|
||
We're not going to do anything per-Rust-sourcefile, because that kind of compilation simply doesn't exist. rust-url, for example, is compiled entirely as one unit and would only produce one library.
Assignee | ||
Comment 25•9 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #22) > It's not infeasible that people might have an ancient rustc laying around > earlier in $PATH. That's a good point. I think we should in fact target the 1.0.x stable releases, which start next week. I'll file a follow-up bug for this.
Comment 26•9 years ago
|
||
Comment on attachment 8602255 [details] [diff] [review] Part 2 - mozbuild support for rustc under Makefiles v3 Review of attachment 8602255 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/rules.mk @@ +935,5 @@ > +# Assume any system libraries rustc links against are already > +# in the target's LIBS. > +$(RSOBJS): > + $(REPORT_BUILD) > + $(RUSTC) --crate-type staticlib $(_VPATH_SRCS) Also, see comment 10.
Assignee | ||
Comment 27•9 years ago
|
||
Update makefile to set an explicit output name. Carrying forward r=ted.
Attachment #8602255 -
Attachment is obsolete: true
Comment 28•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b811c7d4f39b https://hg.mozilla.org/integration/mozilla-inbound/rev/ea593c621ffe https://hg.mozilla.org/integration/mozilla-inbound/rev/083d678bb7fd
Comment 29•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b811c7d4f39b https://hg.mozilla.org/mozilla-central/rev/ea593c621ffe https://hg.mozilla.org/mozilla-central/rev/083d678bb7fd
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•