-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
Update cargo builder to fetch registry dynamically. #27964
Conversation
pkgs/build-support/rust/default.nix
Outdated
|
||
patchRegistryDeps = ./patch-registry-deps; | ||
|
||
buildInputs = [ git rust.cargo rust.rustc ] ++ buildInputs; | ||
buildInputs = [ git rust.cargo rust.rustc strace ] ++ buildInputs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strace should be probably removed again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually use sysdig for those cases as I don't need to modify the environment.
The biggest benefit is that we no longer have to update the registry package. This means that just about any cargo package can be built by nix. No longer does `cargo update` need to be feared because it will update to packages newer then what is available in nixpkgs. This works very similarly to the old technique except instead of using cargo's internal cache and stripping out as much as possible it uses the new custom registry support to generate a minimal registry with exactly what we need. Like this existing method this will download crates multiple times for different packages. This is a shame because the Cargo.lock hash hashes inside of it but Nix doesn't really support this. This also uses the new --frozen and --locked flags which is nice. Currently cargo-local-registry only provides binaries for Linux and macOS 64-bit. This can be solved by building it for the other architectures and uploading it somewhere (like the NixOS cache). This also has the downside that it requires a change to everyone's deps hash. And if the old one is used because it was cached it will fail to build as it will attempt to use the old version.
let | ||
version = "0.1.4"; | ||
platform = | ||
if stdenv.system == "x86_64-linux" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will also need aarch64
. Maybe we can work-around getting cargo build in bootstrap phase without this package and build cargo-local-registry
from source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of dependencies and other stuff to manage. I don't think I know enough about the rust setup to do it myself but if someone can take it on that would be great. I was focused here on showing that cargo-local-registry is probably the way forward.
However building might not actually be necessary if we can rely on the cached deps pacakge. This package is archetecture independant so once we have them we can just build cargo-local-registry from source. I don't know if there is a policy about relying on the cache for things like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can use this one for bootstrapping: #24991
I have not found yet find to the time to inspect it closer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I prefer this approach is it doesn't require any preprocessing or extra files. It simply reads the Cargo.toml file. I do think that those are fixable but this is also a nice quick improvement on the current state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But a solution for aarch64 must be found first. If upstream adds support for this architecture as well, I am ok with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we could just build our own boostrap binary 🤷. Once the bootstrap is in place we can just grab a couple of versions back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mic92 As far as i can tell there is currently no support for rust bootstrap for aarch64 either? There are aarch64 builds available on the official rust installer page. Adding a binaries for aarch64 to upstream cargo-local-registry
would be easy tough.
@kevincox I was working on something similar but based on cargo-vendor. Not sure yet which one is better suited but according to the authore (which is the same for both) cargo-vendor
is getting more attention at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevincox I forget to mention that cargo-vendor
might be easier to bootstrap soon as it will soon have a tarball with vendored dependencies available. See: alexcrichton/cargo-vendor#51
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm not an expert on the differences but it seems like cargo-local-registry is designed to "mirror" the required dependencies whereas cargo-vendor is supposed to the upstream project to vendor dependencies themselves. Either will probably work in practice but cargo-local-registry looks like a better fit for our use case.
And it seems like the bootstrapping issue shouldn't be a major concern as cargo can cross-compile easily. Someone just needs to but in some effort to bootstrap the first version.
That being said I'm happy for whatever solution works.
Might solve #23282 |
@ckauhaus It would, added to the description. |
In its current form, this PR breaks all existing Rust packages in an unobvious way. Due to the transition from registry.index to local-registry, the required format of the deps derivation has changed. This means we need new depsSha256 checksums everywhere. I'd personally prefer to rename this attribute (e.g., cargoDepsSha256) so that old Rust builds print a clear error message. |
Merged this into #30088 |
Motivation for this change
See commit message
The biggest benifit is that we no longer have to update the registry
package. This means that just about any cargo package can be built by
nix. No longer does
cargo update
need to be feared because it willupdate to packages newer then what is available in nixpkgs.
Things done
Please check what applies. Note that these are not hard requirements but merely serve as information for reviewers.
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)Fixes #14469
Fixes #23282