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

rust hook requires --path attribute #1112

Closed
zimbatm opened this issue Aug 8, 2019 · 4 comments · Fixed by #1113
Closed

rust hook requires --path attribute #1112

zimbatm opened this issue Aug 8, 2019 · 4 comments · Fixed by #1113

Comments

@zimbatm
Copy link
Contributor

zimbatm commented Aug 8, 2019

Cargo has changed how packages get installed and requires an extra --path <destination> attribute.

Symptom:

[INFO] Initializing environment for https://github.com/nix-community/nixpkgs-fmt.
[INFO] Installing environment for https://github.com/nix-community/nixpkgs-fmt.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: Command: ('/nix/store/fcc3x8zwq1c0667xjs7bkn6ay8j4fdpz-rust-1.38.0-nightly-2019-08-07-ad7c55e1f/bin/cargo', 'install', '--bins', '--root', '/home/zimbatm/.cache/pre-commit/repoeft6xm6t/rustenv-default')
Return code: 101
Expected return code: 0
Output: (none)
Errors: 
    error: Using `cargo install` to install the binaries for the package in current working directory is no longer supported, use `cargo install --path .` instead. Use `cargo build` if you want to simply build the package.

I guess the fix should be done where here:

'cargo', 'install', '--bins', '--root', directory, *package,

Do we want to make pre-commit compatible with multiple versions of cargo or just the latest one?

/cc @asottile @chriskuehl

@asottile
Copy link
Member

asottile commented Aug 8, 2019

heh that's annoying, yeah let's update to the latest install construct

@zimbatm
Copy link
Contributor Author

zimbatm commented Aug 8, 2019

I think the change would look like?

diff --git a/pre_commit/languages/rust.py b/pre_commit/languages/rust.py
index e09d007..c036739 100644
--- a/pre_commit/languages/rust.py
+++ b/pre_commit/languages/rust.py
@@ -84,7 +84,11 @@ def install_environment(prefix, version, additional_dependencies):
 
         for package in packages_to_install:
             cmd_output(
-                'cargo', 'install', '--bins', '--root', directory, *package,
+                'cargo', 'install',
+                '--bins',
+                '--root', directory,
+                '--path', directory,
+                *package,
                 cwd=prefix.prefix_dir
             )

This install the bin in the <directory>/bin folder

@asottile
Copy link
Member

asottile commented Aug 8, 2019

Seems right to me, there's pretty good tests for this so hopefully the test suite should agree 👍

bors bot added a commit to nix-community/nixpkgs-fmt that referenced this issue Aug 9, 2019
102: pre-commit r=zimbatm a=zimbatm

Add pre-commit to the project so we keep the syntax nicely formatted.

This is a bit of a warmup. Once pre-commit/pre-commit#1112 is fixed it will be possible to expose nixpkgs-fmt as a pre-commit hook as well!

Co-authored-by: zimbatm <zimbatm@zimbatm.com>
bors bot added a commit to nix-community/nixpkgs-fmt that referenced this issue Aug 9, 2019
102: pre-commit r=zimbatm a=zimbatm

Add pre-commit to the project so we keep the syntax nicely formatted.

This is a bit of a warmup. Once pre-commit/pre-commit#1112 is fixed it will be possible to expose nixpkgs-fmt as a pre-commit hook as well!

Co-authored-by: zimbatm <zimbatm@zimbatm.com>
@asottile
Copy link
Member

The puzzling thing to me is why I can't reproduce this with our testsuite, we have a tiny little rust package that should also trigger this but doesn't seem to 🤔 -- I did figure out a fix though I'll add it to your PR 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants