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

vargo: Decouple from rustup #877

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zhaofengli
Copy link
Contributor

This PR makes vargo aware of a new environment variable, VARGO_TOOLCHAIN, which can be one of the following:

  1. Unset: Uses rustup with the toolchain set in rust-toolchain.toml (current behavior)
  2. Any valid rustup toolchain specification: Uses rustup with the specified toolchain
  3. host: Uses the Rust toolchain from PATH

(3) allows Verus to be built without rustup, which is useful when the Rust toolchain is managed by something else like Nix + rust-overlay. Furthermore, (2) can be used to temporarily override the rustup toolchain to test Rust updates.

We've been using this downstream for a while since we use Nix to manage everything. We are submitting this upstream to see if this is something you would like and to stop bumping into merge conflicts 😄

Copy link
Collaborator

@utaal utaal left a comment

Choose a reason for hiding this comment

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

Hi! Thank you for the PR!
This all seems reasonable, but let me think through the implications. One small question in the meantime (Edit: now I've actually posted the question :).

Comment on lines +98 to +107
if let Ok(sysroot) = std::env::var("RUST_SYSROOT") {
child_args.push("--sysroot".to_string());
child_args.push(sysroot);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need this? Is it to set a sysroot manually, that would otherwise be set by rustup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Somehow missed the notification, my apologies)

Yes, rustup wrappers automatically set the correct LD_LIBRARY_PATH (or equivalents on other platforms) when executing the toolchain binaries, and binaries distributed by rust-overlay and fenix have their runpaths patched so the default sysroot detection logic works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! Do you know if there's a way to set the sysroot for rustc with an environment variable? Or is it just the --sysroot flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Sorry again)

I quickly looked, and I couldn't find an environment variable to do this override.

@zhaofengli
Copy link
Contributor Author

Sorry all, it's been a while but I rebased the PR on top of the latest changes.

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.

2 participants