Closed Bug 1249511 Opened 4 years ago Closed 4 years ago

Add cargo to tooltool

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: valentin, Assigned: rillian)

References

Details

Attachments

(2 files)

No description provided.
Is there context for this? I don't think we want to use cargo as part of the Gecko build.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> Is there context for this? I don't think we want to use cargo as part of the
> Gecko build.

This is a blocker to using rust-url. It currently depends on a handful of crates, and the strategy we use for mp4parse is too awkward for this.
Are there any issues with using cargo in the build? It only needs to generate a static lib for us, just as rustc does.
Alex had some comments on this in: https://bugzilla.mozilla.org/show_bug.cgi?id=1231764#c4

I believe that the approach we have been looking at most closely is the fourth one that Alex described (vendoring all of the dependencies in-tree). There are a bunch of unknowns here, though:

1) If they are all vendored in-tree, do we then shell out to rustc normally via the mozbuild rules and ignore Cargo.toml at build time? 
2) Are the tools that perform this vendoring produced and supported by the Rust/Cargo team or would they be scripts that we write and land in gecko? (described in https://github.com/rust-lang/cargo/issues/1926)

I think that these issues around cargo vendoring and two-way source sync (more links at https://wiki.mozilla.org/Oxidation) are the big ones that we probably need to have a bigger meeting around to flesh out designs and figure out who's building which pieces.
(In reply to Valentin Gosu [:valentin] from comment #2)
> This is a blocker to using rust-url. It currently depends on a handful of
> crates, and the strategy we use for mp4parse is too awkward for this.
> Are there any issues with using cargo in the build? It only needs to
> generate a static lib for us, just as rustc does.

Yeah, we understand that this is broken right now. froydnj is working on something better in bug 1163224, but it still might not fully meet your needs.

Running `cargo build` during the Gecko build is going to be a non-starter unless we have some way to tell cargo not to hit the network at all, and provide a local path to its dependencies.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)

> Running `cargo build` during the Gecko build is going to be a non-starter
> unless we have some way to tell cargo not to hit the network at all, and
> provide a local path to its dependencies.

Alex has done some work to support this recently in https://github.com/rust-lang/cargo/pull/2361, but it hasn't landed yet. I think once it does we might be able to use it either by checking in tarballs, or pulling a dep blob from tooltool, or the artifact mechanism if that works in buildbot. Sounds like another round of work will be needed after that if we want cargo to use in-tree sources directly.

I don't think it much matters whether the gecko build system invokes cargo or rustc to build things, but we must do something to support cargo's manifest formats and interaction with crates.io. Anything else would be a disaster for developer productivity.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> Yeah, we understand that this is broken right now. froydnj is working on
> something better in bug 1163224, but it still might not fully meet your
> needs.

That's a great start, and required if we want to link with multiple staticlibs.

> 
> Running `cargo build` during the Gecko build is going to be a non-starter
> unless we have some way to tell cargo not to hit the network at all, and
> provide a local path to its dependencies.

If Cargo.toml only references local crates, it doesn't hit the network at all.

(In reply to Ralph Giles (:rillian) from comment #5)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> 
> I don't think it much matters whether the gecko build system invokes cargo
> or rustc to build things, but we must do something to support cargo's
> manifest formats and interaction with crates.io. Anything else would be a
> disaster for developer productivity.

Right, rebuilding cargo's features in the build system would be a waste of time. And building complex crates such as rust-url with only rustc would be a pain.
It's currently possible to use cargo in the build - it requires changing Cargo.toml from the upstream crates to point to local paths, and bug 1163224 would allow us to use several rust libs in the tree.
Anyway, I can upload cargo for you if you want to test try pushes. Let me know if that would be helpful.

Are you planning to do the build system patches to call cargo? I've been thinking a new CRATES mozbuild variable would make more sense than the current scheme of putting the top-level lib.rs in SOURCES.
(In reply to Ralph Giles (:rillian) from comment #7)
> Anyway, I can upload cargo for you if you want to test try pushes. Let me
> know if that would be helpful.

I was about to look into that, but if you could do it, that would be great! Thanks!

> Are you planning to do the build system patches to call cargo? I've been
> thinking a new CRATES mozbuild variable would make more sense than the
> current scheme of putting the top-level lib.rs in SOURCES.

For now I can get away with a Makefile.in that calls cargo, but after landing bug 1163224 it might be worth adding mozbuild support.
Uploaded cargo for linux64 from rust-1.6.0 stable release package, repacked so it will appear as rustc/bin/cargo. Untested! Let me know if you need other archs or hit problems with this one.

[
{
"algorithm": "sha512",
"visibility": "public",
"filename": "rustc-cargo-1.6.0-x86_64-unknown-linux-gnu.tar.xz",
"unpack": true,
"digest": "9ce3e540ebae1a2b184af9e60e57f0d0f74fed59d94c8b23403512f9e7ba5e2252474cdfc1679b29072c3fc52af7cc6ebbc1c25f1f7a90cbb7d344e7fc5c54c4",
"size": 2852376
}
]
Alex has a PR to add `--network-is-error` to cargo:
https://github.com/rust-lang/cargo/pull/2811

We'll want to use that when it lands, so we'll need to use nightly cargo for a while. I think we will only *need* that in CI, for local developer builds we can skip using that and let them use whatever version of cargo they have installed.
Blocks: 1231764
I've updated my repack script to include cargo, and it's currently included in tooltool for linux and windows builds. If mac can wait for bug 1188030 that's the easiest resolution there. If we need it sooner I can add cargo manually. Android isn't yet supported.
Depends on: 1188030
The android case should just fall out once we have support on Linux x86-64, since cargo is a host tool, and the Android builds would be using the Linux-hosted rustc.
Ralph said he would be willing to upload cargo tooltool packages.  My sense is that we want to use stable cargo (i.e. without the --frozen flag that just landed) to more closely match what people are compiling with locally.  Does that seem reasonable?
Assignee: nobody → giles
Flags: needinfo?(giles)
I would like to have a nightly cargo for CI so we can use --frozen when it's available. I think it's fine to check for it in configure and only use it if available.
Repacks of upstream builds cargo 0.13.0-nightly (664125b 2016-07-19)
for each host platform. Unpacks into cargo/bin/cargo.

This version supports `cargo build --frozen` to disallow
network access during the build.

Review commit: https://reviewboard.mozilla.org/r/66238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66238/
Attachment #8773514 - Flags: review?(mshal)
Flags: needinfo?(giles)
Attachment #8773514 - Flags: review?(mshal) → review+
Attachment #8773571 - Flags: review?(mshal) → review+
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d28560cc502b
Add cargo nightly to tooltool. r=mshal
https://hg.mozilla.org/integration/mozilla-inbound/rev/790d7654501c
Make tooltool cargo available in the environment. r=mshal
https://hg.mozilla.org/mozilla-central/rev/d28560cc502b
https://hg.mozilla.org/mozilla-central/rev/790d7654501c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.