search

swellaby / rusty-hook

git hook manager, geared toward Rust projects
MIT License
205 stars 11 forks source link

error: Found argument '' which wasn't expected, or isn't valid in this context #168

Closed winston0410 closed 2 years ago

winston0410 commented 2 years ago

Environment Details

Description

I have a .rusty-hook.toml like this:

[hooks]
pre-commit = ["cargo fmt"]
# pre-commit = ["cargo fmt -- --check"]
pre-push = ["cargo check"]

[logging]
verbose = true

When I commit, I got the following error:

nix-shell ❮ git commit -m "Set up database"
error: Found argument '' which wasn't expected, or isn't valid in this context

USAGE:
    rusty-hook run --hook <HOOK> [-- <git args>...]

For more information try --help
Configured hook command failed
pre-commit hook rejected

But when I run rusty-hook hook, it works ok:

nix-shell ❯ rusty-hook run --hook pre-commit
[rusty-hook] Found configured hook: pre-commit
[rusty-hook] Running command: cargo fmt
winston0410 commented 2 years ago

To be precise I build rusty-hook from source, under the commit f4314083d69a20382c91aafcacec43c2a6b3e8bf

calebcartwright commented 2 years ago

hi thanks for reaching out. i've been meaning to add a note to the readme, but you really shouldn't be trying to install rusty-hook from source as there's some necessary version-managing machinery that's based on published versions on crates.io

Please try uninstalling from your local clone of the repo, and then installing using one of the documented methods in the readme.

The released versions don't support array syntax in the config file yet, so you'd want to convert your config file to something more like:

[hooks]
pre-commit = "cargo fmt"
# pre-commit = "cargo fmt -- --check"
pre-push = "cargo check"

[logging]
verbose = true

and if you need to execute multiple commands for a given hook, you can either chain them with && or invoke a user-defined shell script, e.g.


[hooks]
pre-push = "cargo check && cargo clippy"
winston0410 commented 2 years ago

@calebcartwright Thank you for your reply. I am using NixOS, therefore I cannot use the bin instead by cargo(it will simply not in $PATH), and I will have to build from source. It will be great if there is git tag in your project when you release a new update, as right now I can't find those so I just pick the latest commit to start with.

calebcartwright commented 2 years ago

@calebcartwright Thank you for your reply. I am using NixOS, therefore I cannot use the bin instead by cargo(it will simply not in $PATH), and I will have to build from source. It will be great if there is git tag in your project when you release a new update, as right now I can't find those so I just pick the latest commit to start with.

Please note that having cargo's bin directory on your path is the one and only mandatory prerequisite for being able to use rusty-hook (besides having the Rust of course :smile:), as detailed at the top of the Readme.

While we do have every intention of incorporating GitHub releases and tags at some point down the road, it's not a matter of the type of ref/which ref to use to build from source; it's really not viable to use rusty-hook by building from source. The rust/cargo ecosystem still does not support project-local bin dependencies, and this in turn adds some extra work for rusty-hook around version management which is what predicates the requirement on cargo and crates.io.

I'm not a Nix user myself but know others that are and who have gotten their path wired up correctly for cargo, so I feel like that should be a possibility for you.

winston0410 commented 2 years ago

Thank you for your reply again @calebcartwright. Even though Nix users can install package from Nixpkgs, all those packages on Nix are built from source(execpt some edge cases where they are create from AppImage or other things). But yes I do agree it will be some extra workload, not sure if something like np from NPM for easy publish and tagging exists for Cargo.

Aside from the building issue, is this error reproducible on your end? I am still getting this error by changing the value from array to string. It will be awesome if this doesn't appear in your dev version. I hope this issue is useful, given the limitation of my OS.

calebcartwright commented 2 years ago

But yes I do agree it will be some extra workload, not sure if something like np from NPM for easy publish and tagging exists for Cargo.

Sorry, but I'm not following you and I feel like you might not have read my last message thoroughly or perhaps I was not clear.

So again, I want to stress that you cannot use rusty-hook reliably if you don't have cargo's bin directory on your path and if you can't/won't install rusty-hook from crates.io. Full stop. If that's not something your interested in doing/able to do on your system, then I'd suggest you take a look at some alternatives (we include a few at the bottom of our own readme).

The nature of other languages/package management ecosystem and their tooling to facilitate the creation of git tags isn't particularly relevant here. It does not make a difference whether or not we have tags because the execution flow will not work with consistently with rusty-hook built-from-github/local-git-clones. There are multiple pieces to rusty-hook's execution, beyond any initial installation phase and, as I said previously, these other phases are strictly and exclusively coupled to cargo and crates.io by design.

This could all become much simpler if some RFCs move forward and cargo's able to support project-local binary dependencies, but unless and until it does, rusty-hook has to deal with a single global binary while simultaneously accounting for any unknown quantity of versions of local git hook scripts and config files (e.g. users can have rusty-hook v0.1.0 in project/directory foo and some other rusty-hook v23.8.1 in project/directory bar on the same machine). This is why rusty-hook has some strict versioning management it deals with internally, and that internal version management breaks quickly with git-based installs.

Even though Nix users can install package from Nixpkgs, all those packages on Nix are built from source(except some edge cases where they are create from AppImage or other things)

This is where you lost me as I'm only talking about rusty-hook. Regardless of how you get cargo/rust on your system (and yes, I'm aware there's several options for this in the Nix world), are you saying that it's impossible to use cargo for dependency management on Nix, and that you've got to manually build everything in your dependency tree by pulling from github? I freely admit to being relatively ignorant on Nix :smile: but my reading of the Nix Rust docs is that if you use the rustup installation method and get the shells wired up correctly then the development/cargo flows would be similar to other platforms.

Aside from the building issue, is this error reproducible on your end? I am still getting this error by changing the value from array to string. It will be awesome if this doesn't appear in your dev version.

No, the only way to introduce this is do exactly what you've done and try to build/install from source, and this is precisely one of the reasons why I've been saying you can't do this :wink:

Because you've broken the versioning (introduced a mismatch between local scripts and global bin by building from source), you'll most likely need to reset your local git repo. You can either manually clean out your local .git/hooks directory, or (assuming you've got our bin on your path) if running rusty-hook --version shows v0.11.2 or lower, then you should be able to run rusty-hook init in your local git repo (doesn't matter which directory, just somewhere within the repo) to reset things back to compatible versioning.

I hope this issue is useful, given the limitation of my OS.

I remain optimistic that it's perfectly viable to use rusty-hook on Nix provided proper rust configuration/setup. However, either way yes this should indeed prove to be a useful discussion, since it should help identify if there are certain rust setups on Nix where it's not possible, e.g. because cargo and the ability to install from crates.io are made unavailable, or if it is then it should help inform some documentation/guidance for future users.

calebcartwright commented 2 years ago

Guess it makes sense to close at this point. Please feel free to share any additional findings, questions you may have, etc. and thanks again for the report!