1231764 - Support cargo in the gecko build system

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Ralph Giles (:rillian)]

This is a tracking bug for supporting the rust ecosystem's `cargo` build and dependency tool from the gecko build system.

Comment 1

[Ralph Giles (:rillian)]

Follow-up from today's meeting. Cargo is the native package manager for rust code, and it would be convenient to be able to use it for gecko development.

We'll need a way to share code between gecko, servo, and other projects. This is intended to help: Any implementation needs dependency declarations between rust crates. Cargo already documents this in a standard way.

In general, we'd like it to be easy to use crates.io during development, but have tooling to help embed that code in m-c so we get complete builds. Or to go the other way: patch the m-c version and make it easy to update published crates.io package, an upstream repo used by other projects.


Issues:

- How do we refer to creates in moz.build? `CRATES = foo` where foo is a directory containing a Cargo.toml?

- It sounds like we want a top level gk_rust crate we use cargo to build a staticlib from, and like that into XUL. We'd want to teach mozbuild to take the representation above and codegen this crate source in the object tree.

- Alternative (might be useful in the short term) is collecting deps and linking the rust code ourselves. Bug 1163224.

- How do we vendor dependencies? Can cargo help with this?

Alex is working on a proposal to have cargo download and package up dependencies in some kind of hash-identified bundle. Could we use this somehow, with tooltool or mozharness taking care of the bundle download for integration builds?

Cargo.toml can point to a path for dependencies, which is a way to use in-tree source. Need tooling to maintain this.

Cargo.lock is intended for automated modification, can check this into version control to fix revisions. Could be better for re-directing to vendored source, but needs cargo extension.

Comment 2

[Lars Bergstrom (:larsberg)]

One other alternative I was wondering about was if we provided a way to internally mirror crates.io that could sit next to the mercurial repo and the Gaia github mirrors. If I understand correctly, Gaia works similarly (that is - GitHub is where code is landed, but the internal mirror is how a Firefox OS build is generated).

A setup like that would allow Gecko developers to use the identical cargo workflow while working on m-c, but then when official builds or tests are run, they would be performed against the internal cache.

One tricky piece might be `try` builds, as if they are similarly cut off from the internet, then developers might have to push their pending changes to crates.io, which seems suboptimal. Today, people test polyrepo changes by pushing each of their subrepo changes to branches in each of their personal GitHub forks, and then have a redirect (aka "`paths` override) to force the main repo to run against each of those personal fork branches.

Added :wycats for Cargo-related feedback/insight.

Comment 3

[Mike Hommey [:glandium]]

That wouldn't help the case of Linux distros, who usually take self-contained source tarballs (which are essentially a copy of the repository at the time of the build + configure + js/src/configure) and build them on build machines without network access.

Comment 4

[Alex Crichton [:acrichto]]

I'd personally highly recommend enabling Cargo usage in the Gecko build as so much Rust code in general relies on various dependencies in crates.io, and Cargo makes it quite easy to fetch and compile them. I suspect that Gecko will benefit greatly from being able to tap into the crates.io ecosystem, and it's generally the best "bang for your buck" when writing Rust.

Rust crates today typically end up depending on a number of crates from crates.io pretty quickly as well, so I suspect that it may get to the point that a dozen or so dependencies from crates.io are needed for the Gecko build (and this may grow over time as well). In terms of how these crates are treated, I would *highly* recommend the mentality that crates.io is the "source of truth" for the crate. In that sense, I would discourage local modifications within Gecko that aren't submitted back upstream. This may become more relevant over time as Gecko picks up more Servo packages, but at least for the core dependencies from crates.io (e.g. a url parser) I think it's especially important that the version Gecko uses doesn't diverge from upstream.

Now in terms of where everything is actually stored, that's a different story! I think today that Cargo probably doesn't have a mode of operation that meshes well enough with what Gecko does, so we'd be blazing some new frontiers there (and hence have a number of options):

- First, Gecko could simply use crates.io and Cargo as usual. Dependencies would be fetched as necessary and the normal Rust workflows would work as expected. The downside of this, however, is that many build services don't have network access, so the "fetch dependencies" step would have to be moved up to the point of where the source tarball is created (or the tree is checked out).

- If Gecko used vanilla crates.io, it would also be possible to add a new subcommand to Cargo which would have two modes of operation. The command would first build a binary blob from a Cargo.lock (a Cargo dependency graph) which is a completely self-contained copy of the ecosystem. Later on the binary blob could be fed back into the subcommand at which point it will compile the entire Cargo graph. This is similar to the bullet above but provides a more principled method of "fetch all dependencies" during the "download the source code" step.

- When using vanilla crates.io, the Gecko build servers (and perhaps others) may be able to use a crates.io mirror which contains everything Gecko wants. This may not end up working in the long run, however.

- Instead of looking at crates.io, Gecko could instead vendor dependencies in-tree. There are a number of possibilities of how to inform Cargo of these vendored dependencies, but the most likely of which to be implemented probably looks like https://github.com/rust-lang/cargo/issues/2212. We've created that issue for packaging Rust software in Debian, and the idea is that there would be a canonical layout of a directory where rust packages can come from, which Gecko would then manage. The interaction with crates.io in this case is unclear, for example `cargo update` probably won't "just work" as it would with the solutions above.


I suspect that those are not an exhaustive list of options either! I would personally recommend trying to stay as close as possible to idiomatic Rust, which would involve using crates.io wherever possible. Constraints such as network access or release tarballs I believe can be solved through other means (which Cargo probably wants regardless), and sticking to idiomatic Rust means things like "cargo update", pulling in external Rust, and helping new contributors write Rust will typically just work without any caveats.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

I did some investigation of using Cargo in Gecko, and posted about it on dev-servo [1].

It seems that in-tree vendoring of crates is already possible using a .cargo/config file in m-c, which specifies the paths to the crates, and sets a timeout = 0 which causes fetching non-local dependencies to fail. [2]
Further work might be needed to avoid trying to update the index and to make sure HTTP requests aren't made at all for timeout = 0.

I don't know if using crates.io would be possible. I suspect even getting the dependencies for rust-url into the tree might be difficult. There's also the understanding that fetching m-c would be enough to build Gecko.

Ralph, do you know if the Cargo binary is also available to tooltool on the try servers, or what we'd need to do to get it there? My hope is that we'd only use Cargo.toml for configuration, and that moz.build would only mark for which crates we need to call `cargo build` (+optional args). I see it as the simplest and most flexible approach, and makes it easy to stay in sync with upstream.

[1] https://groups.google.com/forum/#!topic/mozilla.dev.servo/qESJoCIsPU8
[2] https://github.com/rust-lang/cargo/issues/1926

Comment 6

[(not currently active) Ted Mielczarek]

I've been mentioning this a lot, and I mentioned it in the oxidation meeting in London last week, but I think we should switch from invoking rustc to invoking cargo ASAP. cargo is the standard Rust developer workflow and we should make it as easy as possible to integrate existing Rust code into Gecko.

After talking to wycats and acrichto, I think our first step would be to move to structuring our in-tree Rust code as crates with Cargo.toml files, with any dependencies manually vendored into mozilla-central and using path dependencies in Cargo.toml. We'll probably wind up just running cargo for every build, even if we don't need to, but I think we can live with that for a short while.

The next step after that would be to use the `cargo vendor` tool to manage our vendored dependencies when that becomes available, so that we don't have to change any Cargo.toml files to use path dependencies, we can just use normal crates.io-style dependencies.

Comment 7

[(not currently active) Ted Mielczarek]

I think bug 1282129 is the only hard blocker to this work--we should have some backstop to ensure that we don't accidentally introduce a dependency on fetching an external crate from crates.io to the build.

Comment 8

[(not currently active) Ted Mielczarek]

My bikeshed proposal for what this would look like in moz.build is:
```
RUST_CRATES += ['rust-url']
```

where 'rust-url' is a path to a source directory containing a Cargo.toml file. We'd build that as an rlib and link it into Gecko similar to how we are currently doing things.

The part I'm least sure about is how we tie things into specific libraries. We could have a single `RUST_CRATE` in toolkit/library, and have its Cargo.toml specify path dependencies to all other Rust code in the tree, and then have a single cargo invocation build all the Rust code in one fell swoop, but that doesn't match the way we specify other things in the build system very well. We could have a `RUST_CRATE` per arbitrary level of componentization, like `rust-url` might have its own, and `stylo` might have its own, and then the build system could generate a crate with a Cargo.toml that specifies a path dependency for each of those, and we invoke cargo one time to build that. That's probably fairly sensible, and it's not too far off what we're doing now, which is generating a crate via the build system (without Cargo.toml) and invoking rustc on it directly, pointing to the other rlibs the build system has generated.

Comment 9

[Ralph Giles (:rillian)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)

> RUST_CRATES += ['rust-url']

I would name the bikeshed `CRATES`. Simpler and the fact that they're rust code will eventually become obvious. 

> The part I'm least sure about is how we tie things into specific libraries.

I think we should generate a staticlib target crate for each FINAL_LIBRARY or executable target any SOURCES entries in the same moz.build would end up in and link the CRATES entries into that. Note that we already have separate xul and xul-gtest targets with the current rust code.

> We could have a single `RUST_CRATE` in toolkit/library, and have its
> Cargo.toml specify path dependencies to all other Rust code in the tree, and
> then have a single cargo invocation build all the Rust code in one fell
> swoop, but that doesn't match the way we specify other things in the build
> system very well. We could have a `RUST_CRATE` per arbitrary level of
> componentization, like `rust-url` might have its own, and `stylo` might have
> its own, and then the build system could generate a crate with a Cargo.toml
> that specifies a path dependency for each of those, and we invoke cargo one
> time to build that. That's probably fairly sensible, and it's not too far
> off what we're doing now, which is generating a crate via the build system
> (without Cargo.toml) and invoking rustc on it directly, pointing to the
> other rlibs the build system has generated.

Comment 10

[Simon Sapin (:SimonSapin)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> with any dependencies manually vendored into mozilla-central and using path
> dependencies in Cargo.toml.

Do you mean something like this?

  [dependencies]
  url = {path = "path/to/rust-url"}

That means modifying the Cargo.toml file of all non-leaf recursive dependencies, which is one more thing to do when merging changes to/from upstream.

Cargo provides two out-of-band alternatives: path overrides and replacements.

With path overrides http://doc.crates.io/specifying-dependencies.html#overriding-dependencies you give a list of directories in .cargo/config, and whatever Cargo.toml files are found there are used instead of any dependency with a crate name. They’re kinda hacky and fragile.

Replacements (aka top-level overrides) https://github.com/rust-lang/cargo/pull/2385 were introduced recently and are less of a hack in that they’re reflected in Cargo.lock. You specify in a top-level Cargo.toml file a list of (name, version) pairs to be replaced with some other arbitrary source (which can be a path dependency).

Comment 11

[(not currently active) Ted Mielczarek]

(In reply to Simon Sapin (:SimonSapin) from comment #10)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> > with any dependencies manually vendored into mozilla-central and using path
> > dependencies in Cargo.toml.
> 
> Do you mean something like this?
> 
>   [dependencies]
>   url = {path = "path/to/rust-url"}
> 
> That means modifying the Cargo.toml file of all non-leaf recursive
> dependencies, which is one more thing to do when merging changes to/from
> upstream.

Yeah, this is not great but it's only intended to be a temporary measure until we get `cargo vendor` sorted out. Using Cargo.toml files, even with hand-editing would still be an improvement over our current situation.

Comment 12

[Alex Crichton [:acrichto]]

Eventually I believe Gecko will still want to create one monolithic "staticlib" of all Rust code being included. Right now that's the only (er, best) way to avoid the Rust libstd from being included multiple times by accident (causing errors and/or binary bloat). In general though Cargo isn't designed to be run multiple times as part of a build, so it'll be pretty difficult for one invocation of Cargo to use artifacts from another.

Along those lines I'd personally recommend a top-level Cargo.toml which emits a staticlib, although perhaps this Cargo.toml could get generated at build time depending on the values of RUST_CRATES for each module? (sorry, I'm pretty unfamiliar with Gecko's build system)

Comment 13

[Nika Layzell [:nika] (ni? for response)]

(In reply to Alex Crichton [:acrichto] from comment #12)
> Eventually I believe Gecko will still want to create one monolithic
> "staticlib" of all Rust code being included. Right now that's the only (er,
> best) way to avoid the Rust libstd from being included multiple times by
> accident (causing errors and/or binary bloat). In general though Cargo isn't
> designed to be run multiple times as part of a build, so it'll be pretty
> difficult for one invocation of Cargo to use artifacts from another.
> 
> Along those lines I'd personally recommend a top-level Cargo.toml which
> emits a staticlib, although perhaps this Cargo.toml could get generated at
> build time depending on the values of RUST_CRATES for each module? (sorry,
> I'm pretty unfamiliar with Gecko's build system)

I believe that the current build system generates a toplevel crate which then includes each of the subcrates which are built as rlibs and defined from the RUST_CRATES in moz.build. The new system could do the same thing, except instead work by generating a toplevel Cargo crate which refers to the cargo crates listed in RUST_CRATES.

Comment 14

[(not currently active) Ted Mielczarek]

Yeah, that's correct. Thanks, we'll do that. We could have a top-level crate in-tree, but that doesn't jive very well with the way the existing build system defines things. I think we'll try adding `CRATES = [...]` and see how that works out.

Comment 15

[(not currently active) Ted Mielczarek]

I'm not going to have time to do this right now, but I'd really like to see it done. I'd be happy to mentor someone else to do the work.

Comment 16

[Nathan Froyd [:froydnj]]

David asked me to look at this, and Ted indicated that he would be OK not mentoring this bug.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11)
> (In reply to Simon Sapin (:SimonSapin) from comment #10)
> > (In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> > > with any dependencies manually vendored into mozilla-central and using path
> > > dependencies in Cargo.toml.
> > 
> > Do you mean something like this?
> > 
> >   [dependencies]
> >   url = {path = "path/to/rust-url"}
> > 
> > That means modifying the Cargo.toml file of all non-leaf recursive
> > dependencies, which is one more thing to do when merging changes to/from
> > upstream.
> 
> Yeah, this is not great but it's only intended to be a temporary measure
> until we get `cargo vendor` sorted out. Using Cargo.toml files, even with
> hand-editing would still be an improvement over our current situation.

What is this mythical `cargo vendor` of which you speak?  Is this the "given these cargo specs, download the world and throw it into $DIRECTORY"?

I was trying to avoid introducing CRATES, but given cargo's insistence on package names matching up between a Cargo.toml file for $PACKAGE and a Cargo.toml file that uses $PACKAGE, we can't use the automatically-generated package names we were using before.  So, would people prefer:

1) Only specify Cargo.toml paths in CRATES, and have moz.build parse out the package names:

CRATES += [ "Cargo.toml", "other-crate/Cargo.toml" ]

2) Require the package names to be specified in CRATES along with the paths to Cargo.toml files:

CRATES += [ ("package", "Cargo.toml"), ("whizbang", "other-crate/Cargo.toml") ]

Option (1) is all single-source-of-truthy, but option (2) requires less fiddling in the build system itself.

Preferences, anybody?

Comment 18

[Ralph Giles (:rillian)]

(In reply to Nathan Froyd [:froydnj] from comment #17)

> What is this mythical `cargo vendor` of which you speak?  Is this the "given
> these cargo specs, download the world and throw it into $DIRECTORY"?

I'm not Simon, but yes. And then it generates some file that either acts as a local static registry, or overrides the crates.io source locations referenced implicitly from Cargo.toml. There's an experiment available at https://github.com/alexcrichton/cargo-vendor

> 1) Only specify Cargo.toml paths in CRATES, and have moz.build parse out the
> package names:
> 
> CRATES += [ "Cargo.toml", "other-crate/Cargo.toml" ]

No. Cargo imposes a standard source layout. We should have moz.build parse out the package names, and assume Cargo.toml locations compatible with cargo's assumptions.

  CRATES += [ 'foo', 'helper/subdir/bar' ]

where moz.build will expect there to exist `foo/Cargo.toml` and `helper/subdir/bar/Cargo.toml`, will parse these files for the actual library names. Top-level source for libfoo.rlib would be in `foo/src/lib.rs` or similar.

Anything else involves redundant typing and/or violates least surprise when interacting with the rust ecosystem.

Comment 19

[(not currently active) Ted Mielczarek]

Yes, CRATES should specify a path to a directory containing a Cargo.toml file, and we should parse that (or ask cargo to spit out information, whatever is easiest).

Comment 20

[Nathan Froyd [:froydnj]]

(In reply to Ralph Giles (:rillian) needinfo me from comment #18)
> (In reply to Nathan Froyd [:froydnj] from comment #17)
> 
> > What is this mythical `cargo vendor` of which you speak?  Is this the "given
> > these cargo specs, download the world and throw it into $DIRECTORY"?
> 
> I'm not Simon, but yes. And then it generates some file that either acts as
> a local static registry, or overrides the crates.io source locations
> referenced implicitly from Cargo.toml. There's an experiment available at
> https://github.com/alexcrichton/cargo-vendor
> 
> > 1) Only specify Cargo.toml paths in CRATES, and have moz.build parse out the
> > package names:
> > 
> > CRATES += [ "Cargo.toml", "other-crate/Cargo.toml" ]
> 
> No. Cargo imposes a standard source layout. We should have moz.build parse
> out the package names, and assume Cargo.toml locations compatible with
> cargo's assumptions.
> 
> Anything else involves redundant typing and/or violates least surprise when
> interacting with the rust ecosystem.

I have no objections to using Cargo's layout; I think specifying things as directories is pretty unusual in the build system, though.

Comment 21

[Ralph Giles (:rillian)]

(In reply to Nathan Froyd [:froydnj] from comment #20)

> I have no objections to using Cargo's layout; I think specifying things as
> directories is pretty unusual in the build system, though.

It's unusual for sources, but typing CRATES += ['crate/Cargo.toml'] doesn't make any more sense than DIRS += [ 'config/moz.build' ] or TEST_DIRS += [ 'gtest/moz.build' ]. Like with moz.build, the unit of compilation in rust is a filesystem subtree described by a build control file, which I think justifies the variance from C++ or other per-file languages.

Comment 22

[Nathan Froyd [:froydnj]]

Attachment: part 0 - only link librul once

Through an oversight, we listed librul.a twice when linking libxul: once
as part of the "objects" we were linking, and once as a static library.
This duplication is unnecessary and would cause problems later when we
try to generate librul.a via cargo, as cargo will put it someplace
different from where we expect and the two names will conflict.  Let's
have rules.mk be the single source of truth for how librul.a is named,
and then the code to link libxul can simply refer to that name.

Comment 23

[Nathan Froyd [:froydnj]]

Attachment: part 1 - search for cargo when --enable-rust

We're going to use it, the location of it should be configurable.

Comment 24

[Nathan Froyd [:froydnj]]

Attachment: part 2 - pass the srcdir of an rlib in the moz.build object

We're going to need this when we write out libxul's Cargo.toml so we can point
cargo to the correct srcdir locations of the dependent crates.

Comment 25

[Nathan Froyd [:froydnj]]

Attachment: part 3 - rename RustRlibLibrary to RustCrate

rlibs are going to be less important once we start building with cargo:
the focus will move to crates instead, so that's what we should call the
moz.build object.

Comment 26

[Nathan Froyd [:froydnj]]

Attachment: part 4 - remove rlib_filename from RustCrate

This member was only needed so we could tell rustc where to look for
rlibs that it would link into the final staticlib.  With cargo, cargo
handles all those details for us...and we wouldn't even know the name of
the rlib anyway due to the compilation hash rustc sticks in the name.

Comment 27

[Nathan Froyd [:froydnj]]

Attachment: part 5 - add pytoml to the virtualenv

We need to parse Cargo.toml files from moz.build to determine crate
package names.

I'm not sure if we have any restrictions on what sort of licenses the libraries
we use need to have; I figure the MIT license is permissive enough.

Comment 28

[Nathan Froyd [:froydnj]]

Attachment: part 6 - add a Cargo.toml for the dom/media/gtest/ code

We are going to need a Cargo.toml for all Rust code in the tree, no
matter how trivial.

Comment 29

[Nathan Froyd [:froydnj]]

Attachment: part 7 - build rust code via cargo

This patch is really two separate changes.

The first is that rust crates are large, standalone entities that may
contain multitudes of source files.  It therefore doesn't make sense to
keep them in SOURCES, as we have been doing.  Moving to use cargo will
requiring knowing actual names of the crates we are compiling, anyway,
which suggests that we need a different, higher-level representation for
Rust sources in the build system.  CRATES it is, then.

The second is that we switch the core build system over to building via
cargo, rather than invoking rustc directly.

This depends on bug 1284589 and a yet-to-be-written bug that edits mp4parse's
Cargo.toml to cut out its build dependency on rusty-cheddar.  This patch also
doesn't include support for cargo's "don't hit the network" flag, which
probably ought to be added...In any event, this patch set works well enough for
local builds with --enable-rust to build with cargo.

We do regress slightly in terms of exposing dependencies to make because of
bugs in cargo such as https://github.com/rust-lang/cargo/issues/2559 .  But
theoretically cargo knows all the dependencies here, so invoking cargo every
build should be OK.  $OBJDIR/toolkit/library/ is also somewhat messier thanks
to cargo creating a large network of directories.

Comment 30

[Nathan Froyd [:froydnj]]

Oh, all this also depends on cargo-in-tooltool and modifying the appropriate mozconfigs to point at the cargo we're going to use.  The latter will be another patch that will be committed along with part 7.

Comment 31

[Nathan Froyd [:froydnj]]

Oh, we're also now batching up compilation of all the Rust code when we get ready to compile libxul.  I don't know how much of a problem this is in practice, because we need to run LTO anyway to get rid of unused Rust code.  But this part of the build is now a bit more serial than it would be otherwise, even though we only have a small amount of code, so it doesn't matter much at the moment.

Comment 32

[Ralph Giles (:rillian)]

Attachment: Part 7a - Caseshift '-' to '_' in crate names

Fixup patch to make `./mach gtest rust.*` work.

dom/media/gtest/Cargo.toml declares a crate named 'mp4parse-gtest'. Which is fine, but rust doesn't allow hyphens in identifiers, to cargo converts this to an underscore when it actually compiles the crate.

We should do the same when generating rul.rs so the reference resolves properly. This patch does it at generation time. It may make more sense to do it at read time in frontend/emitter.py, but then we have to track the path separately.

Comment 33

[Nathan Froyd [:froydnj]]

(In reply to Ralph Giles (:rillian) needinfo me from comment #32)
> Fixup patch to make `./mach gtest rust.*` work.

Thanks for this patch; I'll look at this tomorrow.

I also realized that moz.build can enforce the requirement that crates point to in-tree dependencies; I'll try to add that and some corresponding tests tomorrow.  cargo can obviously enforce that too, and we can/should use that option, but getting the error at configure time rather than build time is more developer-friendly.

Comment 34

[(not currently active) Ted Mielczarek]

I had vague ideas around that. I was thinking it'd be nice to have a developer mode where you could put arbitrary crate references in and not worry about vendoring. When I'm doing Rust development I often wind up pulling in external crates for things, and it'd be nice to support that workflow without having an extra step in the middle. We'd obviously require a vendoring step for pushing anywhere.

Comment 35

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8768553 [details] [diff] [review]
part 1 - search for cargo when --enable-rust

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

::: build/moz.configure/rust.configure
@@ +19,2 @@
>  rustc = check_prog('RUSTC', rust_compiler_names, allow_missing=True)
> +cargo = check_prog('CARGO', cargo_binary_names, allow_missing=True)

I don't think assigning to a variable is necessary here, unless you're using it in a later patch.

Comment 36

[(not currently active) Ted Mielczarek]

(In reply to Nathan Froyd [:froydnj] from comment #31)
> Oh, we're also now batching up compilation of all the Rust code when we get
> ready to compile libxul.  I don't know how much of a problem this is in
> practice, because we need to run LTO anyway to get rid of unused Rust code. 
> But this part of the build is now a bit more serial than it would be
> otherwise, even though we only have a small amount of code, so it doesn't
> matter much at the moment.

Presumably we could build the rust code at any time during the compile tier, right? It would not be great because cargo doesn't yet support make jobservers, but that's something Alex said they were interested in doing:
https://github.com/rust-lang/cargo/issues/1744

Comment 37

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8768555 [details] [diff] [review]
part 3 - rename RustRlibLibrary to RustCrate

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

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +728,2 @@
>                                            mozpath.join(context.srcdir, mozpath.dirname(f)),
>                                            rlib_filename, final_lib)

Consider updating the indentation of this argument list with this change.

Comment 38

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8768559 [details] [diff] [review]
part 7 - build rust code via cargo

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1209,5 @@
> +debug = true
> +rpath = false
> +lto = true
> +debug-assertions = false
> +code-gen-units = 1

It's code-gen-units here, and codegen-units above. One of these is probably wrong.

Comment 39

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8768556 [details] [diff] [review]
part 4 - remove rlib_filename from RustCrate

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

One question, but nothing I think we should block on.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1219,5 @@
>                                          % (relpath, lib.import_name))
>                      if isinstance(obj, SharedLibrary):
>                          write_shared_and_system_libs(lib)
>                  elif isinstance(lib, RustCrate):
> +                    continue

We could just remove this branch entirely.

@@ -1220,5 @@
>                      if isinstance(obj, SharedLibrary):
>                          write_shared_and_system_libs(lib)
>                  elif isinstance(lib, RustCrate):
> -                    backend_file.write_once('RLIB_EXTERN_CRATE_OPTIONS += --extern %s=%s/%s\n'
> -                                            % (lib.crate_name, relpath, lib.rlib_filename))

This looks like a change that will prevent this series from being commit-level bisect-able... is there a particular reason to structure the patch this way?

Comment 40

[Nathan Froyd [:froydnj]]

(In reply to Chris Manchester (:chmanchester) from comment #39)
> Comment on attachment 8768556 [details] [diff] [review]
> part 4 - remove rlib_filename from RustCrate
> 
> Review of attachment 8768556 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One question, but nothing I think we should block on.
> 
> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +1219,5 @@
> >                                          % (relpath, lib.import_name))
> >                      if isinstance(obj, SharedLibrary):
> >                          write_shared_and_system_libs(lib)
> >                  elif isinstance(lib, RustCrate):
> > +                    continue
> 
> We could just remove this branch entirely.

We actually need to keep it here; otherwise we fall into the condition just after this and things go haywire.

> @@ -1220,5 @@
> >                      if isinstance(obj, SharedLibrary):
> >                          write_shared_and_system_libs(lib)
> >                  elif isinstance(lib, RustCrate):
> > -                    backend_file.write_once('RLIB_EXTERN_CRATE_OPTIONS += --extern %s=%s/%s\n'
> > -                                            % (lib.crate_name, relpath, lib.rlib_filename))
> 
> This looks like a change that will prevent this series from being
> commit-level bisect-able... is there a particular reason to structure the
> patch this way?

Hm, that's a good point.  I guess we should just fold this into part 7 instead.

Comment 41

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8768559 [details] [diff] [review]
part 7 - build rust code via cargo

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

::: config/rules.mk
@@ +925,5 @@
>  # in the target's LIBS.
>  $(RSOBJS):
>  	$(REPORT_BUILD)
> +	#$(RUSTC) $(RUST_TARGET) $(RUSTFLAGS) --crate-type rlib --emit dep-info=$(MDDEPDIR)/$(call mk_libname,$<).pp,link=$(call mk_libname,$<) $(_VPATH_SRCS)
> +	@echo NOT BUILDING BECAUSE CARGO

Is there any meaning to the RSOBJS variable after this change? I don't see any. If not, let's get rid of this.

@@ +930,4 @@
>  
>  $(RS_STATICLIB_CRATE_OBJ):
>  	$(REPORT_BUILD)
> +	#$(RUSTC) $(RUST_TARGET) $(RUSTFLAGS) --crate-type staticlib $(RLIB_EXTERN_CRATE_OPTIONS) --emit dep-info=$(MDDEPDIR)/$(call mk_global_crate_libname,$(RS_STATICLIB_CRATE_SRC)).pp,link=$@ $(RS_STATICLIB_CRATE_SRC)

Let's delete this line.

Comment 42

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8768598 [details] [diff] [review]
Part 7a - Caseshift '-' to '_' in crate names

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1169,4 @@
>              with self._write_file(extern_crate_file) as f:
>                  f.write('// AUTOMATICALLY GENERATED.  DO NOT EDIT.\n\n')
>                  for rlib in rust_rlibs:
> +                    f.write('extern crate %s;\n' % rlib.crate_name.replace('-', '_')

This is missing a final closing paren.

Comment 43

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8768559 [details] [diff] [review]
part 7 - build rust code via cargo

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

What's the plan for dependencies with this patch? When I build locally, I can edit .rs files, run |./mach build|, and cargo isn't invoked.

I think you mentioned this, but this can't land as long as it hits the network, so clearing review for now.

::: config/rules.mk
@@ +930,4 @@
>  
>  $(RS_STATICLIB_CRATE_OBJ):
>  	$(REPORT_BUILD)
> +	#$(RUSTC) $(RUST_TARGET) $(RUSTFLAGS) --crate-type staticlib $(RLIB_EXTERN_CRATE_OPTIONS) --emit dep-info=$(MDDEPDIR)/$(call mk_global_crate_libname,$(RS_STATICLIB_CRATE_SRC)).pp,link=$@ $(RS_STATICLIB_CRATE_SRC)

It appears our build system no longer has much use for RUSTFLAGS after this patch, let's get rid of extraneous references to them.

::: dom/media/gtest/moz.build
@@ -36,5 @@
>          'TestWebMWriter.cpp',
>      ]
>  
>  if CONFIG['MOZ_RUST']:
> -    SOURCES += ['hello.rs',]

With .rs sources in SOURCES gone, don't we want to get rid of support for them, and remove references to RSSRCS in the backend?

::: media/libstagefright/moz.build
@@ +80,5 @@
>  ]
>  
>  if CONFIG['MOZ_RUST']:
> +    CRATES += [
> +        'binding/mp4parse/',

I might leave off the trailing '/' here.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1209,5 @@
> +debug = true
> +rpath = false
> +lto = true
> +debug-assertions = false
> +code-gen-units = 1

According to the cargo docs, "codegen-units" is ignored when lto = true.

@@ +1210,5 @@
> +rpath = false
> +lto = true
> +debug-assertions = false
> +code-gen-units = 1
> +panic = "unwind"

I recall a long discussion on dev-platform about this, I'm sure I'm missing some nuance here, but my impression from that was we should be using panic = "abort". I'm mostly curious if someone can comment on this.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +704,5 @@
> +                raise SandboxValidationError(
> +                    'Rust crates must be destined for a FINAL_LIBRARY')
> +            if shared_lib:
> +                raise SandboxValidationError(
> +                    'No Rust crates permitted as an immediate input of %s: %s' % (shlib, crates))

This seems to repeat the typo `shlib` from this line's old location. We have some decent unit tests for the some of the code in this file, for instance in python/mozbuild/mozbuild/test/frontend/test_emitter.py.

Comment 44

[Nathan Froyd [:froydnj]]

ni? to Chris for clarification on the network dependency part.

(In reply to Chris Manchester (:chmanchester) from comment #43)
> What's the plan for dependencies with this patch? When I build locally, I
> can edit .rs files, run |./mach build|, and cargo isn't invoked.

Ah, good point.  |cargo rustc| doesn't pass through output options to rustc properly; I think this is https://github.com/rust-lang/cargo/issues/2559 .  So emitting dependency information winds up in the wrong place--assuming you pass the appropriate options, which this patch (wrongly) doesn't do.  I guess we could hack around this by importing the cargo-generated dependency information from the "wrong" place just for directories that build the librul crate.

> I think you mentioned this, but this can't land as long as it hits the
> network, so clearing review for now.

For avoidance of doubt, do you mean that mean that landing requires there to be no-network checks somewhere, whether that's in moz.build parsing the Cargo.toml files to verify they all have path= specifications or passing the appropriate flags to cargo (or both!)...or do you mean something else?

> It appears our build system no longer has much use for RUSTFLAGS after this
> patch, let's get rid of extraneous references to them.
> 
> With .rs sources in SOURCES gone, don't we want to get rid of support for
> them, and remove references to RSSRCS in the backend?

I should write a separate patch that goes through and deletes detritus that we no longer need after this series.

> @@ +1210,5 @@
> > +rpath = false
> > +lto = true
> > +debug-assertions = false
> > +code-gen-units = 1
> > +panic = "unwind"
> 
> I recall a long discussion on dev-platform about this, I'm sure I'm missing
> some nuance here, but my impression from that was we should be using panic =
> "abort". I'm mostly curious if someone can comment on this.

We should be, and I see that now Rust 1.10 is released, we could...except that AIUI, we are still using an old version of Rust on Mac, which means that we have to stick with panic="unwind".  In any event, I vote that switching from unwind to abort be a separate bug so we can evaluate those changes on their own.

Comment 45

[Simon Sapin (:SimonSapin)]

Cargo just landed this pull request https://github.com/rust-lang/cargo/pull/2811 : "Add a flag to force network access to be an error"

Comment 46

[Chris Manchester (limited bugmail, email directly)]

(In reply to Nathan Froyd [:froydnj] from comment #44)
> ni? to Chris for clarification on the network dependency part.
> 
> (In reply to Chris Manchester (:chmanchester) from comment #43)
> > What's the plan for dependencies with this patch? When I build locally, I
> > can edit .rs files, run |./mach build|, and cargo isn't invoked.
> 
> Ah, good point.  |cargo rustc| doesn't pass through output options to rustc
> properly; I think this is https://github.com/rust-lang/cargo/issues/2559 . 
> So emitting dependency information winds up in the wrong place--assuming you
> pass the appropriate options, which this patch (wrongly) doesn't do.  I
> guess we could hack around this by importing the cargo-generated dependency
> information from the "wrong" place just for directories that build the
> librul crate.
> 
> > I think you mentioned this, but this can't land as long as it hits the
> > network, so clearing review for now.
> 
> For avoidance of doubt, do you mean that mean that landing requires there to
> be no-network checks somewhere, whether that's in moz.build parsing the
> Cargo.toml files to verify they all have path= specifications or passing the
> appropriate flags to cargo (or both!)...or do you mean something else?

This is what I mean. Both sounds reasonable if it ends up saving people time by moving any error earlier, but it looks like we need to at least pass --frozen to cargo and have a mp4parse that builds with --frozen (comment 29 mentions a build-time dependency that's out of tree, I don't know the status of that).

> 
> > It appears our build system no longer has much use for RUSTFLAGS after this
> > patch, let's get rid of extraneous references to them.
> > 
> > With .rs sources in SOURCES gone, don't we want to get rid of support for
> > them, and remove references to RSSRCS in the backend?
> 
> I should write a separate patch that goes through and deletes detritus that
> we no longer need after this series.
> 
> > @@ +1210,5 @@
> > > +rpath = false
> > > +lto = true
> > > +debug-assertions = false
> > > +code-gen-units = 1
> > > +panic = "unwind"
> > 
> > I recall a long discussion on dev-platform about this, I'm sure I'm missing
> > some nuance here, but my impression from that was we should be using panic =
> > "abort". I'm mostly curious if someone can comment on this.
> 
> We should be, and I see that now Rust 1.10 is released, we could...except
> that AIUI, we are still using an old version of Rust on Mac, which means
> that we have to stick with panic="unwind".  In any event, I vote that
> switching from unwind to abort be a separate bug so we can evaluate those
> changes on their own.

Of course. Thanks for the explanation.

Comment 47

[(not currently active) Ted Mielczarek]

I believe that Alex has said that if we commit a Cargo.lock file to the tree that contains only path references to crates in the tree that cargo should not hit the network. The argument mentioned in comment 45 is a backstop for that, to ensure that it errors instead of hitting the network if our configuration is wrong.

Comment 48

[Nathan Froyd [:froydnj]]

Attachment: part 4 - add pytoml to the virtualenv

Renumbering patches.

Comment 49

[Nathan Froyd [:froydnj]]

Attachment: part 5 - add a Cargo.toml for the dom/media/gtest/ code

Renumbering patches.

Comment 50

[Nathan Froyd [:froydnj]]

Attachment: part 6 - build rust code via cargo

This is a second attempt at building everything via cargo.  Notable changes
from the first version:

* We squashed part 4 into this so as to try and not break commit-level
  bisection.

* Tests.

* cargo is invoked on every build for completeness purposes.

* The caseshifting patch Ralph posted has been folded into the patch series.

* Various detritus from the Old Way of doing things has been cleaned up.

I think this ought to build with multiple crates in the tree, but I haven't
really tried it.

One thing worth noting: I had to clean out the mp4parse Cargo.toml file of
dependencies and whatnot, as we check the Cargo.toml files to ensure that every
dependency is specified with a local path, including build dependencies.  This
is good, but also slightly non-optimal in that we have to verify optional
dependencies too.  We could have ignored them, but then we might start fetching
things from the network if appropriate flags somehow get flipped.  I was not
brave enough to determine whether we had flags turned on that would require
optional crates (and I think we can turn those on at cargo invocation time
anyway?).  Ideas on this welcome.

Asking Ralph for review on the mp4parse Cargo.toml changes, which will have to
be put in a .patch file somewhere too; I just realized that while writing this,
but I also wanted to get the patch up for review.

Comment 51

[Ralph Giles (:rillian)]

Comment on attachment 8772922 [details] [diff] [review]
part 6 - build rust code via cargo

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

r=me for the mp4parse changes. Please do add a patch file and a line applying it in media/libstagefright/binding/update-rust.sh. Otherwise it will get clobbered the next time we update the source.

::: media/libstagefright/moz.build
@@ +80,5 @@
>  ]
>  
>  if CONFIG['MOZ_RUST']:
> +    CRATES += [
> +        'binding/mp4parse/',

You still have a trailing directory separator here.

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1207,5 @@
> +[profile.release]
> +opt-level = 2
> +debug = true
> +rpath = false
> +lto = true

I assume this works on try? In bug 1268547 I had trouble with -C lto and `./mach gtest rust.*` because of the two staticlibs that get linked into libxul-gtest. I haven't tried recently though.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +19,5 @@
>  )
>  
>  import mozpack.path as mozpath
>  import mozinfo
> +import pytoml

For the sake of my curiosity, why is pytoml better than toml?

@@ +750,5 @@
> +                            if not os.path.exists(mozpath.join(context.srcdir, values['path'])):
> +                                raise SandboxValidationError(
> +                                    '%s %s of crate %s refers to a non-existent path' % (description, dep_crate_name, crate_name),
> +                                    context)
> +                        

trailing whitespace

Comment 52

[Ralph Giles (:rillian)]

Nathan, I hear you're planning to land this this week, which is great! Do you want me to add cargo to the mac tooltool rust? If so please file a bug.

Comment 53

[Ralph Giles (:rillian)]

Ah, I see you already answered in bug 1268982.

Comment 54

[Ralph Giles (:rillian)]

Answered the `-C lto` question, I mean.

Comment 55

[Nathan Froyd [:froydnj]]

(In reply to Ralph Giles (:rillian) needinfo me from comment #51)
> ::: media/libstagefright/moz.build
> @@ +80,5 @@
> >  ]
> >  
> >  if CONFIG['MOZ_RUST']:
> > +    CRATES += [
> > +        'binding/mp4parse/',
> 
> You still have a trailing directory separator here.

I prefer trailing directory separators! =D

> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +1207,5 @@
> > +[profile.release]
> > +opt-level = 2
> > +debug = true
> > +rpath = false
> > +lto = true
> 
> I assume this works on try? In bug 1268547 I had trouble with -C lto and
> `./mach gtest rust.*` because of the two staticlibs that get linked into
> libxul-gtest. I haven't tried recently though.

It works locally.  I do need to run things through try.

> ::: python/mozbuild/mozbuild/frontend/emitter.py
> @@ +19,5 @@
> >  )
> >  
> >  import mozpack.path as mozpath
> >  import mozinfo
> > +import pytoml
> 
> For the sake of my curiosity, why is pytoml better than toml?

Because pytoml can actually parse things like:

[dependencies]
byteorder = { version = "0.5.0", path = ... }

and toml cannot (at least, released versions of toml cannot; some of the github issues suggest that it can?).

(In reply to Ralph Giles (:rillian) needinfo me from comment #52)
> Nathan, I hear you're planning to land this this week, which is great! Do
> you want me to add cargo to the mac tooltool rust? If so please file a bug.

I would like to land this week!...though I cannot recall sharing that information.  Is that a nudge to get this landed? :p

It seems like it would be easier to package cargo separately in this instance, so we can ensure that we're using the same cargo version across all platforms?  I am ambivalent on this issue.

Comment 56

[Ralph Giles (:rillian)]

(In reply to Nathan Froyd [:froydnj] from comment #55)

> > For the sake of my curiosity, why is pytoml better than toml?
> 
> Because pytoml can actually parse things like:
> 
> [dependencies]
> byteorder = { version = "0.5.0", path = ... }

Ah, ok. I'd tried something simpler and seen the toml4 references on github and assumed it would. Thanks!

> I would like to land this week!...though I cannot recall sharing that
> information.  Is that a nudge to get this landed? :p

It was in some meeting notes. I _am_ very excited about it landing though!

> It seems like it would be easier to package cargo separately in this
> instance, so we can ensure that we're using the same cargo version across
> all platforms?  I am ambivalent on this issue.

I'm ambivalent too. It might be sometimes nice to update them separately, like now. OTOH, upstream releases them together, in the sense that a rust release build points to a cargo release build, so we'll want to bump cargo when we bump the toolchain. The rustup tool maintains them in the same directory, and one tooltool download is easier than two.

Based on that I've already included cargo for the linux and windows builders in the same package as rustc. I'll patch the mac build tomorrow.

Comment 57

[Chris Manchester (limited bugmail, email directly)]

(In reply to Ralph Giles (:rillian) needinfo me from comment #56)
> (In reply to Nathan Froyd [:froydnj] from comment #55)
> 
> > > For the sake of my curiosity, why is pytoml better than toml?
> > 
> > Because pytoml can actually parse things like:
> > 
> > [dependencies]
> > byteorder = { version = "0.5.0", path = ... }
> 
> Ah, ok. I'd tried something simpler and seen the toml4 references on github
> and assumed it would. Thanks!
> 
> > I would like to land this week!...though I cannot recall sharing that
> > information.  Is that a nudge to get this landed? :p
> 
> It was in some meeting notes. I _am_ very excited about it landing though!
> 
> > It seems like it would be easier to package cargo separately in this
> > instance, so we can ensure that we're using the same cargo version across
> > all platforms?  I am ambivalent on this issue.
> 
> I'm ambivalent too. It might be sometimes nice to update them separately,
> like now. OTOH, upstream releases them together, in the sense that a rust
> release build points to a cargo release build, so we'll want to bump cargo
> when we bump the toolchain. The rustup tool maintains them in the same
> directory, and one tooltool download is easier than two.
> 
> Based on that I've already included cargo for the linux and windows builders
> in the same package as rustc. I'll patch the mac build tomorrow.

Ideally we'd start with a cargo that includes the pull request mentioned in comment 45. This seems like a compelling reason to update them separately (at least temporarily).

Comment 58

[Ralph Giles (:rillian)]

It's fine to require a particular cargo version for our integration builds, but I don't want to ask developers to build and install a tool from source for their local builds to work. We've been working with the stable rust toolchain for a year now. We can't pass --frozen unconditionally because cargo errors on unrecognized options. Therefore if we want to require this on integration builds we need to detect whether it is supported by a particular cargo and enable it only then, coupled with some way to force this on integration builds.

Further, this patch is using `cargo rustc` which doesn't currently support the option. Nathan, is this just so force the final library name rather than relying on cargo matching our libname generation? We'd either need to switch to `cargo build` or wait for another patch to go into cargo.

I think this should be handled in a follow-up bug. What we have here isn't any worse that the current code. There is no risk of subsequent changes causing cargo to pull dependencies from the network because the buildbot workers are firewalled; any such patch will fail integration builds.

Comment 59

[Nathan Froyd [:froydnj]]

(In reply to Ralph Giles (:rillian) needinfo me from comment #58)
> Further, this patch is using `cargo rustc` which doesn't currently support
> the option. Nathan, is this just so force the final library name rather than
> relying on cargo matching our libname generation?

The major reason I did it was so I didn't have to fish the static library out of cargo's maze of directories that it generates, taking care to copy the binary into the place we expect it only if it changed, etc. etc.

I suppose we could instead inform the build system that the canonical location is wherever cargo generates it, but that still requires a bunch of directory fishing.  If we could rely on RUST_TARGET:

http://dxr.mozilla.org/mozilla-central/source/build/moz.configure/rust.configure#147

always being present (it may not be present for some native compilations), that would simplify matters somewhat.  Double-checking with |cargo build| shows that it creates librul.a, so naming is not a problem.

I didn't realize that |cargo rustc| wasn't available everywhere; I can use |cargo build| if that would make life simpler.

Comment 60

[(not currently active) Ted Mielczarek]

I don't think rillian was talking about `cargo rustc`, but rather that the new `--frozen` option is only available in the `cargo build` command.

I don't think the buildbot builders are actually firewalled that tightly, I think that's a myth. AIUI, if we have a committed Cargo.lock file in-tree (I haven't read this patch series yet, sorry) then `cargo build` should not hit the network if it's all path dependencies, but if we don't, it might still hit the network anyway. `--frozen` is just to assert that condition.

Comment 61

[Nathan Froyd [:froydnj]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #60)
> I don't think the buildbot builders are actually firewalled that tightly, I
> think that's a myth. AIUI, if we have a committed Cargo.lock file in-tree (I
> haven't read this patch series yet, sorry)

For avoidance of doubt: we do not have a Cargo.lock checked into the tree after this patch series.  I'm unsure of how to set this up, since the Cargo.toml file used to direct cargo is generated in the objdir, so that's where Cargo.lock winds up.  I don't see in the cargo docs how to generate/use a Cargo.lock in a different directory from where the Cargo.toml file is.

I guess we could just not generate the libxul/libxul-gtest Cargo.toml files at build time, but have |mach regenerate-cargo-toml| do that work, place the appropriate files in the srcdir, and then we could check in the Cargo.{toml,lock} files?  I know we don't like having generated files in the tree like that, but there is precedent for it...

Comment 62

[Ralph Giles (:rillian)]

(In reply to Nathan Froyd [:froydnj] from comment #59)

> The major reason I did it was so I didn't have to fish the static library
> out of cargo's maze of directories that it generates, taking care to copy
> the binary into the place we expect it only if it changed, etc. etc.

Ah, that makes sense. I guess it's in target/[$RUSTC_TARGET/]$profile/ which is a bit tricky.

In my testing, if I `cargo build --target x86_64-apple-darwin` it puts the output in target/x86_64-apple-darwin/debug/libname.a even though x86_64-apple-darwin is the native host triple. If I don't pass `--target` the output is target/debug/libname.a. That's deterministic if a little complicated. Maybe this bit can be in python?

I suppose it doesn't matter much, but I think `cargo build` is better if we can make it work. It's the command developers are most familiar with.

> I didn't realize that |cargo rustc| wasn't available everywhere; I can use
> |cargo build| if that would make life simpler.

I believe `cargo rustc` is available everywhere. The command landed in May 2015. It's that `cargo rustc --frozen` isn't implemented, only `cargo build --frozen`.

Comment 63

[Ralph Giles (:rillian)]

(In reply to Nathan Froyd [:froydnj] from comment #61)

> For avoidance of doubt: we do not have a Cargo.lock checked into the tree
> after this patch series.  I'm unsure of how to set this up, since the
> Cargo.toml file used to direct cargo is generated in the objdir, so that's
> where Cargo.lock winds up.

This is just the rul crate's Cargo.toml and Cargo.lock though. I'd rather hoped that since the rul crate only references in-tree crates, that if all the in-tree crates reference in-tree dependencies, either directly as paths or through an in-tree registry, that it would just work, and we'd only need something like Alex's `cargo vendor` tool to maintain all those links, and 'cargo build --frozen' to check that it worked. 

That said, there is precedent for checking in a Cargo.lock on the rust side as well: applications wanting deterministic builds are encouraged to keep their lock file in-tree so everyone gets the same versions of dependencies and updates are explicitly tracked in history.

Comment 64

[Ralph Giles (:rillian)]

I filed https://github.com/rust-lang/cargo/issues/2902 about `cargo rustc --frozen` not working.

Comment 65

[Ralph Giles (:rillian)]

Looks like I was confused about `cargo rustc --frozen` not working. It does work in a fresh build of cargo master. Sorry for the confusion!

Comment 66

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #60)
> I don't think the buildbot builders are actually firewalled that tightly, I
> think that's a myth.

They are, but they also have a big hole: they can access github.

Comment 67

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8772922 [details] [diff] [review]
part 6 - build rust code via cargo

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

Based on recent discussion, we still need to figure out how to pass --frozen to cargo in automation. The docs for --frozen indicate it asserts Cargo.lock doesn't change, so that means figuring out how to check in a Cargo.lock.

::: python/mozbuild/mozbuild/test/frontend/test_emitter.py
@@ +1062,5 @@
> +        reader = self.reader('crate-final-library')
> +        with self.assertRaisesRegexp(SandboxValidationError,
> +             'Rust crates must be destined for a FINAL_LIBRARY'):
> +            objs = self.read_topsrcdir(reader)
> +

These `objs` are mostly dead.

Comment 68

[Ralph Giles (:rillian)]

Attachment: part 6a - Pass RUSTC to CARGO

I did a try push on top of the cargo nightlies added by bug 1249511. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad6e37fabd81&selectedJob=24332954

First issue is that we need to define RUSTC in the environment when we invoke cargo. Otherwise it tries to use the rustc in path, which could be wrong, and isn't actually available in integration builds. This patch adds that.

That gets linux debug builds working.

Linux opt builds seem to still have `-C lto` problems with multiple symbol errors linking gtestxul. Probably we can avoid that by leaving turning on rust lto to bug 1268547.

Mac builds fail because of panic=abort in Cargo.toml. Turning that on should be deferred to bug 1268727. The mac rust toolchain is too old to support this option, but we expect that to be able to update them to rust 1.12.

I'm less clear what's going on with the windows build. From the win64 debug build log:

16:30:09     INFO -     Compiling rul v0.1.0 (file:///C:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/obj-firefox/toolkit/library)
16:30:09     INFO -       Running `c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/rustc/bin/rustc.EXE rul.rs --crate-name rul --crate-type staticlib -C panic=unwind -C codegen-units=1 -g -o librul.lib --out-dir c:\\builds\\moz2_slave\\try-w64-d-00000000000000000000\\build\\src\\obj-firefox\\toolkit\\library\\.\\x86_64-pc-windows-msvc\\debug --emit=dep-info,link --target x86_64-pc-windows-msvc -L dependency=c:\\builds\\moz2_slave\\try-w64-d-00000000000000000000\\build\\src\\obj-firefox\\toolkit\\library\\.\\x86_64-pc-windows-msvc\\debug -L dependency=c:\\builds\\moz2_slave\\try-w64-d-00000000000000000000\\build\\src\\obj-firefox\\toolkit\\library\\.\\x86_64-pc-windows-msvc\\debug\\deps --extern mp4parse=c:\\builds\\moz2_slave\\try-w64-d-00000000000000000000\\build\\src\\obj-firefox\\toolkit\\library\\.\\x86_64-pc-windows-msvc\\debug\\deps\\libmp4parse-50e3ca22d6ce156d.rlib`
16:30:09     INFO -  warning: ignoring specified output filename because multiple outputs were requested
16:30:09     INFO -  warning: ignoring --out-dir flag due to -o flag.
[...]
16:30:11     INFO -  Exception: File not found: librul.lib
16:30:11     INFO -  c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/config/rules.mk:784: recipe for target 'xul_s.lib.desc' failed

Comment 69

[Nathan Froyd [:froydnj]]

(In reply to Ralph Giles (:rillian) needinfo me from comment #68)
> First issue is that we need to define RUSTC in the environment when we
> invoke cargo. Otherwise it tries to use the rustc in path, which could be
> wrong, and isn't actually available in integration builds. This patch adds
> that.

Nice catch, thanks.

> Linux opt builds seem to still have `-C lto` problems with multiple symbol
> errors linking gtestxul. Probably we can avoid that by leaving turning on
> rust lto to bug 1268547.

That's odd; Linux opt builds work for me locally.  Oh, I didn't try gtestxul, I should investigate that.

> Mac builds fail because of panic=abort in Cargo.toml.

Who is turning on panic=abort?  These patches specify panic=unwind.

> I'm less clear what's going on with the windows build. From the win64 debug
> build log:

I wonder if we're generating the wrong file, like rul.lib or something like that; we'd have to turn on --verbose from cargo to see what it's doing.

Comment 70

[Ralph Giles (:rillian)]

Attachment: part 6b - Defer rust optimzations with blocking bugs

Remove optimizations with blocking bugs. This fixes the linux and mac builds. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f80523a252ac

Sorry, I think the issue is specifying a panic strategy at all. The 1.6 compiler rejects `-C panic=unwind` as well.

Wrong file is certainly possible, or some interaction with -o and windows paths end up with it in the wrong directory. We already pass --verbose to cargo, so you can see the rustc command lines in the log, but it doesn't pass it on to rustc, so maybe `-- --verbose` would help.

Comment 71

[Nathan Froyd [:froydnj]]

Comment on attachment 8773565 [details] [diff] [review]
part 6a - Pass RUSTC to CARGO

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

Are you posting these with the intent that they be rolled in, or committing as separate patches?  Giving you credit by checking them in as separate patches seems appropriate, but rolling them in works for me too...

Comment 72

[Nathan Froyd [:froydnj]]

Comment on attachment 8773604 [details] [diff] [review]
part 6b - Defer rust optimzations with blocking bugs

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

Does rustc new enough to support -C abort=panic also always default to abort=unwind, or is it possible to change the default when you compile rustc?

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1213,3 @@
>  debug-assertions = false
>  # code-gen-units doesn't work with lto=true.
> +code-gen-units = 1

This reminds me that I need to address comment 38.

Comment 73

[Ralph Giles (:rillian)]

(In reply to Nathan Froyd [:froydnj] from comment #71)

> Are you posting these with the intent that they be rolled in, or committing
> as separate patches?  Giving you credit by checking them in as separate
> patches seems appropriate, but rolling them in works for me too...

I assumed you'd roll them up, but I don't mind either way.

Comment 74

[Nathan Froyd [:froydnj]]

(In reply to Chris Manchester (:chmanchester) from comment #67)
> Based on recent discussion, we still need to figure out how to pass --frozen
> to cargo in automation. The docs for --frozen indicate it asserts Cargo.lock
> doesn't change, so that means figuring out how to check in a Cargo.lock.

Do you have any thoughts on how you would like to see comment 61 addressed, then?  There's a tension between trying to specify all the crates in moz.build and having that generating the appropriate Cargo.toml vs. checking in Cargo files to make things like --frozen work.

We can make moz.build check that all crates specified have local paths provided (this functionality is in the patch) and extending it to check dependencies recursively would be straightforward.  I agree that it would be nice to utilize Cargo features for this, but what do we gain trying to reconfigure things to use --frozen vs. the checking that we have now?

Comment 75

[(not currently active) Ted Mielczarek]

The motivation for `--frozen` is to ensure that cargo won't hit the network for any reason, and will fail with a sensible error if we provide a bad configuration that would have caused it to. The risk we take with a standalone check is that something in cargo changes and we are no longer validating things the same way and we miss something that causes us to wind up hitting the network during the build.

Comment 76

[Ralph Giles (:rillian)]

(In reply to Nathan Froyd [:froydnj] from comment #72)

> Does rustc new enough to support -C abort=panic also always default to
> abort=unwind, or is it possible to change the default when you compile rustc?

The default seems to be hard-coded to abort=unwind, so one would have to patch the compiler source to get different behaviour without passing a codegen flag. https://github.com/rust-lang/rust/blob/d46ed83/src/librustc/session/config.rs#L651

Comment 77

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #61)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #60)
> > I don't think the buildbot builders are actually firewalled that tightly, I
> > think that's a myth. AIUI, if we have a committed Cargo.lock file in-tree (I
> > haven't read this patch series yet, sorry)
> 
> For avoidance of doubt: we do not have a Cargo.lock checked into the tree
> after this patch series.  I'm unsure of how to set this up, since the
> Cargo.toml file used to direct cargo is generated in the objdir, so that's
> where Cargo.lock winds up.  I don't see in the cargo docs how to
> generate/use a Cargo.lock in a different directory from where the Cargo.toml
> file is.

For edification, since I just figured this out: We can check in a Cargo.toml file and use --manifest-path to point cargo at it at the appropriate point in the build.

Comment 78

[Chris Manchester (limited bugmail, email directly)]

(In reply to Nathan Froyd [:froydnj] from comment #74)
> (In reply to Chris Manchester (:chmanchester) from comment #67)
> > Based on recent discussion, we still need to figure out how to pass --frozen
> > to cargo in automation. The docs for --frozen indicate it asserts Cargo.lock
> > doesn't change, so that means figuring out how to check in a Cargo.lock.
> 
> Do you have any thoughts on how you would like to see comment 61 addressed,
> then?  There's a tension between trying to specify all the crates in
> moz.build and having that generating the appropriate Cargo.toml vs. checking
> in Cargo files to make things like --frozen work.
> 
> We can make moz.build check that all crates specified have local paths
> provided (this functionality is in the patch) and extending it to check
> dependencies recursively would be straightforward.  I agree that it would be
> nice to utilize Cargo features for this, but what do we gain trying to
> reconfigure things to use --frozen vs. the checking that we have now?

The two options that come to mind would be: a developer mode where the build system generates a top-level Cargo.toml and lets you run cargo without `--frozen`, or a separate mach command to generate the appropriate Cargo files locally. In either case, it might be nice for an automation build to use CRATES from moz.build to verify they're all present in the master Cargo.toml (and provide a helpful message if they're not).

Ted pretty much said this, but even if our check for path-only dependencies means cargo doesn't hit the network during a build, `--frozen` is what we want verify this is doing what we think it is and will continue to do so in the future.

Comment 79

[Chris Manchester (limited bugmail, email directly)]

(In reply to Nathan Froyd [:froydnj] from comment #77)
> (In reply to Nathan Froyd [:froydnj] from comment #61)
> > (In reply to Ted Mielczarek [:ted.mielczarek] from comment #60)
> > > I don't think the buildbot builders are actually firewalled that tightly, I
> > > think that's a myth. AIUI, if we have a committed Cargo.lock file in-tree (I
> > > haven't read this patch series yet, sorry)
> > 
> > For avoidance of doubt: we do not have a Cargo.lock checked into the tree
> > after this patch series.  I'm unsure of how to set this up, since the
> > Cargo.toml file used to direct cargo is generated in the objdir, so that's
> > where Cargo.lock winds up.  I don't see in the cargo docs how to
> > generate/use a Cargo.lock in a different directory from where the Cargo.toml
> > file is.
> 
> For edification, since I just figured this out: We can check in a Cargo.toml
> file and use --manifest-path to point cargo at it at the appropriate point
> in the build.

Great, I was going to suggest short of that we use something like FINAL_TARGET_FILES to put them in the right spot as part of the build.

Comment 80

[Nathan Froyd [:froydnj]]

(In reply to Chris Manchester (:chmanchester) from comment #78)
> (In reply to Nathan Froyd [:froydnj] from comment #74)
> > Do you have any thoughts on how you would like to see comment 61 addressed,
> > then?  There's a tension between trying to specify all the crates in
> > moz.build and having that generating the appropriate Cargo.toml vs. checking
> > in Cargo files to make things like --frozen work.
>
> The two options that come to mind would be: a developer mode where the build
> system generates a top-level Cargo.toml and lets you run cargo without
> `--frozen`, or a separate mach command to generate the appropriate Cargo
> files locally. In either case, it might be nice for an automation build to
> use CRATES from moz.build to verify they're all present in the master
> Cargo.toml (and provide a helpful message if they're not).

"top-level Cargo.toml" sounds like a misnomer as we currently have two different Cargo.toml files being generated with the current patch.  And surely people are going to want to build host binaries (or even target ones) at some point...and presumably Rust usage in non-xul libraries...

Putting everything in a single Cargo.toml file at the toplevel feels unwieldy to me, but maybe that's our best option?  We're trying to specify things in the build system and then have those translated into cargo-land; it feels like that's running into complexity as we try to have Cargo.lock work appropriately and future extensions to our minimal Rust usage here.  (Or I might just be grumpy on a Friday.)  Perhaps we should specify things entirely in cargo-land and then pull those into moz.build as appropriate:

RUST_LIBRARY = ["rul"]

RUST_BINARIES = ["fancy", "killer-feature"]

I think cargo has support for building specific libraries or binaries, so there's less magic on the actual build system side.  Doing things this way would also avoid squirreling settings (cargo profiles, etc.) away in recursivemake.py.  I guess when people want to use a new crate in libxul, they'd have to add it manually, but maybe that's not such a bad thing?

Comment 81

[Chris Manchester (limited bugmail, email directly)]

(In reply to Nathan Froyd [:froydnj] from comment #80)
> (In reply to Chris Manchester (:chmanchester) from comment #78)
> > (In reply to Nathan Froyd [:froydnj] from comment #74)
> > > Do you have any thoughts on how you would like to see comment 61 addressed,
> > > then?  There's a tension between trying to specify all the crates in
> > > moz.build and having that generating the appropriate Cargo.toml vs. checking
> > > in Cargo files to make things like --frozen work.
> >
> > The two options that come to mind would be: a developer mode where the build
> > system generates a top-level Cargo.toml and lets you run cargo without
> > `--frozen`, or a separate mach command to generate the appropriate Cargo
> > files locally. In either case, it might be nice for an automation build to
> > use CRATES from moz.build to verify they're all present in the master
> > Cargo.toml (and provide a helpful message if they're not).
> 
> "top-level Cargo.toml" sounds like a misnomer as we currently have two
> different Cargo.toml files being generated with the current patch.  And
> surely people are going to want to build host binaries (or even target ones)
> at some point...and presumably Rust usage in non-xul libraries...

My mistake, that should have been "...generates any necessary Cargo.toml files and...". I just mean that one way to approach this would be to have this patch work almost exactly as it does now, except where the step we generate Cargo.toml and Cargo.lock files doesn't happen in automation.

> 
> Putting everything in a single Cargo.toml file at the toplevel feels
> unwieldy to me, but maybe that's our best option?  We're trying to specify
> things in the build system and then have those translated into cargo-land;
> it feels like that's running into complexity as we try to have Cargo.lock
> work appropriately and future extensions to our minimal Rust usage here. 
> (Or I might just be grumpy on a Friday.)  Perhaps we should specify things
> entirely in cargo-land and then pull those into moz.build as appropriate:
> 
> RUST_LIBRARY = ["rul"]
> 
> RUST_BINARIES = ["fancy", "killer-feature"]
> 
> I think cargo has support for building specific libraries or binaries, so
> there's less magic on the actual build system side.  Doing things this way
> would also avoid squirreling settings (cargo profiles, etc.) away in
> recursivemake.py.  I guess when people want to use a new crate in libxul,
> they'd have to add it manually, but maybe that's not such a bad thing?

Comment 82

[Nathan Froyd [:froydnj]]

I was excited about putting everything into a single Cargo.toml file, as it solves a bunch of problems in one fell swoop, but we can't do that, because you can't have multiple library specifications in a Cargo.toml. :(

I think having a separate mach command to update the Cargo.toml files is unwieldly, as that mach command is essentially running |mach build-backend| but not generating a lot of things.  We could instead do something like what Android does:

http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in#557

and require people to copy things out of the objdir back to the srcdir; the srcdir versions are then the canonical copies.  We'd then invoke cargo with the srcdir versions so Cargo.lock works correctly.

Comment 83

[Lars Bergstrom (:larsberg)]

Have you considered using Cargo workspaces?
https://github.com/rust-lang/rfcs/blob/master/text/1525-cargo-workspace.md

It allows you to basically do something like:
[workspace]
members = [
  "components/servo",
  "ports/cef",
  "ports/geckolib",
]

And have a single Cargo.lock file, building all the different topics. Here's an example of what it looks like in the context of Servo:
https://github.com/servo/servo/pull/12391

The only limitation we've been hitting is that you can't use a `[replace]` for a single one of the members, otherwise we'd have landed it already in Servo :-)

Comment 84

[Nathan Froyd [:froydnj]]

(In reply to Lars Bergstrom (:larsberg) from comment #83)
> Have you considered using Cargo workspaces?
> https://github.com/rust-lang/rfcs/blob/master/text/1525-cargo-workspace.md

This is a great suggestion!  My only complaint with workspaces is it doesn't address how to share things between members.  I filed a Cargo bug with some of the things I noticed: https://github.com/rust-lang/cargo/issues/2916

Comment 85

[Nathan Froyd [:froydnj]]

Oh, workspaces also landed less than a week ago, so they'd be (currently) incompatible with our goal of requiring stable Rust to build Firefox.

I think we're just going to have multiple Cargo.toml files with multiple Cargo.lock files for now.  Perhaps when workspaces hit stable, we can consolidate the Cargo.lock files.

Comment 86

[Nathan Froyd [:froydnj]]

Attachment: part 6 - build rust code via cargo

This patch is essentially a complete rewrite to try and use Cargo's concepts of
things in the build system, rather than using crates as the units of things we
want to talk about.  It's not perfect (xul-gtest fails to link, for instance),
but I think it's a definite improvement on what we had before.  The commit
message has a bit of a rationale on why we want Cargo's concepts, rather than
crates.

The mechanism for compiling Rust libraries is much closer to how we do plain
old static libraries, which I think is another plus.  Doing things this way
also makes it significantly easier to add support for Rust binaries to be
built, which I have heard some people express interest in.

Comment 87

[Nathan Froyd [:froydnj]]

Comment on attachment 8775317 [details] [diff] [review]
part 6 - build rust code via cargo

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

::: toolkit/library/gtest/rust/Cargo.toml
@@ +9,5 @@
> +mp4parse-gtest = { path = "../../../../dom/media/gtest" }
> +
> +[lib]
> +path = "lib.rs"
> +crate-type = ["rlib"]

Changing this to "staticlib" makes xul-gtest link again.  I was trying to be too clever and failed. :(

Comment 88

[Nathan Froyd [:froydnj]]

Comment on attachment 8775317 [details] [diff] [review]
part 6 - build rust code via cargo

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

With this problem and the previous noted problem fixed, try builds look pretty green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87a501881149

::: config/expandlibs_exec.py
@@ +55,5 @@
>          return self
>          
>      def __exit__(self, type, value, tb):
>          '''Automatically remove temporary files'''
> +        return

This is also obviously detritus.

Comment 89

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8775317 [details] [diff] [review]
part 6 - build rust code via cargo

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

I have a couple of questions some of which should be addressed, but no hard blockers, so r+. Great stuff.

::: build/moz.configure/rust.configure
@@ +39,5 @@
> +    try:
> +        lines = subprocess.check_output(
> +            [cargo, 'help', 'build']
> +        ).splitlines()
> +        return any('    --frozen' in l for l in lines)

Is there a less brittle check we could do here, such as for the version?

@@ +43,5 @@
> +        return any('    --frozen' in l for l in lines)
> +    except subprocess.CalledProcessError as e:
> +        die('Failed to call cargo: %s', e.message)
> +
> +set_config('MOZ_CARGO_SUPPORTS_FROZEN', cargo_supports_frozen)

We should probably find a way to fail if MOZ_AUTOMATION and not MOZ_CARGO_SUPPORTS_FROZEN.

::: config/rules.mk
@@ +557,5 @@
>  # Dependencies which, if modified, should cause everything to rebuild
>  GLOBAL_DEPS += Makefile $(addprefix $(DEPTH)/config/,$(INCLUDED_AUTOCONF_MK)) $(MOZILLA_DIR)/config/config.mk
> +ifdef MOZ_RUST
> +ifdef CARGO_FILE
> +GLOBAL_DEPS += $(CARGO_FILE)

I'm skeptical. CARGO_FILE can change the result of building RUST_LIBRARY_FILE, but that's ok, we always build RUST_LIBRARY_FILE. Why does changing CARGO_FILE mean we need to rebuild everything?

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1172,5 @@
>          if libdef.no_expand_lib:
>              backend_file.write('NO_EXPAND_LIBS := 1\n')
>  
> +    def _process_rust_library(self, libdef, backend_file):
> +        backend_file.write_once('RUST_LIBRARY := %s\n' % libdef.basename)

Is this variable used?

@@ +1186,5 @@
>  
>      def _process_linked_libraries(self, obj, backend_file):
>          def write_shared_and_system_libs(lib):
>              for l in lib.linked_libraries:
> +                if isinstance(l, (StaticLibrary, RustLibrary)):

The comment above claims we don't call this function for rust libraries.

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +279,5 @@
>              if force_static:
>                  path = path[7:]
>              name = mozpath.basename(path)
>              dir = mozpath.dirname(path)
> +            candidates = [l for l in self._libs[name] if l.KIND == obj.KIND and l.basename == name]

I don't quite understand this change... it looks a bit like every library in self._libs[name] has basename == name.

@@ +488,5 @@
> +        if not crate_type in ('rlib', 'staticlib'):
> +            raise SandboxValidationError(
> +                'crate-type %s is not permitted for %s' % (crate_type, libname),
> +                context)
> +                

Extraneous whitespace.

@@ +714,5 @@
> +                # later on.  XXX is this the right place for this check?
> +                if is_rust_library:
> +                    staticlibs = [l for l in self._libs[libname]
> +                                  if isinstance(l, RustLibrary) and l.crate_type == 'staticlib']
> +                    if len(staticlibs) > 1:

This seems like a reasonable place for this check. This might be a little nicer implemented with `any`.

::: toolkit/library/gtest/rust/Cargo.toml
@@ +2,5 @@
> +name = "rul-gtest"
> +version = "0.1.0"
> +authors = ["[e-mail]"]
> +license = "MPL-2.0"
> +description = "There is no testing, there is only Rul"

Um...

Comment 90

[Nathan Froyd [:froydnj]]

(In reply to Chris Manchester (:chmanchester) from comment #89)
> ::: build/moz.configure/rust.configure
> @@ +39,5 @@
> > +    try:
> > +        lines = subprocess.check_output(
> > +            [cargo, 'help', 'build']
> > +        ).splitlines()
> > +        return any('    --frozen' in l for l in lines)
> 
> Is there a less brittle check we could do here, such as for the version?

I don't know that I want to do a bunch of version parsing, but maybe we already have that kind of stuff in moz.configure?

> @@ +43,5 @@
> > +        return any('    --frozen' in l for l in lines)
> > +    except subprocess.CalledProcessError as e:
> > +        die('Failed to call cargo: %s', e.message)
> > +
> > +set_config('MOZ_CARGO_SUPPORTS_FROZEN', cargo_supports_frozen)
> 
> We should probably find a way to fail if MOZ_AUTOMATION and not
> MOZ_CARGO_SUPPORTS_FROZEN.

Good idea. I'll file a followup.

> ::: config/rules.mk
> @@ +557,5 @@
> >  # Dependencies which, if modified, should cause everything to rebuild
> >  GLOBAL_DEPS += Makefile $(addprefix $(DEPTH)/config/,$(INCLUDED_AUTOCONF_MK)) $(MOZILLA_DIR)/config/config.mk
> > +ifdef MOZ_RUST
> > +ifdef CARGO_FILE
> > +GLOBAL_DEPS += $(CARGO_FILE)
> 
> I'm skeptical. CARGO_FILE can change the result of building
> RUST_LIBRARY_FILE, but that's ok, we always build RUST_LIBRARY_FILE. Why
> does changing CARGO_FILE mean we need to rebuild everything?

Hm.  I guess cargo will figure out if CARGO_FILE has been changed, and recompile what it needed based on that, and then other things will get rebuilt from that.  I'll remove this bit.

> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +1172,5 @@
> >          if libdef.no_expand_lib:
> >              backend_file.write('NO_EXPAND_LIBS := 1\n')
> >  
> > +    def _process_rust_library(self, libdef, backend_file):
> > +        backend_file.write_once('RUST_LIBRARY := %s\n' % libdef.basename)
> 
> Is this variable used?

Probably not, I'll remove it.

> @@ +1186,5 @@
> >  
> >      def _process_linked_libraries(self, obj, backend_file):
> >          def write_shared_and_system_libs(lib):
> >              for l in lib.linked_libraries:
> > +                if isinstance(l, (StaticLibrary, RustLibrary)):
> 
> The comment above claims we don't call this function for rust libraries.

Right, we don't call it on isinstance(obj, RustLibrary), but non-RustLibrary things can have RustLibrary objects linked into them.

> ::: python/mozbuild/mozbuild/frontend/emitter.py
> @@ +279,5 @@
> >              if force_static:
> >                  path = path[7:]
> >              name = mozpath.basename(path)
> >              dir = mozpath.dirname(path)
> > +            candidates = [l for l in self._libs[name] if l.KIND == obj.KIND and l.basename == name]
> 
> I don't quite understand this change... it looks a bit like every library in
> self._libs[name] has basename == name.

Thanks for catching this; this was some debugging code when I was confused; I'll remove it.

> @@ +714,5 @@
> > +                # later on.  XXX is this the right place for this check?
> > +                if is_rust_library:
> > +                    staticlibs = [l for l in self._libs[libname]
> > +                                  if isinstance(l, RustLibrary) and l.crate_type == 'staticlib']
> > +                    if len(staticlibs) > 1:
> 
> This seems like a reasonable place for this check. This might be a little
> nicer implemented with `any`.

Good point, I'll do that.

> ::: toolkit/library/gtest/rust/Cargo.toml
> @@ +2,5 @@
> > +name = "rul-gtest"
> > +version = "0.1.0"
> > +authors = ["[e-mail]"]
> > +license = "MPL-2.0"
> > +description = "There is no testing, there is only Rul"
> 
> Um...

No Ghostbusters fans in the house? ;)

Comment 91

[Nathan Froyd [:froydnj]]

A few other issues have cropped up in testing, but nothing major:

* Specifying "rlib" as the kind of library cargo should build doesn't work all that well, so we should probably disallow all crate-types besides "staticlib".  Supporting other things would require modifications to the build system anyway.

* |cargo rustc| doesn't understand -o flags being passed to the rust compiler:

https://github.com/rust-lang/cargo/issues/2559
https://github.com/rust-lang/cargo/issues/2928

which means the patch as currently structured rebuilds librul.a on every build, which means that libxul gets rebuilt every time.  This is obviously suboptimal, and I wouldn't want to land the patch in that state.

I think what this means is that we'll need to teach moz.build (specifically, RustLibrary) about how cargo lays out its objdir, and have it give that as the canonical file. :(

Comment 92

[Chris Manchester (limited bugmail, email directly)]

(In reply to Nathan Froyd [:froydnj] from comment #90)
> (In reply to Chris Manchester (:chmanchester) from comment #89)
> > ::: build/moz.configure/rust.configure
> > @@ +39,5 @@
> > > +    try:
> > > +        lines = subprocess.check_output(
> > > +            [cargo, 'help', 'build']
> > > +        ).splitlines()
> > > +        return any('    --frozen' in l for l in lines)
> > 
> > Is there a less brittle check we could do here, such as for the version?
> 
> I don't know that I want to do a bunch of version parsing, but maybe we
> already have that kind of stuff in moz.configure?

We have a class for parsing versions: http://searchfox.org/mozilla-central/rev/1112b7a5222b71a3b5b68bd531f50ded6bcbc770/python/mozbuild/mozbuild/configure/util.py#32

This may be helpful. If not we can leave this to a follow up.

> 
> > @@ +43,5 @@
> > > +        return any('    --frozen' in l for l in lines)
> > > +    except subprocess.CalledProcessError as e:
> > > +        die('Failed to call cargo: %s', e.message)
> > > +
> > > +set_config('MOZ_CARGO_SUPPORTS_FROZEN', cargo_supports_frozen)
> > 
> > We should probably find a way to fail if MOZ_AUTOMATION and not
> > MOZ_CARGO_SUPPORTS_FROZEN.
> 
> Good idea. I'll file a followup.
> 
> > ::: config/rules.mk
> > @@ +557,5 @@
> > >  # Dependencies which, if modified, should cause everything to rebuild
> > >  GLOBAL_DEPS += Makefile $(addprefix $(DEPTH)/config/,$(INCLUDED_AUTOCONF_MK)) $(MOZILLA_DIR)/config/config.mk
> > > +ifdef MOZ_RUST
> > > +ifdef CARGO_FILE
> > > +GLOBAL_DEPS += $(CARGO_FILE)
> > 
> > I'm skeptical. CARGO_FILE can change the result of building
> > RUST_LIBRARY_FILE, but that's ok, we always build RUST_LIBRARY_FILE. Why
> > does changing CARGO_FILE mean we need to rebuild everything?
> 
> Hm.  I guess cargo will figure out if CARGO_FILE has been changed, and
> recompile what it needed based on that, and then other things will get
> rebuilt from that.  I'll remove this bit.
> 
> > ::: python/mozbuild/mozbuild/backend/recursivemake.py
> > @@ +1172,5 @@
> > >          if libdef.no_expand_lib:
> > >              backend_file.write('NO_EXPAND_LIBS := 1\n')
> > >  
> > > +    def _process_rust_library(self, libdef, backend_file):
> > > +        backend_file.write_once('RUST_LIBRARY := %s\n' % libdef.basename)
> > 
> > Is this variable used?
> 
> Probably not, I'll remove it.
> 
> > @@ +1186,5 @@
> > >  
> > >      def _process_linked_libraries(self, obj, backend_file):
> > >          def write_shared_and_system_libs(lib):
> > >              for l in lib.linked_libraries:
> > > +                if isinstance(l, (StaticLibrary, RustLibrary)):
> > 
> > The comment above claims we don't call this function for rust libraries.
> 
> Right, we don't call it on isinstance(obj, RustLibrary), but non-RustLibrary
> things can have RustLibrary objects linked into them.
> 
> > ::: python/mozbuild/mozbuild/frontend/emitter.py
> > @@ +279,5 @@
> > >              if force_static:
> > >                  path = path[7:]
> > >              name = mozpath.basename(path)
> > >              dir = mozpath.dirname(path)
> > > +            candidates = [l for l in self._libs[name] if l.KIND == obj.KIND and l.basename == name]
> > 
> > I don't quite understand this change... it looks a bit like every library in
> > self._libs[name] has basename == name.
> 
> Thanks for catching this; this was some debugging code when I was confused;
> I'll remove it.
> 
> > @@ +714,5 @@
> > > +                # later on.  XXX is this the right place for this check?
> > > +                if is_rust_library:
> > > +                    staticlibs = [l for l in self._libs[libname]
> > > +                                  if isinstance(l, RustLibrary) and l.crate_type == 'staticlib']
> > > +                    if len(staticlibs) > 1:
> > 
> > This seems like a reasonable place for this check. This might be a little
> > nicer implemented with `any`.
> 
> Good point, I'll do that.
> 
> > ::: toolkit/library/gtest/rust/Cargo.toml
> > @@ +2,5 @@
> > > +name = "rul-gtest"
> > > +version = "0.1.0"
> > > +authors = ["[e-mail]"]
> > > +license = "MPL-2.0"
> > > +description = "There is no testing, there is only Rul"
> > 
> > Um...
> 
> No Ghostbusters fans in the house? ;)

Comment 93

[Ralph Giles (:rillian)]

(In reply to Nathan Froyd [:froydnj] from comment #90)

> > Is there a less brittle check we could do here, such as for the version?
> 
> I don't know that I want to do a bunch of version parsing, but maybe we
> already have that kind of stuff in moz.configure?

The --frozen option appeared in cargo 0.13.0-nightly around 2016-07-22. Unfortunately, it doesn't look like cargo supports `--version --verbose` like rustc does, so you can't reuse the code I just landed in bug 1290522. However, you should be able able to `Version(check_cmd_output([cargo, '--version']).split()[1])` or similar. I expect just checking for 0.13 will be sufficient.

> Hm.  I guess cargo will figure out if CARGO_FILE has been changed, and
> recompile what it needed based on that, and then other things will get
> rebuilt from that.  I'll remove this bit.

Cargo will do its own dependency tracking. If we can rely on the touch date of the output library to avoid relinking everything which depends on cargo output, just always invoking cargo should be sufficient.

> > > +description = "There is no testing, there is only Rul"
> > 
> > Um...
> 
> No Ghostbusters fans in the house? ;)

I'm more bothered by the US Republican reference. :/

Comment 94

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #90)
> (In reply to Chris Manchester (:chmanchester) from comment #89)
> > ::: build/moz.configure/rust.configure
> > @@ +39,5 @@
> > > +    try:
> > > +        lines = subprocess.check_output(
> > > +            [cargo, 'help', 'build']
> > > +        ).splitlines()
> > > +        return any('    --frozen' in l for l in lines)
> > 
> > Is there a less brittle check we could do here, such as for the version?
> 
> I don't know that I want to do a bunch of version parsing, but maybe we
> already have that kind of stuff in moz.configure?

I'm going to leave this one for a followup.

> > ::: config/rules.mk
> > @@ +557,5 @@
> > >  # Dependencies which, if modified, should cause everything to rebuild
> > >  GLOBAL_DEPS += Makefile $(addprefix $(DEPTH)/config/,$(INCLUDED_AUTOCONF_MK)) $(MOZILLA_DIR)/config/config.mk
> > > +ifdef MOZ_RUST
> > > +ifdef CARGO_FILE
> > > +GLOBAL_DEPS += $(CARGO_FILE)
> > 
> > I'm skeptical. CARGO_FILE can change the result of building
> > RUST_LIBRARY_FILE, but that's ok, we always build RUST_LIBRARY_FILE. Why
> > does changing CARGO_FILE mean we need to rebuild everything?
> 
> Hm.  I guess cargo will figure out if CARGO_FILE has been changed, and
> recompile what it needed based on that, and then other things will get
> rebuilt from that.  I'll remove this bit.

I remember the justification for this: editing CARGO_FILE can change the names of the libraries that we generate in some way; the conservatively correct thing to do is to assume all CARGO_FILE changes have done that, and that means re-running the build generator to make sure that the build definition is consistent with those changes.

> > @@ +714,5 @@
> > > +                # later on.  XXX is this the right place for this check?
> > > +                if is_rust_library:
> > > +                    staticlibs = [l for l in self._libs[libname]
> > > +                                  if isinstance(l, RustLibrary) and l.crate_type == 'staticlib']
> > > +                    if len(staticlibs) > 1:
> > 
> > This seems like a reasonable place for this check. This might be a little
> > nicer implemented with `any`.
> 
> Good point, I'll do that.

Ah, I remember why I went this route: I wanted to include the names of the libraries in the error message, and using |any| requires duplicating some logic somewhere.

> > ::: toolkit/library/gtest/rust/Cargo.toml
> > @@ +2,5 @@
> > > +name = "rul-gtest"
> > > +version = "0.1.0"
> > > +authors = ["[e-mail]"]
> > > +license = "MPL-2.0"
> > > +description = "There is no testing, there is only Rul"
> > 
> > Um...
> 
> No Ghostbusters fans in the house? ;)

I talked about this offline with Ralph.  We think gkrust would be a better name for the library (by analogy with gkmedias), and it avoids cryptic references, intended or otherwise.

Comment 95

[Nathan Froyd [:froydnj]]

Attachment: fix part 6 nits

This patch is intended to be applied in patch 6 as is provided essentially as
an interdiff for convenience.  The changes of interest:

* The Rust libraries have been renamed 'gkrust' and 'gkrust-test';
  Cargo.{toml,lock} and moz.build files have been updated to reflect this.

* RUST_TARGET was previously the string --target=FOO (or empty); it has been
  modified to always be FOO and to error if we cannot determine an appropriate
  --target flag for Rust.  This change makes the determination of the output of
  'cargo build' simpler.  Since this change only has an effect if --enable-rust
  (which is still optional), I think that erroring here is OK.  And since we
  eventually want to make --enable-rust the default, such a change would be
  needed anyway...

* Cargo libraries are built via 'cargo build' rather than 'cargo rustc'; the
  latter is apparently intended for debugging purposes rather than use as a
  build tool.  This change required changes in how we calculate the filename
  for RustLibrary.

* Various small comments from comment 89 have been addressed.  I've responded
  to other comments and/or filed followup bugs.

Comment 96

[Chris Manchester (limited bugmail, email directly)]

(In reply to Nathan Froyd [:froydnj] from comment #94)
> (In reply to Nathan Froyd [:froydnj] from comment #90)
> > (In reply to Chris Manchester (:chmanchester) from comment #89)
> > > ::: build/moz.configure/rust.configure
> > > @@ +39,5 @@
> > > > +    try:
> > > > +        lines = subprocess.check_output(
> > > > +            [cargo, 'help', 'build']
> > > > +        ).splitlines()
> > > > +        return any('    --frozen' in l for l in lines)
> > > 
> > > Is there a less brittle check we could do here, such as for the version?
> > 
> > I don't know that I want to do a bunch of version parsing, but maybe we
> > already have that kind of stuff in moz.configure?
> 
> I'm going to leave this one for a followup.
> 
> > > ::: config/rules.mk
> > > @@ +557,5 @@
> > > >  # Dependencies which, if modified, should cause everything to rebuild
> > > >  GLOBAL_DEPS += Makefile $(addprefix $(DEPTH)/config/,$(INCLUDED_AUTOCONF_MK)) $(MOZILLA_DIR)/config/config.mk
> > > > +ifdef MOZ_RUST
> > > > +ifdef CARGO_FILE
> > > > +GLOBAL_DEPS += $(CARGO_FILE)
> > > 
> > > I'm skeptical. CARGO_FILE can change the result of building
> > > RUST_LIBRARY_FILE, but that's ok, we always build RUST_LIBRARY_FILE. Why
> > > does changing CARGO_FILE mean we need to rebuild everything?
> > 
> > Hm.  I guess cargo will figure out if CARGO_FILE has been changed, and
> > recompile what it needed based on that, and then other things will get
> > rebuilt from that.  I'll remove this bit.
> 
> I remember the justification for this: editing CARGO_FILE can change the
> names of the libraries that we generate in some way; the conservatively
> correct thing to do is to assume all CARGO_FILE changes have done that, and
> that means re-running the build generator to make sure that the build
> definition is consistent with those changes.

In that case I think the thing to do is add CARGO_FILE to backend_input_files in recursivemake.py (we do something similar for things like test manifests and build-system related Python in the tree).

> 
> > > @@ +714,5 @@
> > > > +                # later on.  XXX is this the right place for this check?
> > > > +                if is_rust_library:
> > > > +                    staticlibs = [l for l in self._libs[libname]
> > > > +                                  if isinstance(l, RustLibrary) and l.crate_type == 'staticlib']
> > > > +                    if len(staticlibs) > 1:
> > > 
> > > This seems like a reasonable place for this check. This might be a little
> > > nicer implemented with `any`.
> > 
> > Good point, I'll do that.
> 
> Ah, I remember why I went this route: I wanted to include the names of the
> libraries in the error message, and using |any| requires duplicating some
> logic somewhere.
> 
> > > ::: toolkit/library/gtest/rust/Cargo.toml
> > > @@ +2,5 @@
> > > > +name = "rul-gtest"
> > > > +version = "0.1.0"
> > > > +authors = ["[e-mail]"]
> > > > +license = "MPL-2.0"
> > > > +description = "There is no testing, there is only Rul"
> > > 
> > > Um...
> > 
> > No Ghostbusters fans in the house? ;)
> 
> I talked about this offline with Ralph.  We think gkrust would be a better
> name for the library (by analogy with gkmedias), and it avoids cryptic
> references, intended or otherwise.

Comment 97

[Nathan Froyd [:froydnj]]

Attachment: part 6 - build rust code via cargo

Three small changes:

* Since we now require RUST_TARGET in frontend data structures, there were
  some small tweaks required to tests to provide an appropriate RUST_TARGET.

* Cargo.toml is now registered in backend_input_files rather than GLOBAL_DEPS.

* The mp4parse update script now applies the Cargo.toml changes.  Tested via
  running the update script, verifying that the patch applies, and that files
  were unchanged.

As these are all small changes, carrying over r+.

Comment 98

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a945dbf63410
part 0 - only link librul once; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/643b03aae1dd
part 1 - search for cargo when --enable-rust; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3af0b1d3473
part 2 - pass the srcdir of an rlib in the moz.build object; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/f933119e57e4
part 3 - rename RustRlibLibrary to RustCrate; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b204c2f5a7
part 4 - add pytoml to the virtualenv; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/234d8a930afa
part 5 - add a Cargo.toml for the dom/media/gtest/ code; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b71272c92cf
part 6 - build rust code via cargo; r=chmanchester

Comment 99

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this all out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2cfad6ba81 because this apparently caused a web platform test to start permafailing on 32-bit linux debug:
https://treeherder.mozilla.org/logviewer.html#?job_id=33254208&repo=mozilla-inbound

Comment 100

[Nathan Froyd [:froydnj]]

(In reply to Wes Kocher (:KWierso) from comment #99)
> I had to back this all out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2cfad6ba81 because
> this apparently caused a web platform test to start permafailing on 32-bit
> linux debug:
> https://treeherder.mozilla.org/logviewer.html#?job_id=33254208&repo=mozilla-
> inbound

This is:


12:23:54     INFO - TEST-PASS | /media-source/mediasource-remove.html | Test remove with an negative start. 
12:23:54     INFO - TEST-PASS | /media-source/mediasource-remove.html | Test remove with non-finite start. 
12:23:54     INFO - TEST-PASS | /media-source/mediasource-remove.html | Test remove with a start beyond the duration. 
12:23:54     INFO - TEST-PASS | /media-source/mediasource-remove.html | Test remove with a start larger than the end. 
12:23:54     INFO - TEST-PASS | /media-source/mediasource-remove.html | Test remove with a NEGATIVE_INFINITY end. 
12:23:54     INFO - TEST-PASS | /media-source/mediasource-remove.html | Test remove with a NaN end. 
12:23:54     INFO - TEST-PASS | /media-source/mediasource-remove.html | Test remove after SourceBuffer removed from mediaSource. 
12:23:54     INFO - TEST-PASS | /media-source/mediasource-remove.html | Test remove while update pending. 
12:23:54     INFO - TEST-PASS | /media-source/mediasource-remove.html | Test aborting a remove operation. 
12:23:54     INFO - TEST-UNEXPECTED-TIMEOUT | /media-source/mediasource-remove.html | Test remove with a start at the duration. - Test timed out
12:23:54     INFO - 
12:23:54     INFO - TEST-UNEXPECTED-TIMEOUT | /media-source/mediasource-remove.html | Test remove transitioning readyState from 'ended' to 'open'. - Test timed out
12:23:54     INFO - 
12:23:54     INFO - TEST-UNEXPECTED-TIMEOUT | /media-source/mediasource-remove.html | Test removing all appended data. - Test timed out
12:23:54     INFO - TEST-INFO | expected FAIL
12:23:54     INFO - 
12:23:54     INFO - TEST-UNEXPECTED-TIMEOUT | /media-source/mediasource-remove.html | Test removing beginning of appended data. - Test timed out
12:23:54     INFO - TEST-INFO | expected FAIL
12:23:54     INFO - 
12:23:54     INFO - TEST-UNEXPECTED-TIMEOUT | /media-source/mediasource-remove.html | Test removing the middle of appended data. - Test timed out
12:23:54     INFO - TEST-INFO | expected FAIL
12:23:54     INFO - 
12:23:54     INFO - TEST-UNEXPECTED-TIMEOUT | /media-source/mediasource-remove.html | Test removing the end of appended data. - Test timed out
12:23:54     INFO - TEST-INFO | expected FAIL
12:23:54     INFO - TEST-UNEXPECTED-TIMEOUT | /media-source/mediasource-remove.html | expected OK

...wat.

Anthony, Ralph's on PTO; is it possible someone on your team could look at this?  ("No" is an acceptable answer, but I figure somebody from the media team, possibly with experience in the mp4parse code, would have a much better/quicker experience of figuring out what's going wrong here.)  Alternatively, if we can disable that test for Linux 32-bit, would you support that?

Comment 101

[Nathan Froyd [:froydnj]]

...assuming disabling the test is even possible...

Comment 102

[Josh Matthews [:jdm]]

Add this to the relevant test's ini file:
disabled:
  if (os == "linux") and (bits == 32): bug 1231764

Comment 103

[Nathan Froyd [:froydnj]]

Attachment: fix timeouts in wpt-6 on linux debug

This patch is intended to be rolled into part 6, but there's no sense in
reposting the whole patch just to view this part.

This patch does two things:

* adds --verbose when invoking |cargo build|, which is useful and similar to
  what our C++ build does;

* changes opt-level to 1 for debug builds.

The opt-level change is consistent with what our debug builds in automation do.
This change does impact developers debugging on their local machines, but
there's no good equivalent of --enable-debug --disable-optimize, which would be
useful for developers debugging on their local machines.  Similarly, there's no
good way for automation to turn on optimization for debug builds if we were to
leave opt-level at 0.  So we pick the default that works for automation and
local developers can tweak opt-level if they really need to.  Cargo would need
a richer profile model to correspond to the options we can enable from
configure.

Comment 104

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8778387 [details] [diff] [review]
fix timeouts in wpt-6 on linux debug

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

Do we happen to know why building without optimizations causes test timeouts?

Comment 105

[Nathan Froyd [:froydnj]]

(In reply to Chris Manchester (:chmanchester) from comment #104)
> Do we happen to know why building without optimizations causes test timeouts?

No.  Presumably the parsing is slower (the mp4parse-related code is certainly much bigger when compiled without optimization), but that doesn't account for why only these tests timed out and only on 32-bit Linux.

Comment 106

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e983e50e47fb
part 0 - only link librul once; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/66d5d4c28e8d
part 1 - search for cargo when --enable-rust; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/e331cbf6534c
part 2 - pass the srcdir of an rlib in the moz.build object; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad4664e340d8
part 3 - rename RustRlibLibrary to RustCrate; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e0bcb0402b5
part 4 - add pytoml to the virtualenv; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/edfecf5c779f
part 5 - add a Cargo.toml for the dom/media/gtest/ code; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ceb4834ee8f
part 6 - build rust code via cargo; r=chmanchester

Comment 107

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e983e50e47fb
https://hg.mozilla.org/mozilla-central/rev/66d5d4c28e8d
https://hg.mozilla.org/mozilla-central/rev/e331cbf6534c
https://hg.mozilla.org/mozilla-central/rev/ad4664e340d8
https://hg.mozilla.org/mozilla-central/rev/1e0bcb0402b5
https://hg.mozilla.org/mozilla-central/rev/edfecf5c779f
https://hg.mozilla.org/mozilla-central/rev/0ceb4834ee8f

Comment 108

[Gregory Szorc [:gps]]

Comment on attachment 8772917 [details] [diff] [review]
part 4 - add pytoml to the virtualenv

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

::: build/virtualenv_packages.txt
@@ +35,5 @@
>  futures.pth:python/futures
>  ecc.pth:python/PyECC
>  xpcshell.pth:testing/xpcshell
>  pyyaml.pth:python/pyyaml/lib
> +pytoml.pth:python/pytoml

For future reference, additions to virtualenv_packages.txt also need to be made in build/mach_bootstrap.py in order for `mach` to load the modules.

virtualenv_packages.txt only impacts the virtualenv made in <objdir>/_virtualenv and Python processes run from it.

The `mach` Python process initially lives outside the virtualenv. However, some mach commands do activate the build system's virtualenv.

It's complicated and nuanced, I know :/