-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 to install rust deps into //third_party/rust_crates #383
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! This is much better!
Cargo.toml
Outdated
[dependencies] | ||
url = {git = "https://github.com/servo/rust-url.git", rev = "fbe5e50316105482dcd53d2dabb148c445a5f4cd"} | ||
libc = {git = "https://github.com/rust-lang/libc.git", rev = "8a85d662b90c14d458bc4ae9521a05564e20d7ae"} | ||
log = {git = "https://github.com/rust-lang-nursery/log.git", rev = "7a6028666b6e21e8cd39203e1f1114afc44333ef"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m fine with using normal versions instead of git + rev - I think that’s preferable ?
Cargo.toml
Outdated
version = "0.0.0" | ||
|
||
[lib] | ||
path = "empty.rs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all of the above lines necessary ?
Go ahead and use “deno” instead of “dummy” ... maybe add a comment here about how this is only used for dependency management and called from tools/build_third_party.py only when adding deps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The [lib] stuff is not. Forgot to remove it when I was experimenting.
Will add the comment.
I added the RLS thing to .gitignore because RLS starts generating stuff when it detects Cargo.toml at the root of your project (in VSCode). |
tools/build_third_party.py
Outdated
@@ -29,3 +29,4 @@ | |||
remove_and_symlink(join("v8", "third_party", "markupsafe"), "markupsafe") | |||
run(["gclient", "sync", "--shallow", "--no-history"]) | |||
run(["yarn"]) | |||
run(["cargo", "fetch", "--manifest-path=../Cargo.toml"], False, {'CARGO_HOME': os.getcwd() + '/rust_crates'}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than using cwd, use the root_path defined above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh didn't see that.
tools/build_third_party.py
Outdated
@@ -29,3 +29,4 @@ | |||
remove_and_symlink(join("v8", "third_party", "markupsafe"), "markupsafe") | |||
run(["gclient", "sync", "--shallow", "--no-history"]) | |||
run(["yarn"]) | |||
run(["cargo", "fetch", "--manifest-path=../Cargo.toml"], False, {'CARGO_HOME': third_party_path + '/rust_crates'}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is over 80 cols - run ./tools/format.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
(Just two more comments...)
tools/build_third_party.py
Outdated
run(["cargo", "fetch", "--manifest-path=../Cargo.toml"], False, | ||
{'CARGO_HOME': third_party_path + '/rust_crates'}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: you don't have to specify the quiet arg:
run(["cargo", "fetch", "--manifest-path=../Cargo.toml"], env={'CARGO_HOME': third_party_path + '/rust_crates'})
Move more tests: - serialize_deserialize_test - Various error stack tests - Promise rejection order test Switched from file:// namespace to test:// namespace so errors are identical across systems. JsError during module evaluation are printed as `[ERR] ` in output.
Addresses https://github.com/ry/deno/issues/366