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
Closed

Conversation

Ralith
Copy link
Contributor

@Ralith Ralith 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.

This fixes building packages with git (and potentially other non-crates.io) dependencies.
@Ralith
Copy link
Contributor Author

Ralith commented Jan 19, 2018

cc @Mic92


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

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?

Copy link
Member

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.

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.

Copy link
Member

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

Copy link
Contributor Author

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?

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 :(

Copy link
Contributor Author

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.

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?

Copy link
Contributor Author

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.

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
Copy link
Member

@Mic92 Mic92 Jan 19, 2018

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?

Copy link
Member

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.

Copy link
Contributor Author

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?

@ghost
Copy link

ghost commented Jan 19, 2018

cc @zimbatm
#31982 (comment)

@zimbatm
Copy link
Member

zimbatm commented Jan 20, 2018

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

@Ralith
Copy link
Contributor Author

Ralith 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
Copy link
Member

bachp 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
Copy link
Member

zimbatm 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
Copy link
Member

zimbatm commented Jan 20, 2018

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

@Mic92 Mic92 mentioned this pull request Feb 21, 2018
8 tasks
@garbas
Copy link
Member

garbas commented Mar 9, 2018

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

/cc @zimbatm

@Ralith
Copy link
Contributor Author

Ralith 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
Copy link
Member

Mic92 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
Copy link
Contributor Author

Ralith 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
Copy link
Member

Mic92 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
Copy link
Contributor

jbboehr commented Jun 6, 2018

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

@mmahut
Copy link
Member

mmahut commented Aug 3, 2019

What is the status of this pull request?

@zimbatm
Copy link
Member

zimbatm 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants