cachix / devenv

Fast, Declarative, Reproducible, and Composable Developer Environments
https://devenv.sh
Apache License 2.0
3.56k stars 259 forks source link

Custom rust toolchains #667

Closed cdmistman closed 10 months ago

cdmistman commented 11 months ago

Right now using custom fenix toolchains (ie, for use with rust-toolchain.toml) requires duplicating a let-bound tools list, which is inconvenient. Further, the rustup components that are installed is fixed, so eg miri can't be installed without adding it to top-level packages.

This change makes it easier to support both use-cases:

  1. components is now an option and is used in place of the let-bound tools
  2. languages.rust.packages has been renamed to languages.rust.toolchain to be more consistent with rustup's terminology, and now has a freeformType to allow usage of components that aren't in nixpkgs
  3. version has been renamed to channel and is a bit more consistent with rustup's concept of a channel
  4. the rust-src component is now properly overridden when defined in components

Here's a few examples (using fenix as an overlay):

# use a custom toolchain
languages.rust.toolchain = pkgs.fenix.latest;

# use a standard fenix toolchain
languages.rust.channel = "nightly";

# override a single component
languages.rust.toolchain.rustfmt = pkgs.fenix.latest.rustfmt;

# only use `rustc` and `cargo`
languages.rust.components = [ "rustc" "cargo" ];
cdmistman commented 11 months ago

i also considered defining toolchain instead, which would be assigned to fenix's toolchain type or a channel. This would allow for a very similar api (although would require nix-community/fenix#108):

languages.rust.toolchain = pkgs.fenix.stable;

languages.rust.toolchain = pkgs.fenix.fromManifestFile inputs.rustup-manifest;

# depends on https://github.com/nix-community/fenix/pull/108
languages.rust.toolchain = pkgs.fenix.fromToolchainName (importTOML ./rust-toolchain.toml).toolchain.channel;

let me know if a different API is desired and I can implement that instead :) Either way, making it easier to use custom toolchains would be awesome

domenkozar commented 11 months ago

Have you seen https://github.com/cachix/devenv/pull/640

cdmistman commented 11 months ago

i haven't! i'll note that the current implementation of that PR recreates a rust-toolchain.toml file from the rust options, which means

  1. repos already using a rust-toolchain.toml file must recreate the toolchain spec in the devenv config
  2. the recreated toolchain then results in an Import From Derivation

My PR addresses both; you may use a pre-existing rust-toolchain.toml file directly or avoid an IFD by adding the manifest file to the flake inputs.

If @nothingnesses would accept contributions to their PR I'd be glad to merge our work :)

cdmistman commented 11 months ago

additionally, that PR cuts out rust components from nixpkgs - if I'd known this was fine I would've done a couple things differently 😅 namely, rust.toolchain could be a fenix toolchain type

nothingnesses commented 11 months ago

Hi. I'd be happy to work together on this issue, but am not sure if there's anything much I can contribute further. I already like your solution better than mine and agree that recreating a "rust-toolchain.toml" file is undesirable and would prefer a way to just use an existing one (I couldn't figure out a way to do it without having to make a TOML parser). I think your solution would even allow using a different overlay, right?

One thing I think I'd change is the default components. I think it'd be better just to stick with the ones included in the default rustup profile, which I've done in my PR. To handle the pre-commit hooks, I just set the components to be the ones from nixpkgs, if they weren't already included in the toolchain options/part of the selected profile; I think you should be able to do something similar to that with you solution.

cdmistman commented 11 months ago

I couldn't figure out a way to do it without having to make a TOML parser

nixpkgs.lib.importTOML works and there's also builtins.fromTOML :)

I think your solution would even allow using a different overlay, right?

ah, i guess that's true! didn't think about that - yeah, https://github.com/oxalica/rust-overlay should work with this design as well

I think it'd be better just to stick with the ones included in the default rustup profile, which I've done in my PR

Hm, yeah that should work. I guess I'd like to know if a goal of devenv is to handle language server installation? @domenkozar if you can weigh in here that'd be appreciated. Additionally, I'd like to know if you have any input on the trade-off between only accepting fenix's toolchain type (which would result in a much-friendlier API here) or if I should keep the API flexible with the single options.package as defined in my PR

To handle the pre-commit hooks, I just set the components to be the ones from nixpkgs, if they weren't already included in the toolchain options/part of the selected profile; I think you should be able to do something similar to that with you solution.

yes I could, but I think it's better to use the binary from the package - this has 2 benefits:

  1. the components aren't built 2x (in the event of no substituters)
  2. if the user is using nightly features and is using the nightly toolchain, those will work OOTB with pre-commit-hooks.

I guess I forgot to change that part in this PR, I'll use the bin directory of the built package derivation if the bin exists there - if it doesn't, it'll be null

cdmistman commented 11 months ago

Additionally, I'd like to know if you have any input on the trade-off between only accepting fenix's toolchain type (which would result in a much-friendlier API here) or if I should keep the API flexible with the single options.package as defined in my PR

it seems gleam and ruby both have hard-dependencies on external inputs, but nonetheless I think I made a flexible API, defining both options.package and options.toolchain.

domenkozar commented 10 months ago

Is it ready for review?

cdmistman commented 10 months ago

not quite yet - i tried to use this in a project but didn't have time to dig into failures. i'll have time to take care of this in a few days - i'll ping back here when it works and it's ready for review.

cdmistman commented 10 months ago

but i'll note this is one of the more complex nix modules i've written, any input on its current state/design would be appreciated :)

domenkozar commented 10 months ago

Maybe we should add that as a comment?

On Sun, Jul 2, 2023, 21:11 Simon Žlender @.***> wrote:

@.**** commented on this pull request.

In src/modules/languages/rust.nix https://github.com/cachix/devenv/pull/667#discussion_r1249744925:

  • env.RUST_SRC_PATH = cfg.packages.rust-src;

RUST_SRC_PATH is necessary when rust-src is not at the same location as as rustc. This is the case with the rust toolchain from nixpkgs. You'll run into this issue when using rust-analyzer in vscode.

— Reply to this email directly, view it on GitHub https://github.com/cachix/devenv/pull/667#discussion_r1249744925, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA63A5EF5Y7O3VRZL2WMT3XOHIYDANCNFSM6AAAAAAZI3XGJ4 . You are receiving this because you were mentioned.Message ID: @.***>

cdmistman commented 10 months ago

This is the case with the rust toolchain from nixpkgs. You'll run into this issue when using rust-analyzer in vscode.

I'm really tempted to just implement using fenix. Thoughts? Basically:

cdmistman commented 10 months ago

@domenkozar it's ready for review! happy to answer any questions or address any feedback

when i tried to remove the reliance on nixpkgs, nix flake check failed due to the missing fenix input. this is fine though - everything I've tested still works.

I probably haven't tested all the edge cases, but I've tested with the following configurations:

in a new cargo project, i tested for the following:

szlend commented 10 months ago

Tbh I find the API as it is right now a little confusing. There's package, toolchain, components, version and it's not clear how they interact with each other - if at all. And then there's rust-src which is technically a component, but is in the root of config options. I don't really have a well thought out proposal right now but ideally it would be nice if the incompatible options could exclude each other somehow. Maybe it would be possible to have something like:

toolchain = pkgs.fenix.stable.toolchain;
toolchain = pkgs.fenix.nightly.withComponents [...];
toolchain = pkgs.fenix.fromToolchainFile ./rust-toolchain.toml;
toolchain = pkgs.fenix.fromManifestFile inputs.rustup-manifest;
toolchain = { version = "stable"; };
toolchain = { version = "stable"; components = [...] };

I think it may be possible with coerceTo from attrset to package, but I'd need to think about it some more.

Edit: At the very least, we probably don't need both package and toolchain, no?

szlend commented 10 months ago

Better yet:

# just set the complete toolchain package
package = pkgs.fenix.stable.toolchain;
package = pkgs.fenix.nightly.withComponents [...];
package = pkgs.fenix.fromToolchainFile ./rust-toolchain.toml;
package = pkgs.fenix.fromManifestFile inputs.rustup-manifest;

# or have devenv generate `package` for you from `version` and `components`.
version = "stable";
components = [...];
cdmistman commented 10 months ago

hm, maybe i can get rid of package by making nixpkgs quack like fenix. one sec...

szlend commented 10 months ago

What do we really gain from having the toolchain option anyway? I read through the comments and I still don't understand since all we really do is call withComponents on it, so it should be equivalent to version + components.

Btw I just remembered, one thing this change makes more difficult is taking the stable toolchain and replacing rustfmt with the nightly version. That's something a lot of projects do since rustfmt lags a lot at formatting newly stabilized features.

cdmistman commented 10 months ago

What do we really gain from having the toolchain option anyway? I read through the comments and I still don't understand since all we really do is call withComponents on it, so it should be equivalent to version + components.

in my mind, the fact that you can easily assign a fenix toolchain to an attribute is helpful

one thing this change makes more difficult is taking the stable toolchain and replacing rustfmt with the nightly version. That's something a lot of projects do since rustfmt lags a lot at formatting newly stabilized features.

oooh yeah, i primarily use nightly toolchain anyways so haven't thought about that use-case.


ok, things obviously have to change some more. to flesh this out, it's generally desired:

  1. more control over where components are coming from
  2. more control over which components are enabled
  3. easily specify which channel of the toolchain to use

what's currently implemented in main does 3, partially does 1, but does not support 2 at all. it'd be ideal if there was plug-n-play support for entire toolchain swapping as well - if a user wants more control over the toolchain than just "stable" or "nightly", they should be able to specify that easily. as such, here's the design i'm proposing - @szlend lmk if you have any more input:

# duh
enable = true;

# each component has 2 options. `enable` is self-explanatory...
components.cargo.enable = true;
# overrides the location of the component that would otherwise be set by `toolchain`
components.cargo.package = pkgs.cargo;

# the rustup channel "stable", "beta", or "nightly". sets the default value for `toolchain`
channel = "stable";

# attrset of component names to derivations
toolchain = pkgs.fenix.stable;
cdmistman commented 10 months ago

i guess the above can be simplified even more using //

# no need for `components` since you can override component locations with `//`
# however, the easiest use-case is just using the same channel for all components, so there's a quick override for that
channel = "stable";

# components is the default list of components to grab from `toolchain`
components = [ "cargo" "rustc" ];

# toolchain is the attrset pointing to the location of each component
# if you only need nightly rustfmt for example, you can do this:
toolchain = pkgs.fenix.stable // { inherit (pkgs.fenix.latest) rustfmt; };
cdmistman commented 10 months ago

nice: as of the latest commit, rust-std works without modification for rust-overlay and nixpkgs. if the user sets toolchain = <some fenix toolchain>, then there's an error due to the directory layout of fenix's rust-std output. i think rust-overlay is the right move here.

cdmistman commented 10 months ago

nevermind, i didn't check my work enough and it seems that the toolchain derived from nixpkgs is being used even when channel is set... my nix knowledge is officially failing me, if somebody knows how to fix this I'd appreciate it, otherwise I'm not sure what to do now...

cdmistman commented 10 months ago

ah, i see - rust-overlay only provides the toolchain attrsets when actually used as an overlay. switched back to fenix, again ready for review! hopefully this much simpler api is less confusing :)

szlend commented 10 months ago
channel = "stable";

# components is the default list of components to grab from `toolchain`
components = [ "cargo" "rustc" ];

# toolchain is the attrset pointing to the location of each component
# if you only need nightly rustfmt for example, you can do this:
toolchain = pkgs.fenix.stable // { inherit (pkgs.fenix.latest) rustfmt; };

I like it! Though it would be nice if you could just do this:

channel = "stable";
components = [ "cargo" "rustc" "rustfmt" ]; 
# toolchain is generated from channel and components, howerver...
toolchain.rustfmt = fenix.nightly.rustfmt; # ... we override rustfmt here

To make this work you'd only need to toolchain.<component> = mkDefault toolchainFromChannel.<component> in your module.


The way I see it, you have the following use cases:

  1. I just want the default stable toolchain:
    channel = "stable";
  2. I want the stable toolchain with specific components:
    channel = "stable";
    components = [ "cargo" "rustc" ]; 
  3. I want the stable toolchain but override a component:
    channel = "stable";
    toolchain.rustfmt = fenix.nightly.rustfmt; 
  4. I want full control over the fenix toolchain:
    toolchain = fenix.stable.toolchain;
    toolchain = fenix.nightly.withComponents config.languages.rust.components;
    toolchain = fenix.fromToolchainFile ./rust-toolchain.toml;
    toolchain = fenix.fromManifestFile inputs.rustup-manifest;
    # etc...
  5. I want to add another component without losing defaults:
    components = lib.mkAfter [ "rust-mingw" ];
szlend commented 10 months ago

@cdmistman It'd be nice if there was a way to add target triples as well. I originally opened my PR as I wanted a way to add and compile a project to the "wasm32-unknown-unknown" triple. Is it possible to do that with your solution?

I think you could already do this like so:

toolchain.wasm-rust-std = fenix.targets.wasm32-unknown-unknown.latest.rust-std;

Edit: Actually no, I misunderstood.

I guess we could add something like targets = [ "wasm32-unknown-unknown" ].

cdmistman commented 10 months ago

I guess we could add something like targets = [ "wasm32-unknown-unknown" ].

hm, i'm not sure i know how to do additional targets. the fenix docs say to combine with the <target>.rust-std but:

$ nix repl
nix-repl> :lf fenix
Added 14 variables.

nix-repl> :b outputs.packages.aarch64-darwin.targets.wasm32-wasi.stable.rust-src

This derivation produced the following outputs:
  out -> /nix/store/q8856rcs522pbzjxswa0cgk4kwpfbd5d-rust-src-stable-2023-06-01

nix-repl> :b outputs.packages.aarch64-darwin.stable.rust-src

This derivation produced the following outputs:
  out -> /nix/store/q8856rcs522pbzjxswa0cgk4kwpfbd5d-rust-src-stable-2023-06-01

i guess the only changes necessary to get this working are to get the nixpkgs for the target system and set CARGO_TARGET_<triple>_LINKER env var? i can dig in later but if somebody more knowledgeable than me can provide additional input that'd be appreciated.

szlend commented 10 months ago

Try with rust-std, not rust-src.

Either way though I feel like supporting cross-compilation is a bit much for this PR. I don't have experience with WASM, so maybe that's simpler. But getting other targets to work is far move involved than just installing the target rust-std. For example if you want to target x86_64-linux from aarch64-darwin, you'll need to build a cross-compiling stdenv from source, as nixpkgs doesn't have it in its binary cache.

So if someone with more experience can chip in to make a limited set of targets working, that would be great. But I don't think it should be a blocker.

cdmistman commented 10 months ago

yeah, i'll leave extra targets as a non-blocker for this. if the user wants to accomplish this, they might be able to do so by either combineing the rust-std components in the toolchain configuration or adding it to packages, and then setting the appropriate linker env var

adding a targets option would be cool, but would perhaps be more involved since you'd have to map rust triples to nix systems and packages, as well as (possibly) figuring out how to get the cross-system toolchain available without re-evaluating all of nixpkgs.

i think this use-case is perhaps better solved by using devenv in a nix flake instead of as a standalone tool to only have to evaluate nixpkgs once

martinjlowm commented 10 months ago

Try with rust-std, not rust-src.

Either way though I feel like supporting cross-compilation is a bit much for this PR. I don't have experience with WASM, so maybe that's simpler. But getting other targets to work is far move involved than just installing the target rust-std. For example if you want to target x86_64-linux from aarch64-darwin, you'll need to build a cross-compiling stdenv from source, as nixpkgs doesn't have it in its binary cache.

So if someone with more experience can chip in to make a limited set of targets working, that would be great. But I don't think it should be a blocker.

I was actually hoping for this. :'( We're cross compiling against aarch64-unknown-linux-gnu on x86_64-unknown-linux-gnu in CI but for the system platform on the developer's machine. I'm not too familiar with Nix, but perhaps there's a better way?

cdmistman commented 10 months ago

We're cross compiling against aarch64-unknown-linux-gnu on x86_64-unknown-linux-gnu in CI but for the system platform on the developer's machine

sounds like you want to make it an output in a flake. look at fenix docs for an example of how to accomplish this.

alternatively, you can try my suggestions above

szlend commented 10 months ago

Like @cdmistman said, something like this might work, but I can't test cause I'd need to bootstrap gcc from source:

packages = [ <fenix>.targets.aarch64-unknown-linux-gnu.stable.rust-std ];
env.CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_LINKER =
  let
    inherit (pkgs.pkgsCross.aarch64-multiplatform.stdenv) cc;
  in
  "${cc}/bin/${cc.targetPrefix}cc";

Though this will break as soon as you have to link with any C dependencies.

szlend commented 10 months ago

LGTM, great job on this!

A bit suprised something like components = lib.mkAfter [ "rls" ]; doesn't work. I assumed it would merge with mkOptionDefault, but I guess not. Though I guess the non-default components are kind of niche anyway so it's not really a big issue.

cdmistman commented 10 months ago

I assumed it would merge with mkOptionDefault, but I guess not

yeah, I had thought the same as well - quite disappointing that components isn't easily extensible :( if someone else has an idea of how to do this it'd be greatly appreciated, but that's not a rabbit hole I'm wanting to go down 😅

domenkozar commented 10 months ago

Could you also change examples/rust to instruct how to use it now?

cdmistman commented 10 months ago

i didn't update the documentation - afaict, the docs get auto-updated but i couldn't rebuild them on my machine (aarch64-darwin, can't find libcairo). it seems GHA might rebuild them anyways

cdmistman commented 10 months ago

i have to update the PR description and want to do a little more testing. I'm about to get on a flight but I'll be able to do all of that late tonight/tomorrow morning PST (depending on how i'm feeling tonight)

cdmistman commented 10 months ago

updated the PR description and hopefully fixed failing CI (for rust - not going to fix python-poetry as it's out of scope) :)

if CI passes/no more feedback should be good to merge!

domenkozar commented 10 months ago

Could you also add rename hints for all the changed options?

lib.mkRenamedOptionModule [ "languages" "rust" "version" ] [ "languages" "rust" "channel" ])
domenkozar commented 10 months ago

Great work, thank you for the patience with reviews :)

domenkozar commented 10 months ago

I wonder if it also fixes https://github.com/cachix/devenv/issues/662 by any chance?

jhartma commented 9 months ago

Just leaving this here for anybody who wants to get devenv working with rust wasm.

in devenv.yaml

inputs:
  nixpkgs:
    url: github:NixOS/nixpkgs/nixpkgs-unstable
  fenix:
    url: github:nix-community/fenix
    inputs:
      nixpkgs:
        follows: nixpkgs
  rust-overlay:
    url: github:oxalica/rust-overlay
    overlays:
      - default

and in devenv.nix add:

  ...
  let 
    rustVersion = pkgs.rust-bin.selectLatestNightlyWith (toolchain: toolchain.default);
  in
  ...
  languages = {
    rust = {
      enable = true;
      toolchain.rustc = (rustVersion.override {
        extensions = [ "rust-src" ];
        targets = [ "wasm32-unknown-unknown" ];
      });
    };
  };
  ...
martinjlowm commented 9 months ago

Or for anyone using Flakes, Fenix and a toolchain file:

{
  inputs = {
    nixpkgs.url = "github:NixOS/nixpkgs/nixos-unstable";
    systems.url = "github:nix-systems/default";
    fenix.url = "github:nix-community/fenix";
  };

  outputs = { self, nixpkgs, devenv, systems, fenix, ... } @ inputs:

    let
      system = builtins.currentSystem;
      pkgs = nixpkgs.legacyPackages.${system};
      fenixpkgs = fenix.packages.${system};
      aarch64-binutils = pkgs.pkgsCross.aarch64-multiplatform.stdenv.cc;
      x86_64-binutils = pkgs.pkgsCross.gnu64.stdenv.cc;
    in
      {
        devShell.${system} = devenv.lib.mkShell {
          inherit inputs pkgs;
          modules = [
            {
              packages = [
                pkgs.libunwind
                aarch64-binutils
              ] ++ pkgs.lib.optionals pkgs.stdenv.isDarwin (with pkgs.darwin.apple_sdk; [
                frameworks.Security
                frameworks.CoreFoundation
                x86_64-binutils
              ]);

              languages.rust = {
                enable = true;
                toolchain.rustc = fenixpkgs.fromToolchainFile { dir = ./.; };
              };
            }
          ];
        };
      };
}

rust-toolchain.toml:

[toolchain]
channel = "1.70.0"
targets = ["x86_64-unknown-linux-gnu", "aarch64-unknown-linux-gnu"]
domenkozar commented 9 months ago

Maybe we could allow setting languages.rust.targets and wire everything up?

szlend commented 9 months ago

Maybe we could allow setting languages.rust.targets and wire everything up?

We could, but most targets wont work without a cross stdenv so that's a lot to wire up correctly and most of these toolchains need to be bootstrapped from source which can be hours of compiling, depending on your system. Though you can get pretty far with just cargo zigbuild: https://github.com/rust-cross/cargo-zigbuild

domenkozar commented 9 months ago

That's exactly why we should turn it into a flag so that it's not needed to go though that pain.