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

Replace rust overlay with rustup package #916

Merged
merged 1 commit into from
Aug 1, 2023
Merged

Conversation

doubledup
Copy link
Contributor

Turns out things work just fine without the overlay. Please test this on your machines though in case I missed something when cleaning.

After a git clean and rustup self uninstall, the E2E env starts up in a pure shell (with no local Rust installation) and smoketests build & pass. The only errors I got were from forgetting to run the init.sh and make-bindings.sh scripts, but those are already mentioned in the READMEs 😅

One issue I can think of is if the Nix rustup version in an impure shell behaves in a way that conflicts with a local one. This seems unlikely, but I've reinstalled Rust locally & compiled other projects, then switched back to Snowbridge in an impure shell and the E2E stack still works.

Copy link
Contributor

@yrong yrong left a comment

Choose a reason for hiding this comment

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

Nice!

@yrong
Copy link
Contributor

yrong commented Jul 31, 2023

an impure shell behaves in a way that conflicts with a local one

With the rust-toolchain.toml explicitly declared this is not a problem IMO.

@doubledup doubledup merged commit e67ca14 into main Aug 1, 2023
@doubledup doubledup deleted the david/nix-rustup branch August 1, 2023 00:01
@vgeddes
Copy link
Collaborator

vgeddes commented Aug 1, 2023

@doubledup This is still missing a few things:

  • The scripts/init.sh script needs to explicitly install the rust toolchain as well a nightly for rustfmt.
  • Rustup is going to install the toolchains outside the nix env into the $HOME/.rustup, along with other toolchains that have nothing to do with the nix env. You should set RUSTUP_HOME in flake.nix so that the env has its own rustup dir.

@doubledup
Copy link
Contributor Author

We automatically install the stable toolchain when running web/packages/test/scripts/start-services.sh (I've tested this by removing my local rustup installation), but we do need to install the nightly version. Might as well make the installation explicit in init.sh.

Installing toolchains in the project directory doesn't affect which versions are selected: the stable toolchain version is selected by the toolchain file and we don't have a way to lock the nightly version. This might help if there's some state kept in the rustup directory though.

@vgeddes
Copy link
Collaborator

vgeddes commented Aug 1, 2023

We automatically install the stable toolchain when running web/packages/test/scripts/start-services.sh (I've tested this by removing my local rustup installation), but we do need to install the nightly version. Might as well make the installation explicit in init.sh.

Yes, I'd rather be explicit about installing the toolchain after initialising the nix env.

Installing toolchains in the project directory doesn't affect which versions are selected: the stable toolchain version is selected by the toolchain file and we don't have a way to lock the nightly version. This might help if there's some state kept in the rustup directory though.

Well the other problem is that if you run rustup toolchain list inside the nix env, it's going to show all toolchains, even those installed outside of the nix env. Which is confusing. That's why I'd like to completely segregate Nix's rustup state from the outside world.

@doubledup
Copy link
Contributor Author

doubledup commented Aug 1, 2023

Unfortunately there's no explicit command for installing the version in the toolchain file: rust-lang/rustup#2686

There are other commands that will implicitly install from the toolchain file, but there's talk of removing that behaviour: rust-lang/rustup#1397

I'm seeing two options:

  • We can explicitly install a nightly version, but we need to duplicate the stable version in init.sh in order to install it explicitly. Here we need to update the Rust version in 2 places when upgrading Rust. If we miss this, fresh machines will install the old Rust version but will still use the correct version from the toolchain file.
  • We can use something like rustup show to implicitly install the version in the toolchain file. Then we need to look out for rustup's behaviour changing during an upgrade (via upgrading nixpkgs). If we miss this, fresh machines won't install the required toolchain in init.sh but it's likely that the cargo and rustc wrappers will.

In both cases, we use the toolchain file's version.

I'd prefer to duplicate the versions for now: it looks like inconsistencies in explicitly mentioned Rust versions are easier to detect than changes to rustup's behaviour that only affect fresh environments.
Edit: We also need to duplicate components, the profile and targets - effectively replicating rust-toolchain.toml in the script. Let's either use the toolchain file for the stable version (& install via rustup show) or the init script. I'm leaning towards the toolchain file as rustup & its wrappers use it by default.

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