Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Cargo.toml to fetch cargo crates #370

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- ./tools/build_third_party.py
# ccache needs the custom LLVM to be in PATH and other variables.
- export PATH=`pwd`/third_party/llvm-build/Release+Asserts/bin:$PATH
Expand Down
11 changes: 11 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
[package]
name = "deno"
version = "0.1.0"
authors = "Ryan Dahl <[email protected]>"

[dependencies]
libc = "0.2.42"
url = "1.7.1"
matches = "0.1.6"
unicode-bidi = "0.3.4"
unicode-normalization = "0.1.7"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

6 changes: 3 additions & 3 deletions build_extra/rust/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ rust_component("libc") {
}

rust_component("url") {
source_root = "$crates/url/src/lib.rs"
source_root = "$crates/rust-url/src/lib.rs"
extern = [
":matches",
":idna",
Expand All @@ -31,15 +31,15 @@ rust_component("url") {
}

rust_component("percent_encoding") {
source_root = "$crates/url/percent_encoding/lib.rs"
source_root = "$crates/rust-url/percent_encoding/lib.rs"
}

rust_component("matches") {
source_root = "$crates/rust-std-candidates/matches/lib.rs"
}

rust_component("idna") {
source_root = "$crates/url/idna/src/lib.rs"
source_root = "$crates/rust-url/idna/src/lib.rs"
extern = [
":matches",
":unicode_bidi",
Expand Down
27 changes: 2 additions & 25 deletions gclient_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,30 +36,7 @@
'flatbuffers'
}, {
'url':
'https://github.com/rust-lang/libc.git@8a85d662b90c14d458bc4ae9521a05564e20d7ae',
'https://github.com/avakar/pytoml.git@cb92445ca769b3966b3976cd69f5140761f15843',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'name':
'rust_crates/libc'
}, {
'url':
'https://github.com/servo/rust-url.git@fbe5e50316105482dcd53d2dabb148c445a5f4cd',
'name':
'rust_crates/url'
}, {
# Needed for url.
'url':
'https://github.com/SimonSapin/rust-std-candidates.git@88a017b79ea146d6fde389c96982fc7518ba98bf',
'name':
'rust_crates/rust-std-candidates'
}, {
# Needed for url.
'url':
'https://github.com/servo/unicode-bidi.git@32c81729db0ac90289ebeca9e0d4886f264e724d',
'name':
'rust_crates/unicode-bidi'
}, {
# Needed for url.
'url':
'https://github.com/behnam/rust-unicode-normalization.git@3898e77b110246cb7243bf29b896c58d8975304a',
'name':
'rust_crates/unicode-normalization'
'pytoml'
}]
3 changes: 3 additions & 0 deletions tools/build_third_party.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@

root_path = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
third_party_path = join(root_path, "third_party")
tools_path = join(root_path, "tools")

try:
os.makedirs(third_party_path)
except:
pass
os.chdir(third_party_path)
remove_and_symlink(join("..", "gclient_config.py"), ".gclient")
remove_and_symlink(join("..", "Cargo.toml"), "Cargo.toml")
remove_and_symlink(join("..", "package.json"), "package.json")
remove_and_symlink(join("..", "yarn.lock"), "yarn.lock")
remove_and_symlink(join("v8", "third_party", "googletest"), "googletest")
Expand All @@ -29,3 +31,4 @@
remove_and_symlink(join("v8", "third_party", "markupsafe"), "markupsafe")
run(["gclient", "sync", "--shallow", "--no-history"])
run(["yarn"])
run(["python", join(tools_path, "fetch_cargo_crates.py")])
54 changes: 54 additions & 0 deletions tools/fetch_cargo_crates.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
#!/usr/bin/env python
# Inspired by
# https://fuchsia.googlesource.com/build/+/master/rust/list_3p_crates.py
# https://fuchsia.googlesource.com/build/+/master/rust/compile_3p_crates.py
# Copyright 2018 The Fuchsia Authors. All rights reserved.
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.

import os
from os.path import join, dirname, realpath
import sys
import subprocess

root_path = dirname(dirname(realpath(__file__)))
third_party_path = join(root_path, "third_party")

sys.path += [join(third_party_path, "pytoml")]
import pytoml

def run_command(args, env, cwd):
job = subprocess.Popen(args, env=env, cwd=cwd, stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
stdout, stderr = job.communicate()
return (job.returncode, stdout, stderr)

def clone_crate(name, version):
rust_crates_path = join(third_party_path, "rust_crates")
if not os.path.exists(rust_crates_path):
os.makedirs(rust_crates_path)
call_args = [
"cargo", "clone",
"%s:%s" % (name, version)
]
env = os.environ.copy()
_, _, stderr = run_command(
call_args,
env,
join(third_party_path, "rust_crates")
)
print(stderr)

def main():
Copy link
Member

@ry ry Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ry ry Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

  1. 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).
  2. 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.

Copy link
Author

@f-a-a f-a-a Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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"}

Copy link
Author

@f-a-a f-a-a Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

@robbym robbym Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

cargo_toml_path = join(third_party_path, "Cargo.toml")
with open(cargo_toml_path, "r") as file:
cargo_toml = pytoml.load(file)
for key, value in cargo_toml["dependencies"].items():
if type(value) is dict:
git = value.get("git")
clone_crate(key, git)
else:
clone_crate(key, value)

if __name__ == '__main__':
sys.exit(main())