Use Cargo.toml to fetch cargo crates #370

f-a-a · 2018-07-15T12:23:44Z

This PR attempts to solve #366.

As noted by @robbym, cargo install works by downloading sources to a temporary directory, compiles it and output binaries which in our case that's a step ahead since we wanted to rely on //build_extra/rust/BUILD.gn for building. (p.s: if i understand Rust correctly...)

Here, I'm proposing to introduce a fetch_cargo_crates.py script that I got cues from Fuschia's codebase: list_3p_crates.py and compile_3p_crates.py.

To briefly explain the flow of operation:

Running ./tools/build_third_party.py
- symlinks Cargo.toml from the root dir to third_party dir
- executes fetch_cargo_crates.py
./tools/fetch_cargo_crates.py will
- parse Cargo.toml using pytoml library (pulled in via gclient)
- extract crate name and crate version (with special handling for git defined crates)
- call cargo clone with cargo name and crate version
- and finally outputs cloned repositories to third_party/rust_crates

Caveats

No Cargo.lock is generated
A cargo subcommand needs to be added (i.e. https://github.com/ehuss/cargo-clone-crate)
- Installing seems to take some time... 🙍
- I opted to use this rather than cargo-clone since it is recently published and potentially more maintained.
clone_crate function within the fetch_cargo_crates.py script is rather brittle, it doesn't handle most of Cargo.toml semantics.

Alternative

I have also made an alternative implementation in build_extra/rust/BUILD.gn... but can't seem to get it working in the build step, only in gen step. I discarded the idea since it felt out of place to pull dependencies during the gen step. But if you are interested to see the changes: it's here f-a-a/cargo_fetch2

Outputs like this instead:

cc: @ry @robbym

robbym · 2018-07-15T17:29:01Z

Running git clean -xdf and then ./tools/build_third_party.py gives me errors:

python /home/robby/Desktop/deno/tools/fetch_cargo_crates.py
    Updating registry `https://github.com/rust-lang/crates.io-index`
error: package 'libc:0.2.42' not found

    Updating registry `https://github.com/rust-lang/crates.io-index`
error: package 'url:1.7.1' not found

    Updating registry `https://github.com/rust-lang/crates.io-index`
error: package 'matches:0.1.6' not found

    Updating registry `https://github.com/rust-lang/crates.io-index`
error: package 'unicode-bidi:0.3.4' not found

    Updating registry `https://github.com/rust-lang/crates.io-index`
error: package 'unicode-normalization:0.1.7' not found

But it seems to build just fine.

f-a-a · 2018-07-16T06:47:58Z

I tried running git clean -xdf myself and these were returned instead:

python /Users/Faris/Forks/deno/tools/fetch_cargo_crates.py
fatal: destination path 'libc' already exists and is not an empty directory.
Error: `git clone` did not finish successfully.

fatal: destination path 'rust-url' already exists and is not an empty directory.
Error: `git clone` did not finish successfully.

fatal: destination path 'rust-std-candidates' already exists and is not an empty directory.
Error: `git clone` did not finish successfully.

fatal: destination path 'unicode-bidi' already exists and is not an empty directory.
Error: `git clone` did not finish successfully.

fatal: destination path 'unicode-normalization' already exists and is not an empty directory.
Error: `git clone` did not finish successfully.

which I think is expected because git clean -xdf skips removing rust_crates folder:

Skipping repository third_party/v8
Skipping repository third_party/pytoml
Skipping repository third_party/zlib
Skipping repository third_party/cpplint
Skipping repository third_party/rust_crates/libc
Skipping repository third_party/rust_crates/unicode-normalization
Skipping repository third_party/rust_crates/rust-std-candidates
Skipping repository third_party/rust_crates/unicode-bidi
Skipping repository third_party/rust_crates/rust-url
Skipping repository third_party/flatbuffers
Removing third_party/Cargo.toml
Removing third_party/markupsafe
Removing third_party/llvm-build
Removing third_party/jinja2
Removing third_party/node_modules
Removing third_party/.gclient
Removing third_party/.gclient_entries
Removing third_party/yarn.lock
Removing third_party/package.json
Removing third_party/googletest
Removing tools/util.pyc

ry

@f-a-a Cool thanks for looking into this! So far I'm not so happy with the complexity of this - the current gclient based system is simpler.

+75 −28

Ideally this patch would be roughly switching out the repo URLs in gclient_config.py for some lines in Cargo.toml -- not writing new programs.

ry · 2018-07-16T15:38:14Z

.travis.yml

@@ -28,6 +28,8 @@ install:
 - curl -sSf https://sh.rustup.rs | sh -s -- -y
 - export PATH=$HOME/.cargo/bin:$PATH
 - rustc --version
+ # ./tools/fetch_cargo_crates.py depends on this subcommand (https://github.com/ehuss/cargo-clone-crate)
+ - cargo install cargo-clone-crate


This should be part of build_third_party.py. It's the analog of yarn for rust - and yarn is called there.

ry · 2018-07-16T15:38:43Z

Cargo.toml

+url = "1.7.1"
+matches = "0.1.6"
+unicode-bidi = "0.3.4"
+unicode-normalization = "0.1.7"


Only libc and url are necessary - the others are transitive deps.

ry · 2018-07-16T15:39:35Z

gclient_config.py

@@ -36,30 +36,7 @@
    'flatbuffers'
 }, {
    'url':
-    'https://github.com/rust-lang/libc.git@8a85d662b90c14d458bc4ae9521a05564e20d7ae',
+    'https://github.com/avakar/pytoml.git@cb92445ca769b3966b3976cd69f5140761f15843',


I'm not very keen on adding pytoml as a dependency.

ry · 2018-07-16T15:42:14Z

tools/fetch_cargo_crates.py

+    )
+    print(stderr)
+
+def main():


Is all of this really necessary? I haven't looked into it - but it would be very much preferable to not introduce an alternate custom Cargo.toml parser. The idea of using Cargo is to simply the updating of packages - it will only be used when someone adds a dependency.

I was hoping we could just do something like (i have not researched this - this certainly does not work)

cargo download-deps --download_path=third_party/rust_crates/ --config=./Cargo.toml

I'm not sure it's worth the complexity of maintaining this program. I would rather just manually maintain the dependencies.

I laid out the issues here: https://github.com/ry/deno/issues/366
That functionality doesn't exist as a single tool.

They are trying to add it to vanilla cargo here: rust-lang/cargo#1861

TBH, everything would be much easier if we separated the Rust build from the libdeno build. Cargo is very opinionated in regards to project structure, and most of the Rust tooling expects a certain structure.

It seems like they want to improve Cargo in that regard: rust-lang/rfcs#2136

Ah thanks, I missed those comments.

everything would be much easier if we separated the Rust build from the libdeno build

We should do that but not yet...

The build is very complex - much more so than a typical Rust library. (We build V8; we build a separate executable (snapshot_creator) which outputs a C++ file containing the V8 snapshotted heap, and then compile that C++ file into the final executable for fast startup; we generate typescript and C++ files from flatbuffers; we compile flatc before doing the previous step; we run TSC and Parcel to generate bundles of typescript; we produce intermediate targets for testing.) We want to precisely control dependencies and build configurations. We want to eventually be able to link into other parts of the chrome infrastructure (particularly Angle for webgl support). For this reason we want GN to drive the build, not Cargo (which is not designed for complex builds like this).

Although we aim to implement most of the privileged side of Deno in Rust, the boundaries are currently fluid. In particular, flatbuffer support for Rust is not there yet, so we do a lot of the message passing in C++. These interfaces will change.

For these reasons it makes more sense to treat the rust code as a native code library that we link to, rather than the system that drives compilation. As things solidify and we have more rust code, it will make sense to separate things out more... but I'm happy with the rust code being in src now.. That said, I'm open to changes - let's hash it out on gitter if you have a specific idea.

@ry I can take a stab at writing the Cargo subcommand as you suggested, I'll update this PR when I have it ready... but my Rust-fu is rusty! (ba dum tss)

@f-a-a I threw something together that basically does what we want: https://github.com/robbym/cargo-download-deps
Install via cargo install --git https://github.com/robbym/cargo-download-deps.git
Usage: cargo download-deps ./path/to/Cargo.toml ./path/to/dest

I'm probably missing a bunch of edge cases, but its mostly proof of concept.

The toml needs some dummy stuff:

[package] name = "dummy" version = "0.1.0"

And also an empty ./src/lib.rs

Dependencies can be added as usual:

[dependencies] url = {git = "https://github.com/servo/rust-url.git", rev = "2efa106"}

@robbym i'd be happy to contribute/maintain to your repo, also would you consider supporting the semantics @ry suggest? (i.e. cargo download-deps --download_path=third_party/rust_crates/ --config=./Cargo.toml) rather than positional arguments?

p/s: let me see if i can amend this PR with your cargo subcommand

@f-a-a Yeah I'm fine with any modifications. I don't really want to own or maintain this. Was just an example.

Edit: Changed to use named args.

f-a-a · 2018-07-17T11:41:46Z

since the way forward for #366 is to offload the operations to cargo subcommands, i'll close this PR for now.

Thanks @ry and @robbym for reviewing and testing!!

Faris Amali Alis added 6 commits July 15, 2018 18:40

chore: Remove rust deps and add pytoml to gclient_config.py

1b69edd

Introduce Cargo.toml and symlink to third_party dir

c64fb9b

Add fetch_cargo_crates.py script and run in build_third_party.py

165f19a

Add deps to Cargo.toml and update rust/BUILD.gn

37c4080

Update Travis to install cargo-clone-crate

5831e01

Add rust_crates_path exists check

e232319

ry reviewed Jul 16, 2018

View reviewed changes

f-a-a closed this Jul 17, 2018

f-a-a mentioned this pull request Jul 17, 2018

Use Cargo to install rust deps into //third_party/rust_crates #366

Closed

f-a-a deleted the cargo_fetch branch August 5, 2018 05:31

piscisaureus pushed a commit to piscisaureus/deno that referenced this pull request Oct 7, 2019

remove dead code from log/test.ts (denoland#370)

e2c1870

hardfist pushed a commit to hardfist/deno that referenced this pull request Aug 7, 2024

chore: fix build (denoland#370)

248baf6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Cargo.toml to fetch cargo crates #370

Use Cargo.toml to fetch cargo crates #370

f-a-a commented Jul 15, 2018 •

edited

Loading

robbym commented Jul 15, 2018

f-a-a commented Jul 16, 2018

ry left a comment

ry Jul 16, 2018

ry Jul 16, 2018

ry Jul 16, 2018

ry Jul 16, 2018 •

edited

Loading

robbym Jul 16, 2018

robbym Jul 16, 2018

robbym Jul 16, 2018

ry Jul 16, 2018 •

edited

Loading

f-a-a Jul 17, 2018 •

edited

Loading

robbym Jul 17, 2018

f-a-a Jul 17, 2018 •

edited

Loading

robbym Jul 17, 2018 •

edited

Loading

f-a-a commented Jul 17, 2018 •

edited

Loading

Use Cargo.toml to fetch cargo crates #370

Use Cargo.toml to fetch cargo crates #370

Conversation

f-a-a commented Jul 15, 2018 • edited Loading

Caveats

Alternative

robbym commented Jul 15, 2018

f-a-a commented Jul 16, 2018

ry left a comment

Choose a reason for hiding this comment

ry Jul 16, 2018

Choose a reason for hiding this comment

ry Jul 16, 2018

Choose a reason for hiding this comment

ry Jul 16, 2018

Choose a reason for hiding this comment

ry Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

robbym Jul 16, 2018

Choose a reason for hiding this comment

robbym Jul 16, 2018

Choose a reason for hiding this comment

robbym Jul 16, 2018

Choose a reason for hiding this comment

ry Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

f-a-a Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

robbym Jul 17, 2018

Choose a reason for hiding this comment

f-a-a Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

robbym Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

f-a-a commented Jul 17, 2018 • edited Loading

f-a-a commented Jul 15, 2018 •

edited

Loading

ry Jul 16, 2018 •

edited

Loading

ry Jul 16, 2018 •

edited

Loading

f-a-a Jul 17, 2018 •

edited

Loading

f-a-a Jul 17, 2018 •

edited

Loading

robbym Jul 17, 2018 •

edited

Loading

f-a-a commented Jul 17, 2018 •

edited

Loading