suggest: large suggestion to flake.nix#2136
Conversation
Mostly style changes and missing tools - format with alejandra for better readability - add missing cargo-nextest, rust-src component (for lsp), alejandra - use specialized pkg.mkShell instead of mkDerivation - remove hardeningDisable, not sure it's needed
This comment has been minimized.
This comment has been minimized.
| packages = [ | ||
| rust-toolchain | ||
| pkgs.rust-cbindgen | ||
| pkgs.cargo-nextest | ||
| pkgs.cmake | ||
| pkgs.autoconf | ||
| pkgs.automake | ||
| pkgs.libtool | ||
| pkgs.alejandra | ||
| ]; |
There was a problem hiding this comment.
Nit
| packages = [ | |
| rust-toolchain | |
| pkgs.rust-cbindgen | |
| pkgs.cargo-nextest | |
| pkgs.cmake | |
| pkgs.autoconf | |
| pkgs.automake | |
| pkgs.libtool | |
| pkgs.alejandra | |
| ]; | |
| packages = with pkgs; [ | |
| rust-toolchain | |
| rust-cbindgen | |
| cargo-nextest | |
| cmake | |
| autoconf | |
| automake | |
| libtool | |
| alejandra | |
| ]; |
There was a problem hiding this comment.
I just changed it a bit to separate rust-toolchain in a separate list in case it ever becomes a package in nixpkgs so that it doesn't get overriden.
| overlays = [(import rust-overlay)]; | ||
| }; | ||
| mkShell = rust-toolchain: | ||
| pkgs.mkShell { |
There was a problem hiding this comment.
I totally missed we didn't use mkShell in the previous PR, but just mkDerivation. This is the way to go indeed 👍
There was a problem hiding this comment.
yeah I use mkDerivation often to do things like use clang instead of gcc on Linux or select the llvm version.
| [toolchain] | ||
| channel = "1.87.0" | ||
| components = ["rustfmt", "clippy"] | ||
| components = ["clippy", "rust-src"] |
There was a problem hiding this comment.
Why this change? Doesn't this impact non Nix-users in some way?
There was a problem hiding this comment.
Because we never use the stable rustfmt anyway, also we need rust-src for rust-analyzer to allow go-to-definition to std functions.
I feel like the only impact it could've had is CI failing because it could not find rustfmt but as said above we don't use the stable rustfmt
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
| cmake | ||
| autoconf | ||
| automake | ||
| libtool |
There was a problem hiding this comment.
TBH I don't see how that's better than the previous code.
This reads like DRY for DRY sake.
There was a problem hiding this comment.
I agree, I also prefer the explicit pkgs., reverting this change
There was a problem hiding this comment.
with has it downsides but in this is one case that is simple and clear, and it's rather idiomatic in the Nix community. Though I don't really care and don't have a strong opinion either way 🤷
This reads like DRY for DRY sake.
Isn't this a good thing, as long as it's still readable, clear, and simple? For example we avoid std::collections::HashMap everywhere in a Rust file, because it's clutter, we do use std::collections::HashMap once and use HashMap elsewhere. I'm not sure how this is very different.
This reverts commit 0a490fd.
3 participants
A large suggestions for #2128
remove hardeningDisable, not sure it's needed