Closed Bug 1161339 Opened 9 years ago Closed 9 years ago

Support rust files in moz.build SOURCES

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: rillian, Assigned: rillian)

References

Details

Attachments

(3 files, 7 obsolete files)

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.
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)
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)
Attached patch Part 3 - test code WIP (obsolete) — Splinter Review
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.
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 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.
(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.
(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 :)
(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.
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.
(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...
(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.
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.
(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.
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)
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)
Attached patch Part 3 - test code WIP v2 (obsolete) — Splinter Review
Same for build-test patch.
Attachment #8601217 - Attachment is obsolete: true
(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.
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)
Attached patch Part 3 - test code WIP v3 (obsolete) — Splinter Review
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
Attachment #8602917 - Flags: review?(cajbir.bugzilla)
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
Attachment #8602917 - Flags: review?(cajbir.bugzilla) → review+
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 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+
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.
(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 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.
Update makefile to set an explicit output name. Carrying forward r=ted.
Attachment #8602255 - Attachment is obsolete: true
Blocks: 1163214
Blocks: 1163224
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.