Skip to content

Install Rust through global, pre-installed Rustup#11256

Merged
Eric-Arellano merged 18 commits into
pantsbuild:masterfrom
Eric-Arellano:rustup
Dec 23, 2020
Merged

Install Rust through global, pre-installed Rustup#11256
Eric-Arellano merged 18 commits into
pantsbuild:masterfrom
Eric-Arellano:rustup

Conversation

@Eric-Arellano
Copy link
Copy Markdown
Contributor

This reduces lots of complexity and brings us closer in tooling to a typical Rust project.

This should also reduce space consumption for some developers because they can now leverage the Rust installation they may already have installed globally.

I don't want to battle getting macOS CI to work with modern Rustup expecting a more modern version of `xz` than is installed. `brew install xz` takes multiple minutes because it tries updating brew.

Instead, we go back to the old-school rust-toolchain format. We don't need to specify components anyways; the default profile already includes rustfmt and clippy.
It results in a multiple minute Brew upgrade. Instead, we'll rely on a fix from rustup that will be released shortly.
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@Eric-Arellano Eric-Arellano changed the title WIP: Install Rust through global, pre-installed Rustup Install Rust through global, pre-installed Rustup Dec 23, 2020
@Eric-Arellano Eric-Arellano marked this pull request as ready for review December 23, 2020 19:07
Copy link
Copy Markdown
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Awesome stuff. Thanks a lot for doing this, and to @tdyas for nuking the legacy gRPC install.

Comment on lines +256 to +263
def _install_rust(homedir: str = "${HOME}") -> List[str]:
rustup = (
"curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y "
"--default-toolchain none"
)
# This will mutate the PATH to add `rustup` and `cargo`.
activate_rustup = f"source {homedir}/.cargo/env"
return [rustup, activate_rustup]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it worth doing this in the docker image? Fine as a followup if so.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, but it would mean duplicating this installation logic across the Dockerfile and still having it in .travis.yml for the non-Docker shards like macOS.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm fine with us duplicating this installation code if it means that every build avoids downloading rustup. Frankly, all "fixtures" should be installed into the Docker image and never downloaded during a build. Downloading during a build is another way a build could be unstable due to rate limits or general network errors that could (and have likely) occurred.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, downloading rustup might make sense, but actually downloading a toolchain would be a bit much. Would mean you'd have to edit the docker image to adjust rust-toolchain.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For reference, Toolchain's internal Rust CI image just uses rustup component add to add the toolchain into the Docker image.

Comment on lines +31 to +32
# Pants's auto-detection fails for the Docker image used in CI.
ENV PANTS_DYNAMIC_UI=false
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be in pants.travis-ci.toml instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, lol. It is already. But PANTS_CONFIG_FILES is ignored by the docker environment.

@Eric-Arellano Eric-Arellano merged commit 547ab6a into pantsbuild:master Dec 23, 2020
@Eric-Arellano Eric-Arellano deleted the rustup branch December 23, 2020 20:22
@stuhood stuhood mentioned this pull request Dec 29, 2020
stuhood added a commit that referenced this pull request Dec 29, 2020
### Internal Changes

* Fix too many line breaks for options in docs site ([#11377](#11377))

* Use more newlines in options help messages ([#11372](#11372))

* Fix some newlines not rendering correctly on docsite ([#11371](#11371))

* Fix formatting for `./pants subsystems` and `./pants targets` ([#11368](#11368))

* Simplify `generate_docs.py` to no longer require manually generating the input data ([#11363](#11363))

* Use the whole description for `./pants help $subsystem` ([#11361](#11361))

* Use newlines in `./pants help $target_type` ([#11355](#11355))

* [internal] Install rustup on the lint shards. ([#11392](#11392))

* Install Rust through global, pre-installed Rustup ([#11256](#11256))

* Revert "Add goals to run_info (#11374)" ([#11379](#11379))

* Add goals to run_info ([#11374](#11374))

* Update all rust dependencies within their declared ranges. ([#11366](#11366))

* Update to nails 0.11.0. ([#11370](#11370))

* cleanup some futures-related references ([#11360](#11360))

* [internal] Switch to toolchain pants plugin ([#11357](#11357))

* Simplify and fix the @ensure_daemon decorator. ([#11356](#11356))

* restore some try_future uses that were recently removed ([#11353](#11353))

* remove all remaining uses of futures v0.1.x (in favor of v0.3.x) ([#11352](#11352))

* Prepare the 1.30.2 release notes. ([#11351](#11351))

* remove remaining futures v0.1.x uses from fs/store and downstream callers ([#11346](#11346))

* fix Prost revision that did not get updated in prior PR ([#11345](#11345))

* add extension method for prost::Message for encoding to Bytes ([#11344](#11344))

* Stop installing Cmake, Go, and Protoc when bootstrapping Rust ([#11336](#11336))

* Automatically set the version in generate_docs.py ([#11337](#11337))

* Prepare 2.1.1. ([#11334](#11334))

* Prepare 2.0.1. ([#11333](#11333))

* Prepare the 1.30.2rc3 release notes. ([#11327](#11327))

* Prepare 2.0.1rc4 (again). ([#11323](#11323))

* Add type annotations around Subsystem class ([#11389](#11389))

* Support deprecation_start_version in help info. ([#11350](#11350))

[ci skip-rust]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants