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

shell.nix: Add nix shell #519

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Samir-Rashid
Copy link
Contributor

I have added a working nix shell based off the Tock file (tock/tock#3727). I removed Tockloader, but did not bother pruning other deps that may not be in use.

Using the Tock nix shell does not work to develop this repo because it uses a stable Rust toolchain that gets messed up by the parsing that was there.

nix-shell --pure
error: unable to download 'https://static.rust-lang.org/dist//channel-rust-1.70.toml': HTTP error 404

       response body:

       <?xml version="1.0" encoding="UTF-8"?>
       <Error><Code>NoSuchKey</Code><Message>The specified key does not exist.</Message><Key>dist//channel-rust-1.70.toml</Key><RequestId>QPR91PNA50HR78P4</RequestId><HostId>EZo5fpLe2TlS54L4HQUmGcfqzV+BvFbKRcF+eQbHMUjtm83fhhP7er1+Pq+r2kObbqrto6TcduE=</HostId></Error>
(use '--show-trace' to show detailed location information)

I am able to run make tests properly now.

However, miri does not work on stable Rust toolchains. I am successfully able to run cargo miri test as it is written in the Makefile if I apply this diff.

diff --git a/rust-toolchain.toml b/rust-toolchain.toml
index c4f98b3..303e5cc 100644
--- a/rust-toolchain.toml
+++ b/rust-toolchain.toml
@@ -5,8 +5,8 @@
 # you'd like to use. When you do so, update this to the first Rust version that
 # includes that feature. Whenever this value is updated, the rust-version field
 # in Cargo.toml must be updated as well.
-channel = "1.70"
-components = ["clippy", "rustfmt"]
+channel = "nightly-2023-07-30"
+components = ["clippy", "rustfmt", "miri", "rust-src"]
 targets = ["thumbv6m-none-eabi",
            "thumbv7em-none-eabi",
            "riscv32imac-unknown-none-elf",

This is the same Rust nightly channel that Tock is using right now. I don't understand the justification for the dual toolchain from this commit 0ae3974.
Is it alright for me to submit this toolchain diff?

CC maintainers: @lschuermann @alevy @bradjc

@lschuermann
Copy link
Member

I don't understand the justification for the dual toolchain from this commit 0ae3974.

We want to use a dual-toolchain setup to work around the exact issue you've been encountering: libtock-rs should be able to compile on a stable Rust toolchain, which we want to verify through CI, but we further want to use tools such as Miri, which are not available on a stable toolchain (yet). You may think of this as a "release" and a "development" toolchain, respectively.

As a result, I believe it is fair to have the Nix expression use a nightly toolchain by default. We can introduce a parameter which switches toolchains. However, we should avoid changing the rust-toolchain.toml present in the repository. Perhaps we can maintain a separate rust-toolchain.dev.toml (name to be bikeshed), which is picked up by Nix and CI, respectively.

Apart from that, I have a similar set of concerns w.r.t. to using oxalica's overlay here: tock/tock#3727 (comment)

@jrvanwhy Can you confirm that my above statements about the parallel use of stable & nightly toolchains are indeed correct?

@jrvanwhy
Copy link
Collaborator

I don't understand the justification for the dual toolchain from this commit 0ae3974.

We want to use a dual-toolchain setup to work around the exact issue you've been encountering: libtock-rs should be able to compile on a stable Rust toolchain, which we want to verify through CI, but we further want to use tools such as Miri, which are not available on a stable toolchain (yet). You may think of this as a "release" and a "development" toolchain, respectively.

For futher background: libtock-rs is designed to be used with a variety of toolchains. libtock-rs defines a MSRV (Minimum Supported Rust Version), which is the oldest stable Rust toolchain that libtock-rs should work on. Applications can use any version of Rust that is at least as new as libtock-rs' MSRV.

When a contributor runs make test, tests are run using two toolchains:

  1. The MSRV toolchain, which is the oldest stable Rust toolchain mentioned above. Testing with this toolchain confirms that we did not accidentally depend on a newer Rust version than the MSRV. This toolchain is specified in /rust-toolchain.toml
  2. A nightly toolchain with Miri support. This toolchain is specified in /nightly/rust-toolchain.toml.

Thinking of them as "release" and "development" toolchains isn't accurate -- they're both there for development purposes.

As a result, I believe it is fair to have the Nix expression use a nightly toolchain by default. We can introduce a parameter which switches toolchains. However, we should avoid changing the rust-toolchain.toml present in the repository. Perhaps we can maintain a separate rust-toolchain.dev.toml (name to be bikeshed), which is picked up by Nix and CI, respectively.

I think the rust-toolchain.dev.toml you're referring to is the /nightly/rust-toolchain.toml toolchain I mentioned above.

Just to confirm: the purpose of shell.nix is to help Nix users contribute to libtock-rs, not to help them write applications using libtock-rs, correct?

@lschuermann
Copy link
Member

Just to confirm: the purpose of shell.nix is to help Nix users contribute to libtock-rs, not to help them write applications using libtock-rs, correct?

That's right. We could add another Nix expression for that purpose (canonically called default.nix), but given that the libtock-rs build infrastructure seems to be very much in flux still, I don't think now's the right time to open that can of worms. :)

@jrvanwhy
Copy link
Collaborator

I think we have three options here:

  1. Have Nix provide both needed toolchains, and modify libtock-rs' Makefile to call the appropriate toolchain for each command.
  2. Use rustup even on Nix (I'm not familiar with Nix, but I did a quick search and that appears to be a reasonable solution).
  3. Only provide one toolchain, preventing users from running a full make test. If we do this, we should probably provide the MSRV toolchain, rather than a nightly toolchain.

To elaborate on option 1: We could create a script (I'll call it cargo.sh) that invokes the correct toolchains for us. You'd call it like ./cargo.sh +msrv check --workspace. cargo.sh would detect if it's on Nix, perhaps using NO_RUSTUP, and if so it would invoke the correct toolchain using the correct behavior on Nix. On other systems, it would use rustup to run the correct toolchain.

One behavior the existing implementation has that I would like us to keep is that on systems that use rustup, it automatically installs new toolchains whenever necessary. This is a little tricky: rustup only automatically installs new toolchains when it reads the toolchain from a rust-toolchain or rust-toolchain.toml file; it won't install the toolchain if you specify the toolchain through a commandline argument or RUSTUP_TOOLCHAIN.

@Samir-Rashid
Copy link
Contributor Author

I updated to use the new Tock shell.nix (adds back tockloader, which I incorrectly removed, and uses fenix rust overlay).

Using rustup is antithetical to the determinism of using a nix environment. It also doesn't work right with paths in the nix store. I got Nix to provide both toolchains and hacked up a way to switch between them. You cannot access shell env variables in the Makefile, so I wrote in pseudocode in the Makefile. Running the commands manually works fine.

How does this approach look? jrvanwhy suggested using a bash script to switch toolchains, but I don't want to add another file for something this minor.

Makefile Outdated Show resolved Hide resolved
@lschuermann
Copy link
Member

I don't particularly like this, to be honest. It's weird to have both toolchains in the build inputs (which one ends up in your default path?), and the path rewriting through shell aliases, although clever, seems very brittle.

How about we just bite the bullet and always use Nightly in Nix environments? We could add a switch to the Nix expression's argument list that uses stable instead, where developers just have to know that this toolchains can't execute the nightly-dependent Miri tests. The CI & all other infrastructure for this repo doesn't use Nix anyways, nightly Rust should support a strict superset of the features of stable, and I'm not aware of anyone using the Nix expression to realize "production" builds of any Tock components with a stable toolchain. In fact, Tock itself doesn't compile on a stable toolchain (yet).

@Samir-Rashid Samir-Rashid marked this pull request as draft November 27, 2023 11:03
Specifies a Nix shell that creates a deterministic development
environment. This is based on the Tock Nix shell but is modified to
combine the required dependencies between the dual stable and nightly
toolchains. This `shell.nix` is meant for developing, so it uses the nightly
Rust version in order to be able to use Miri.

Co-authored-by: Anthony Tarbinian <atar137h@gmail.com>
@Samir-Rashid Samir-Rashid marked this pull request as ready for review January 12, 2024 21:56
@Samir-Rashid
Copy link
Contributor Author

The nightly rust-toolchain does not have all the dependencies needed to build the code. I changed the shell.nix to use the nightly Rust version and then add the required stable components and targets.

Comment on lines +44 to +49
# Use nightly development toolchain because Miri is not supported
# by the MSRV (Minimum Supported Rust Version) toolchain.
nightlyToolchain = nixpkgs.fenix.fromToolchainName {
name = nightly.channel;
sha256 = "sha256-R/ONZzJaWQr0pl5RoXFIbnxIE3m6oJWy/rr2W0wXQHQ=";
};
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate your efforts in making this work by combining the two toolchains, and while I think that this is a decent approach generally, to me this really points to a deficiency in how we pick nightly toolchain versions for libtock-rs. For the Tock kernel, we have a script that searches for a Nightly that has all of the required components (including Miri). I think we should rather choose a proper nightly this way, and avoid mix- and matching Rust components. I'm not even sure whether Miri is portable across rustc versions (e.g., in the features that it recognizes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the dual toolchain installation from before. Only the nightly derivation is generated, with the references on lines 41-42 being to the pure text of the file. This Nix logic has the (virtually) same effect as applying the diff in my original post before doing the toolchain derivation generation.
The problem you foresaw with some components being unavailable was not an issue. make test works, but I agree that incompatible nightlies are a risk in the future.

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 sorry, I'm having trouble matching what you write with the current Nix expression of this PR. Lines lines 51-57 still provide you a toolchain that is efffectively mix-and-match between stable and binary, and what you get is dependent on which components these two (stable + nightly) toolchains happen to provide, right?

And lines 58-65 then also throw in some components (namely, rust-std) taken from whatever the fenix overlay defines as the latest toolchain, which only happens to coincide with the pinned nightly toolchain by the correctness of the condition in lines 35? That seems very brittle.

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.

None yet

3 participants