tock / libtock-rs

Rust userland library for Tock
Apache License 2.0
160 stars 105 forks source link

shell.nix: Add nix shell #519

Open Samir-Rashid opened 8 months ago

Samir-Rashid commented 8 months ago

I have added a working nix shell based off the Tock file (https://github.com/tock/tock/pull/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 https://github.com/tock/libtock-rs/commit/0ae3974411877c64e383a316c43888cab776696a. Is it alright for me to submit this toolchain diff?

CC maintainers: @lschuermann @alevy @bradjc

lschuermann commented 8 months ago

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: https://github.com/tock/tock/pull/3727#issuecomment-1787095448

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

jrvanwhy commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 7 months ago

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.

lschuermann commented 7 months ago

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 commented 5 months ago

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.