Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

@Ralith
Copy link
Contributor

commented Jan 19, 2018

This enables rustPlatform to build crates that have dependencies specified as git repositories. Previously, the cargo config file output of cargo 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.

Ralith added 2 commits Jan 19, 2018
rustPlatform: use `cargo vendor` config output
This fixes building packages with git (and potentially other non-crates.io) dependencies.
@Ralith

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2018

cc @Mic92

cp -ar vendor $out
mkdir -p "$out"
cargo vendor "$out/sources" | sed "s|$out|@out@|g" > "$out/config"

This comment has been minimized.

Copy link
@Mic92

Mic92 Jan 19, 2018

Contributor

How stable is the output of this tool?
If we upgrade cargo-vendor we might break checksums again, right?

This comment has been minimized.

Copy link
@bachp

bachp Jan 19, 2018

Contributor

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.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Jan 19, 2018

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.

This comment has been minimized.

Copy link
@Mic92

Mic92 Jan 19, 2018

Contributor

Can we replicate the output ourself instead? I don't want to updated all our checksum on every update of cargo-vendor

This comment has been minimized.

Copy link
@Ralith

Ralith Jan 20, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Jan 22, 2018

Sorry I'm not sure I understand the question :(

This comment has been minimized.

Copy link
@Ralith

Ralith Jan 22, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Jan 22, 2018

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?

This comment has been minimized.

Copy link
@Ralith

Ralith Jan 22, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Jan 23, 2018

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

This comment has been minimized.

Copy link
@Mic92

Mic92 Jan 19, 2018

Contributor

What does cargo-vendor now generate instead for git-dependencies?

This comment has been minimized.

Copy link
@zimbatm

zimbatm Jan 20, 2018

Member

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.

This comment has been minimized.

Copy link
@Ralith

Ralith Jan 20, 2018

Author Contributor

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?

@ghost

This comment has been minimized.

Copy link

commented Jan 19, 2018

@zimbatm

This comment has been minimized.

Copy link
Member

commented Jan 20, 2018

It would be easier to update cargo-vendor if it was just a normal package: #34082

@Ralith

This comment has been minimized.

Copy link
Contributor Author

commented Jan 20, 2018

Why does fetchcargo need to be fixed-output, anyway? Are we unwilling to trust that cargo vendor is well-behaved?

@bachp

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2018

@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 Cargo.lock includes checksums too, couldn't something similar be done too?

Excerpt form Cargo.lock:

...
[metadata]
"checksum advapi32-sys 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "e06588080cb19d0acb6739808aafa5f26bfb2ca015b2b6370028b44cf7cb8a9a"
"checksum ansi_term 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)" = "23ac7c30002a5accbf7e8987d0632fa6de155b7c3d39d0067317a391e00a2ef6"
...
@zimbatm

This comment has been minimized.

Copy link
Member

commented Jan 20, 2018

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.

@zimbatm

This comment has been minimized.

Copy link
Member

commented Jan 20, 2018

@Ralith yes that's the point of nix, not having to trust software to do the right thing.

@Mic92 Mic92 referenced this pull request Feb 21, 2018
4 of 8 tasks complete
@garbas

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2018

is this PR still relevant i think cargo-vendor got updated in #33980?

/cc @zimbatm

@Ralith

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2018

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.

@Mic92

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2018

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.
The same cargo-vendor output should be also provided in the error message.
Then we don't break existing checksums also, we currently have.

@Ralith

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2018

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.

@Mic92

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2018

@Ralith you can just grep for git = whatever cargo-vendor outputs. Even if the syntax changes we can simply upgrade the check without breaking anything.

@jbboehr

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2018

What about adding an option to buildRustPackage/fetchcargo to enable this behavior (or turn it off)?

@mmahut

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

What is the status of this pull request?

@zimbatm

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

pkgs/build-support/rust/fetchcargo.nix now keeps the output of cargo vendor which means that it should work with git dependencies in theory. If anyone wants to test that, I will close the PR in the mean time.

cargo vendor $out | cargo-vendor-normalise > $CARGO_CONFIG

@zimbatm zimbatm closed this Aug 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.