-
Notifications
You must be signed in to change notification settings - Fork 426
rust: fix toolchain creation #1983
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
Conversation
Remove the incorrect patching logic borrowed from the fenix project. We can use overrides to get the exact toolchain we need, already pre-patched.
- Remove the broken patching logic borrowed from the fenix project. - Remove private API usage. - Make toolchain a single derivation. The individual component overrides don't make sense in the context of dynamically-linked components.
121f010
to
eea9ef4
Compare
Claude finished @sandydoo's task —— View job Code Review Complete ✅Reviewed PR #1983 - Rust toolchain creation fixes. Here's my detailed analysis: Review Checklist:
Implementation AnalysisExcellent architectural improvement! The move from manual component combination to using rust-overlay's built-in profiles is a significant improvement that addresses the core issues. Areas for Improvement1. Error Handling & Validationsrc/modules/languages/rust.nix:204 - The baseToolchain = rustBin.${cfg.channel}.${cfg.version}; Recommendation: Add validation to ensure the channel/version combination exists: baseToolchain = rustBin.${cfg.channel}.${cfg.version} or (throw "Rust ${cfg.channel} ${cfg.version} not available in rust-overlay"); 2. Component Validationsrc/modules/languages/rust.nix:207-210 - No validation that requested components are available for the selected profile: combinedToolchain = baseToolchain.${cfg.profile}.override {
extensions = cfg.components;
targets = cfg.targets;
}; Recommendation: Add assertions to validate component availability, especially for custom component lists. 3. Documentation Inconsistenciesdocs/individual-docs/languages/rust.md:114 - The rust-toolchain.toml example shows accessing # This example might be confusing:
languages.rust = {
enable = true;
channel = "stable"; # This makes rustBin available
toolchain = config.languages.rust.rustBin.fromRustupToolchainFile ./rust-toolchain.toml;
}; Recommendation: Clarify that 4. Performance Considerationsrc/modules/languages/rust.nix:201 - The rustBin = rust-overlay.lib.mkRustBin { } pkgs.buildPackages; Recommendation: Consider if this could be memoized or cached, especially in multi-project scenarios. 5. Missing Assertionssrc/modules/languages/rust.nix:207 - No validation that the requested profile exists: combinedToolchain = baseToolchain.${cfg.profile}.override { Recommendation: Add assertion to validate profile availability for the selected toolchain. 6. Potential Breaking ChangesThe move from component-based to profile-based toolchains might break existing configurations that relied on the previous behavior. Recommendation: Consider adding a warning or migration guide for users upgrading from the previous implementation. Security Assessment✅ No security concerns identified. The implementation properly uses Nix's attribute access patterns and doesn't introduce any unsafe operations. Test Coverage ConsiderationsMissing test scenarios:
Recommendation: Add integration tests that verify:
Documentation QualityStrengths:
Improvement needed:
Overall AssessmentThis PR represents a significant architectural improvement that addresses the core issues with the previous implementation. The move to using rust-overlay's built-in profiles is the right approach and should resolve the broken toolchain issues. The main concerns are around error handling and validation - adding better assertions would make the user experience much more robust when misconfigurations occur. |
Deploying devenv with
|
Latest commit: |
79112ea
|
Status: | ✅ Deploy successful! |
Preview URL: | https://af47dc1c.devenv.pages.dev |
Branch Preview URL: | https://fix-rust-toolchain-setup.devenv.pages.dev |
In the switch from fenix to rust-overlay, the final toolchain was not correctly patched. This resulted in runtime linker errors, where tools like clippy were linked against the wrong librustc. PR #1983 was an attempt at adjusting our APIs to better suit the toolchain/profile design of rust-overlay. This PR retains the API compatbility, at the cost of using private APIs.
devenv v1.7.0 is released, but it includes a bug that broke the rust toolchain creation (cachix/devenv#1983), so it is not usable for rust development at the moment. This commits pin devenv to v1.6.1 in the meantime.
* chore: update Cargo.lock, upgrade cargo deps * fix: pin devenv to v1.6.1 to avoid v1.7.0 for now devenv v1.7.0 is released, but it includes a bug that broke the rust toolchain creation (cachix/devenv#1983), so it is not usable for rust development at the moment. This commits pin devenv to v1.6.1 in the meantime. * chore: update flake.lock * build: centralise workspace configuration and deps Centralise workspace configuration (package metadata, lint settings for rustc and clippy) and dependencies in the root `Cargo.toml` file, while making each sub-crates' `Cargo.toml` inherit relevant properties from it.
We decided to merge #2005, which preserves API compatibility at the cost of using private rust-overlay APIs. #2005 is more complicated that I would prefer to maintain. We should eventually either cut down our component API surface, as done here, or work with rust-overlay to expose the component/aggregation functions publicly. |
This is a follow-up to #1500 which ends up building broken
rust-overlay
toolchains.Fixes #1981.
Related to #1982.
Background
rust-overlay
's public API design doesn't really match the way we currently build thenixpkgs
toolchain.The
nixpkgs
toolchain is designed as an attrset of components. Each component is then added to the shell. In theory, this makes each component easily overridable, but in practice this is misleading.Cargo plugins are dynamically linked to cargo and to librustc. They come as a set. Our component architecture makes it seem like you can update components piecemeal, but we don't correctly patch them to make that reliable.
The
rust-overlay
approach is to build an attrset of components and pre-made profiles. The profiles take a set of components for a given rust release andsymlinkJoin
them, patching the binaries in the process. This aggregation function is private, but the profiles can be overridden to add extra components and targets.#1500 borrowed the
combine
function from fenix, but fenix does the majority of its patching elsewhere, which lead to issues like #1981.Solutions
The question we need to answer is whether we really want to maintain a toolchain builder ourselves.
If we want to maintain the component-ized architecture, then we have no choice but to use private APIs to generate our own profiles.
Or we could simplify things, drop the components, and focus on final toolchains:
nixpkgs
For the
nixpkgs
channel, deprecate the individual components and symlinkJoin a single toolchain. If you want more control over the toolchain, userust-overlay
.rust-overlay
If you want to override the individual components of the
rust-overlay
toolchain, then use its public functions and our docs to build your own.TODOs