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
[WIP] Rust platform improvements #34034
Conversation
This fixes building packages with git (and potentially other non-crates.io) dependencies.
cc @Mic92 |
|
||
cp -ar vendor $out | ||
mkdir -p "$out" | ||
cargo vendor "$out/sources" | sed "s|$out|@out@|g" > "$out/config" |
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.
How stable is the output of this tool?
If we upgrade cargo-vendor
we might break checksums again, right?
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 should probably ask @alexcrichton if we can agree on some kind of stable interface here.
Another possibility is to not include the content of config in the checksum but only the result of the final fetch. Not sure how this could be achived tough.
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 exact output to stdout probably isn't the most stable across versions of cargo-vendor
but the output to the vendor
directory should be deterministic.
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.
Can we replicate the output ourself instead? I don't want to updated all our checksum on every update of cargo-vendor
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'm not sure how we'd do that without fragile Cargo.toml parsing. Perhaps @alexcrichton could comment on the feasibility of modifying cargo to make a static configuration viable?
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.
Sorry I'm not sure I understand the question :(
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 difficulty we're having here is that the cargo config varies according to the contents of the input Cargo.toml
, and also apparently isn't stable enough to be relied upon, which means Nix has no reliable way to come up with the correct config file. If cargo could instead pick up git dependencies with magic similar to that which it uses for crates.io dependencies, the config file nix supplies could be static (as it is without this PR), and rust packages with git dependencies could be much more easily packaged.
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.
Hm I'm not sure I understand, isn't that expected? If a crate depends on git repositories we need to tell cargo where to find them instead, right?
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 don't have a particularly deep understanding of how cargo handles dependency resolution in general, but it seems strange that github dependencies must be explicitly enumerated (in an unstable fashion!) whereas crates.io ones can be auto-discovered.
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.
Cargo currently understands "sources" of packages and cargo-vendor emits a source-replacement configuration for each source necessary. All of crates.io is one source, while each git repository is another source.
|
||
[source.vendored-sources] | ||
directory = '$(pwd)/$cargoDepsCopy' | ||
EOF |
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.
What does cargo-vendor now generate instead for git-dependencies?
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 current stdout output for cargo-vendor 0.13 on alacritty is:
To use vendored sources, add this to your .cargo/config for this project:
[source.crates-io]
replace-with = "vendored-sources"
[source."https://github.com/jwilm/libfontconfig"]
git = "https://github.com/jwilm/libfontconfig"
branch = "updated-2017-10-8"
replace-with = "vendored-sources"
[source."https://github.com/jwilm/rust-fontconfig"]
git = "https://github.com/jwilm/rust-fontconfig"
branch = "updated-2017-10-8"
replace-with = "vendored-sources"
[source.vendored-sources]
directory = "/home/zimbatm/go/src/github.com/jwilm/alacritty/vendor"
I remember submitting a PR to send the comment on stderr and remove the indent as well, not sure why it was reverted.
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 remember submitting a PR to send the comment on stderr and remove the indent as well, not sure why it was reverted.
It wasn't; this PR relies on that. Perhaps the indentation is on stderr too?
It would be easier to update cargo-vendor if it was just a normal package: #34082 |
Why does |
@zimbatm I just watched the recording of your talk at NixCon 2017. You mentioned there what you did for Yarn and the lock file which includes checksums. As Excerpt form Cargo.lock:
|
Ideally yes @bachp but it's more difficult to implement because it requires more coordination between nix and cargo. Nix has to download the files and place them where cargo expects them. |
@Ralith yes that's the point of nix, not having to trust software to do the right thing. |
This PR is not merely a cargo-vendor update, and remains relevant so long as Nix's cargo-vendor use fails in the presence of git dependencies. |
A simple workaround to avoid future incompatibility would be to detect if the project uses git dependencies and asks the user to provide the cargo-vendor output instead. |
I'm not sure how to gracefully detect that, but it sounds like an adequate compromise. Hopefully in the future carnix will be stable/robust enough that we can just ditch cargo-vendor wholesale. |
@Ralith you can just grep for |
What about adding an option to buildRustPackage/fetchcargo to enable this behavior (or turn it off)? |
What is the status of this pull request? |
|
This enables
rustPlatform
to build crates that have dependencies specified as git repositories. Previously, the cargo config file output ofcargo vendor
was being discarded in favor of a hardcoded config, which was missing the extra sections necessary for non-crates.io dependencies to work.Unfortunately, this breaks every single
cargoSha256
in nixpkgs, which will be a huge pain to fix. Better ideas very welcome.