1293253 - Add ability to build Rust binaries alongside Firefox

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[James Graham [:jgraham]]

We have written [1] in Rust which we would like to move in-tree to test alongside nightly Gecko (and use it as a harness component for other tests). Therefore it would be ideal to build it as a test-only artifact like xpcshell or certutil or similar.

[1] https://github.com/mozilla/geckodriver

Comment 1

[Gabriele Svelto [:gsvelto]]

We're planning on implementing a separate executable to send telemetry crash pings during Firefox startup crashes. We'd like to do it in Rust since we've got a long term plan to replace most of the (crufty) crash reporting tools in Rust. One of the reasons to do this is that by definition crash reporting tools should be reliable and Rust is a perfect fit for this.

Do we already have an estimate on how long this might take? I've done only little work on the build system a couple of years ago but if there's no one else available to do it I'm willing to pick this up myself.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Gabriele Svelto [:gsvelto] from comment #1)
> Do we already have an estimate on how long this might take? I've done only
> little work on the build system a couple of years ago but if there's no one
> else available to do it I'm willing to pick this up myself.

I can do this; I think it's just a handful of lines of code.  Do you have any sort of working Rust binary that could be integrated right now, even if it's incomplete?  Past experience has shown that concrete use-cases for these sorts of things make the code better.

Comment 3

[Gabriele Svelto [:gsvelto]]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> I can do this; I think it's just a handful of lines of code.  Do you have
> any sort of working Rust binary that could be integrated right now, even if
> it's incomplete?  Past experience has shown that concrete use-cases for
> these sorts of things make the code better.

I've just started working on it so I don't have anything ready yet. James might have something useful though since the code he pointed to in comment 0 seems fully functional.

Comment 4

[James Graham [:jgraham]]

Yes, geckodriver in comment 0 is something that's already in the hands of end users, and currently gets pulled in for tests via tooltool, so it's a pretty concrete example for at least some use cases (the characteristics of geckodriver are similar to those of e.g. certutil i.e. it's a binary that is required during test, and which is used by some end users, but which does not get packaged and shipped with normal firefox releases).

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 1 - modify existing Rust library build rules to be explicitly about libraries

In preparation for a world where we have cargo building binaries too,
the existing rules should be renamed to reflect their library
associations.  The lone Cargo invocation should be updated to explicitly
build libraries only, so libraries and binaries in the same directory
will work correctly.

Fixup a leftover comment from a previous set of changes while we're here.

If you're wondering where the $(rustflags_override) bit comes from; it's from
bug 1300835, which ideally will be landing soonish.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 2 - split out cargo_target_directory

We're going to need this functionality for Rust programs as well as Rust
libraries, so we might as well move the functionality out of RustLibrary
into a separate function.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 3 - add configure support for determining a --target value for Rust host code

We'll need this for compiling host binaries.  We could just call `rustc`
without any --target value whatsoever, but since we use --target for
target code, we might as well be consistent and explicit, and use
--target for host code as well.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 4 - add frontend objects for Rust programs

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 5 - move cargo_build_flags determination out of RUST_LIBRARY_FILE block

We need cargo_build_flags not just for Rust libraries now, but for Rust
programs as well.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 6 - add build and installation rules for {HOST_,}RUST_PROGRAMS

The only complicating factor here is having to split out the --target
flag from cargo_build_flags, so we can pass the appropriate one
depending on our build target.

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: part 7 - factor out a Cargo.toml-for-context parsing function

We'll need a function to do this for Rust program definitions, so we
might as well use one that's already there for Rust library definitions.

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: part 8 - factor out a unique binary check function

We'll need this for Rust programs as well.

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: part 9 - handle {Host,}RustProgram objects in the recursivemake backend

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: part 10 - emit {Host,}RustProgram objects from the frontend

Comment 15

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8812264 [details] [diff] [review]
part 2 - split out cargo_target_directory

Review of attachment 8812264 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/frontend/data.py
@@ +428,5 @@
> +    # on the target, but also what sort of build we are doing.
> +    rust_build_kind = 'release'
> +    if context.config.substs.get('MOZ_DEBUG'):
> +        rust_build_kind = 'debug'
> +    return mozpath.join(context.config.substs['RUST_TARGET'], rust_build_kind)

Now that we're extracting this and are going to have a use for it in multiple places, this looks a little out of place here. The definitions in this seem to be intended as simple containers, consider moving this logic to the emitter.

Comment 16

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8812265 [details] [diff] [review]
part 3 - add configure support for determining a --target value for Rust host code

Review of attachment 8812265 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/moz.configure/rust.configure
@@ +81,2 @@
>  
> +    @depends(rust_compiler, rustc, host_or_target)

This can be depends_when(..., when=rust_compiler), which will make the check conditional without checking the value of rust_compiler explicitly below.

@@ +169,5 @@
> +
> +    return rust_target
> +
> +rust_target_target = rust_target_alias(target)
> +rust_host_target = rust_target_alias(host)

Seeing RUST_TARGET, rust_target_target and rust_host_target all at once here starts to get confusing. Can we call these "rust_target_triple"/"rust_host_triple", and our template "rust_triple_alias"?

Comment 17

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8812268 [details] [diff] [review]
part 6 - add build and installation rules for {HOST_,}RUST_PROGRAMS

Review of attachment 8812268 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/rules.mk
@@ +965,5 @@
> +endif # RUST_PROGRAMS
> +ifdef HOST_RUST_PROGRAMS
> +force-cargo-host-program-build:
> +	$(REPORT_BUILD)
> +	env $(rustflags_override) CARGO_TARGET_DIR=. RUSTC=$(RUSTC) $(CARGO) build $(addprefix --bin ,$(HOST_RUST_CARGO_PROGRAMS)) $(cargo_build_flags) $(cargo_host_flag) --

Someone with better Make skills than me might be able to suggest a clever way to use fewer variables and rules here, the duplication here isn't great.

Comment 18

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8812269 [details] [diff] [review]
part 7 - factor out a Cargo.toml-for-context parsing function

Review of attachment 8812269 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +501,5 @@
>                      context)
>  
>      def _rust_library(self, context, libname, static_args):
>          # We need to note any Rust library for linking purposes.
> +        (config, cargo_file) = self._parse_cargo_file(context)

No parens necessary around `config, cargo_file` here.

Comment 19

[Nathan Froyd [:froydnj]]

(In reply to Chris Manchester (Offline Nov 22-28 :chmanchester) from comment #17)
> Comment on attachment 8812268 [details] [diff] [review]
> part 6 - add build and installation rules for {HOST_,}RUST_PROGRAMS
> 
> Review of attachment 8812268 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/rules.mk
> @@ +965,5 @@
> > +endif # RUST_PROGRAMS
> > +ifdef HOST_RUST_PROGRAMS
> > +force-cargo-host-program-build:
> > +	$(REPORT_BUILD)
> > +	env $(rustflags_override) CARGO_TARGET_DIR=. RUSTC=$(RUSTC) $(CARGO) build $(addprefix --bin ,$(HOST_RUST_CARGO_PROGRAMS)) $(cargo_build_flags) $(cargo_host_flag) --
> 
> Someone with better Make skills than me might be able to suggest a clever
> way to use fewer variables and rules here, the duplication here isn't great.

I think what I am going to wind up doing in another bug is adding some sort of invoke-cargo-build "function" that reduces some of the duplication.  We're going to need something like that to avoid modifying three places everytime we want to pass a new environment variable to Cargo anyway.

Comment 20

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8812272 [details] [diff] [review]
part 10 - emit {Host,}RustProgram objects from the frontend

Review of attachment 8812272 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/frontend/context.py
@@ +1347,5 @@
> +
> +        Each name in this variable corresponds to an executable built from
> +        the Cargo.toml in the same directory.
> +        """),
> +                      

Leading white space.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +591,5 @@
> +            all_rust_programs.append((programs, kind, cls))
> +
> +        # Verify Rust program definitions.
> +        if all_rust_programs:
> +            (config, cargo_file) = self._parse_cargo_file(context);

Parens not necessary.

@@ +600,5 @@
> +                    context)
> +
> +            defined_binaries = {b['name'] for b in bin_section}
> +
> +            for (programs, kind, cls) in all_rust_programs:

Parens not necessary.

Comment 21

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8812271 [details] [diff] [review]
part 9 - handle {Host,}RustProgram objects in the recursivemake backend

Review of attachment 8812271 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1045,5 @@
>  
>      def _process_host_program(self, program, backend_file):
>          backend_file.write('HOST_PROGRAM = %s\n' % program)
>  
> +    def _process_rust_program_1(self, obj, backend_file,

This is screaming for a better function name.

Comment 22

[Chris Manchester (limited bugmail, email directly)]

(In reply to Nathan Froyd [:froydnj] from comment #19)
> (In reply to Chris Manchester (Offline Nov 22-28 :chmanchester) from comment
> #17)
> > Comment on attachment 8812268 [details] [diff] [review]
> > part 6 - add build and installation rules for {HOST_,}RUST_PROGRAMS
> > 
> > Review of attachment 8812268 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: config/rules.mk
> > @@ +965,5 @@
> > > +endif # RUST_PROGRAMS
> > > +ifdef HOST_RUST_PROGRAMS
> > > +force-cargo-host-program-build:
> > > +	$(REPORT_BUILD)
> > > +	env $(rustflags_override) CARGO_TARGET_DIR=. RUSTC=$(RUSTC) $(CARGO) build $(addprefix --bin ,$(HOST_RUST_CARGO_PROGRAMS)) $(cargo_build_flags) $(cargo_host_flag) --
> > 
> > Someone with better Make skills than me might be able to suggest a clever
> > way to use fewer variables and rules here, the duplication here isn't great.
> 
> I think what I am going to wind up doing in another bug is adding some sort
> of invoke-cargo-build "function" that reduces some of the duplication. 
> We're going to need something like that to avoid modifying three places
> everytime we want to pass a new environment variable to Cargo anyway.

Sounds reasonable. A lot of this is devoted to figuring out what flags to pass to cargo, I wonder how much of that we could set up in a backend.mk automatically. This could help out other backends when the time comes as well.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03549e92001d
part 1 - modify existing Rust library build rules to be explicitly about libraries; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/779d75b98fc5
part 2 - split out cargo_target_directory; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/009cffc985ab
part 3 - add configure support for determining a --target value for Rust host code; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/5586b76ab2a4
part 4 - add frontend objects for Rust programs; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc2b70ec392b
part 5 - move cargo_build_flags determination out of RUST_LIBRARY_FILE block; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d91224e1389
part 6 - add build and installation rules for {HOST_,}RUST_PROGRAMS; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcc7e0fd607c
part 7 - factor out a Cargo.toml-for-context parsing function; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3505dd036cf
part 8 - factor out a unique binary check function; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/27c257be4c2e
part 9 - handle {Host,}RustProgram objects in the recursivemake backend; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/09f249970fc3
part 10 - emit {Host,}RustProgram objects from the frontend; r=chmanchester

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03549e92001d
https://hg.mozilla.org/mozilla-central/rev/779d75b98fc5
https://hg.mozilla.org/mozilla-central/rev/009cffc985ab
https://hg.mozilla.org/mozilla-central/rev/5586b76ab2a4
https://hg.mozilla.org/mozilla-central/rev/bc2b70ec392b
https://hg.mozilla.org/mozilla-central/rev/0d91224e1389
https://hg.mozilla.org/mozilla-central/rev/fcc7e0fd607c
https://hg.mozilla.org/mozilla-central/rev/b3505dd036cf
https://hg.mozilla.org/mozilla-central/rev/27c257be4c2e
https://hg.mozilla.org/mozilla-central/rev/09f249970fc3