Last Comment Bug 1161339 - Support rust files in moz.build SOURCES
: Support rust files in moz.build SOURCES
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: Unspecified Unspecified
-- normal (vote)
: mozilla40
Assigned To: Ralph Giles (:rillian) | needinfo me
:
: Gregory Szorc [:gps] (away until 2017-03-20)
Mentors:
Depends on:
Blocks: oxidation 1163214 1163224
  Show dependency treegraph
 
Reported: 2015-05-04 16:51 PDT by Ralph Giles (:rillian) | needinfo me
Modified: 2015-05-30 15:49 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Part 1 - Check for rustc in configure. (776 bytes, patch)
2015-05-04 17:03 PDT, Ralph Giles (:rillian) | needinfo me
no flags Details | Diff | Splinter Review
Part 2 - mozbuild support for rustc under Makefiles (2.89 KB, patch)
2015-05-04 17:08 PDT, Ralph Giles (:rillian) | needinfo me
no flags Details | Diff | Splinter Review
Part 3 - test code WIP (1.40 KB, patch)
2015-05-04 17:09 PDT, Ralph Giles (:rillian) | needinfo me
no flags Details | Diff | Splinter Review
Part 1 - Check for rustc in configure v2 (1.39 KB, patch)
2015-05-05 10:48 PDT, Ralph Giles (:rillian) | needinfo me
ted: review+
Details | Diff | Splinter Review
Part 2 - mozbuild support for rustc under Makefiles v2 (2.90 KB, patch)
2015-05-05 10:49 PDT, Ralph Giles (:rillian) | needinfo me
no flags Details | Diff | Splinter Review
Part 3 - test code WIP v2 (1.40 KB, patch)
2015-05-05 10:50 PDT, Ralph Giles (:rillian) | needinfo me
no flags Details | Diff | Splinter Review
Part 2 - mozbuild support for rustc under Makefiles v3 (3.81 KB, patch)
2015-05-06 12:18 PDT, Ralph Giles (:rillian) | needinfo me
ted: review+
Details | Diff | Splinter Review
Part 3 - test code WIP v3 (1.42 KB, patch)
2015-05-06 12:20 PDT, Ralph Giles (:rillian) | needinfo me
no flags Details | Diff | Splinter Review
Part 3 - gtest to verify compilation and linkage v4 (1.45 KB, patch)
2015-05-07 12:15 PDT, Ralph Giles (:rillian) | needinfo me
cajbir.bugzilla: review+
Details | Diff | Splinter Review
Part 2 - mozbuild support for rustc under Makefiles v4 (3.86 KB, patch)
2015-05-08 15:18 PDT, Ralph Giles (:rillian) | needinfo me
no flags Details | Diff | Splinter Review

Description User image Ralph Giles (:rillian) | needinfo me 2015-05-04 16:51:44 PDT
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.
Comment 1 User image Ralph Giles (:rillian) | needinfo me 2015-05-04 17:03:15 PDT
Created attachment 8601214 [details] [diff] [review]
Part 1 - Check for rustc in configure.

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.
Comment 2 User image Ralph Giles (:rillian) | needinfo me 2015-05-04 17:08:04 PDT
Created attachment 8601216 [details] [diff] [review]
Part 2 - mozbuild support for rustc under Makefiles

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.
Comment 3 User image Ralph Giles (:rillian) | needinfo me 2015-05-04 17:09:10 PDT
Created attachment 8601217 [details] [diff] [review]
Part 3 - test code WIP

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 User image Mike Hommey [:glandium] 2015-05-04 17:10:26 PDT
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 User image Gregory Szorc [:gps] (away until 2017-03-20) 2015-05-04 21:03:22 PDT
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 User image Mike Hommey [:glandium] 2015-05-04 21:42:21 PDT
(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 User image Gregory Szorc [:gps] (away until 2017-03-20) 2015-05-04 21:50:09 PDT
(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 User image Mike Hommey [:glandium] 2015-05-04 23:03:53 PDT
(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 User image :Ms2ger (⌚ UTC+1/+2) 2015-05-05 06:46:27 PDT
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 User image Mike Hommey [:glandium] 2015-05-05 06:56:58 PDT
(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...
Comment 11 User image Ralph Giles (:rillian) | needinfo me 2015-05-05 08:29:13 PDT
(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 User image :Ms2ger (⌚ UTC+1/+2) 2015-05-05 10:26:34 PDT
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 User image Gregory Szorc [:gps] (away until 2017-03-20) 2015-05-05 10:42:39 PDT
(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.
Comment 14 User image Ralph Giles (:rillian) | needinfo me 2015-05-05 10:48:30 PDT
Created attachment 8601613 [details] [diff] [review]
Part 1 - Check for rustc in configure v2

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.
Comment 15 User image Ralph Giles (:rillian) | needinfo me 2015-05-05 10:49:42 PDT
Created attachment 8601614 [details] [diff] [review]
Part 2 - mozbuild support for rustc under Makefiles v2

Update build rules to propagate the new MOZ_RUST conditional in place of RUSTC.
Comment 16 User image Ralph Giles (:rillian) | needinfo me 2015-05-05 10:50:17 PDT
Created attachment 8601615 [details] [diff] [review]
Part 3 - test code WIP v2

Same for build-test patch.
Comment 17 User image Ralph Giles (:rillian) | needinfo me 2015-05-05 12:35:41 PDT
(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.
Comment 18 User image Ralph Giles (:rillian) | needinfo me 2015-05-06 12:18:19 PDT
Created attachment 8602255 [details] [diff] [review]
Part 2 - mozbuild support for rustc under Makefiles v3

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.
Comment 19 User image Ralph Giles (:rillian) | needinfo me 2015-05-06 12:20:31 PDT
Created attachment 8602257 [details] [diff] [review]
Part 3 - test code WIP v3

Import rust code as 'extern "C"'. This resolves the remaining linking problem I was having. Thanks to mbrubeck for help with this issue.
Comment 20 User image Ralph Giles (:rillian) | needinfo me 2015-05-07 12:15:05 PDT
Created attachment 8602917 [details] [diff] [review]
Part 3 - gtest to verify compilation and linkage v4
Comment 21 User image Ralph Giles (:rillian) | needinfo me 2015-05-07 12:16:30 PDT
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.*
Comment 22 User image Ted Mielczarek [:ted.mielczarek] 2015-05-08 04:37:22 PDT
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.
Comment 23 User image Ted Mielczarek [:ted.mielczarek] 2015-05-08 12:39:54 PDT
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.
Comment 24 User image :Ms2ger (⌚ UTC+1/+2) 2015-05-08 14:43:53 PDT
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.
Comment 25 User image Ralph Giles (:rillian) | needinfo me 2015-05-08 14:48:47 PDT
(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 User image Mike Hommey [:glandium] 2015-05-08 14:50:07 PDT
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.
Comment 27 User image Ralph Giles (:rillian) | needinfo me 2015-05-08 15:18:46 PDT
Created attachment 8603602 [details] [diff] [review]
Part 2 - mozbuild support for rustc under Makefiles v4

Update makefile to set an explicit output name. Carrying forward r=ted.

Note You need to log in before you can comment on or make changes to this bug.