Deploy precommit to handle rustfmt & cargo.lock

[sylvestre]

I stopped counting the number of time I asked people to run rustfmt on their code or to run cargo +1.40.0 update

We should have an hook to do that kind of work for us on the client side: https://pre-commit.com/#rust

[LevitatingBusinessMan]

@sylvestre Would using the pre-commit tool you are suggesting require users to install a third party tool just to make a commit? If the user does not have it installed, would they be notified of it or would we still end up in the same situation where have to tell people to run certain commands before committing? Git itself has functionality for pre-commit hooks (and other kind of hooks).

[LevitatingBusinessMan]

@sylvestre

#!/bin/sh

CARGO_VER=1.40.0

STAGED=$(git diff --name-only --cached | grep '.*\.rs')

if ! [ "$STAGED" = '' ]; then
    rustfmt --check "$STAGED" || { 
        echo -e "\e[31mPlease run rusftmt on all staged files before committing\e[0m" ; exit 1
    }
fi

echo -e "Running \e[34mcargo +$CARGO_VER update\e[0m ..."
cargo +$CARGO_VER update || {
    echo -e "\e[31mFailed to run cargo update (do you have the $CARGO_VER toolchain installed?)\e[0m" ; exit 1
}

There is also a more major issue, there's no way to ship this with the repo in a way that it's enabled by default. You'll have to force the user to run a script to enable to githook.

Here is a branch with this git-hook added. https://github.com/LevitatingBusinessMan/coreutils/commit/1611296537342fbb2da09550c1b3b42c442c092f Though I feel like it really doesn't solve the issue.

[sylvestre]

@LevitatingBusinessMan A change landed here: https://github.com/uutils/coreutils/pull/1959

I agree this isn't perfect and people will have to use it to benefit from it. Baby steps :)

[LevitatingBusinessMan]

@LevitatingBusinessMan A change landed here:

1959

I have followed the progress of #1959, I still think it's not a good idea to force contributors to install a third-party tool to do something that is native to git. pre-commit's purpose is to be able to use the same git hooks across multiple repositories. Aside from that pre-commit only makes it harder to make a git-hook with custom behavior, you can't do any kind of scripting with it.

If you don't care that you can't have a git-hook enabled by default, then please take a closer look at my solution. https://github.com/LevitatingBusinessMan/coreutils/commit/1611296537342fbb2da09550c1b3b42c442c092f

[offbyone]

pre-commit isn't enforced unless the developer has it installed in their local checkout. That's a good compromise for the tool, imo.

Also, this project implicitly requires the developer to have cargo, rustup, etc, installed, so ... it's not really a stretch to ask for one more.

[offbyone]

pre-commit offers a consistent interface to the git hooks for commits, and bundles up some versions of hooks that are shared easily. I've used it for a lot of projects, and when the hooks are well set up they are not onerous.

[LevitatingBusinessMan]

Also, this project implicitly requires the developer to have cargo, rustup, etc, installed, so ... it's not really a stretch to ask for one more.

However those tools are literally required to compile the project, and present in the systems of all Rust developers.

pre-commit offers a consistent interface to the git hooks for commits, and bundles up some versions of hooks that are shared easily.

That's fun for developers to have the same hooks set for all of their projects, but not something you need here.

I just seriously don't think it's good practice to ask any contributors to install a python package with the only purpose being generating a file to place in .git/hooks when you can also just add that file to the repo and ask people to copy it... Aside from that, pre-commit doesn't make it possible to for instance show any kind of informative error message of why your commit just failed, or even show you what rustfmt changed. It's not at all flexible.

If you guys think all of this is a good idea then sure. But if I were a new contributor, and I made a PR and somebody in the comments said "In the future could you please run pip install pre-commit and then pre-commit install inside this git repo" I'd be confused as to why the hell I need to install 6 python packages just to have a git-hook that runs rustfmt.

[offbyone]

Asking for pre-commit wouldn't block anybody from committing, it just runs checks for them if they choose to use it. The best way I've seen it used is for the CI system to also run it, so there's consistency between the two, but that's optional as well. It's just a helper.

[Arcterus]

Is there a bot we can use to force a rustfmt when commits are pushed to a PR or something like that? I feel like that might be a better solution since it doesn't require the developer to do anything.

[LevitatingBusinessMan]

@Arcterus That's already set up.

[sylvestre]

@LevitatingBusinessMan where? We have a check to tell us if it doesn't match rustfmt output but it won't make change

[LevitatingBusinessMan]

@LevitatingBusinessMan where? We have a check to tell us if it doesn't match rustfmt output but it won't make change

Right, actually applying the rustfmt changes isn't possible though.

[offbyone]

That would be really weird with git; changing a commit after pushing pretty much breaks the git model. pre-commit (which can, if the command is able, make unstaged changes to simplify it) is probably your best bet. Again, the pattern I think works best is if you run pre-commit on the committed files for PRs as well, which will enforce the expectations when submitters offer PRs.

[Arcterus]

I assume it would push a separate commit, which we would then probably squash into the original on merge. If we're going to go with pre-commit, I think we need to provide a script that installs pre-commit and installs the hooks (we could potentially install Rust in this script as well).

[LevitatingBusinessMan]

@offbyone Again, the pattern I think works best is if you run pre-commit on the committed files for PRs as well, which will enforce the expectations when submitters offer PRs.

This is exactly one of the reasons I advised against pre-commit. pre-commit isn't made to make standardized hooks like that, you can't use it server side. If you want something like that, just write a git-hook.... I feel like you guys are really overcomplicating things.

@Arcterus I assume it would push a separate commit, which we would then probably squash into the original on merge.

Bots can't really push commits directly. The best you could have is a bot that tracks the PRs, and then makes another PR against the incoming branch of said PR. None of that is good behavior.

@Arcterus If we're going to go with pre-commit, I think we need to provide a script that installs pre-commit and installs the hooks (we could potentially install Rust in this script as well).

First of all how you'd install either rust or pre-commit is dependent on your distrobution. Secondly you should be able to expect a Rust developer to have Rust installed... No need to set that up for them. There are also multiple ways to install Rust...

Again, if you want your contributors to run a git-hook, just ship your repo with a git-hook and ask them to copy it to .git/hooks. The whole thing with using third party software to handle this is insane.

Edit: If you are wondering this is what I have been using to make sure I don't forget to run rustfmt: https://gist.github.com/LevitatingBusinessMan/e0c18876ff67730a928ca6cb83caf8df

[Arcterus]

Bots can't really push commits directly. The best you could have is a bot that tracks the PRs, and then makes another PR against the incoming branch of said PR. None of that is good behavior.

They definitely can (assuming they have the correct permissions). As for whether it is good behavior, I agree that it is generally not. However, expecting someone to manually install a hook every time they clone the repo is foolish. At best, it will only slightly reduce the number of times we need to tell people to use rustfmt.

First of all how you'd install either rust or pre-commit is dependent on your distrobution. Secondly you should be able to expect a Rust developer to have Rust installed... No need to set that up for them. There are also multiple ways to install Rust...

My suggestion would just use rustup (and also would probably not be enabled by default). This would handle the common case. I've also found that most people just develop using stable or whatever. A script could help enforce using 1.40.

In any case, the easiest way to force the installation of the hook is probably to install it using build.rs, although I think doing so is a little gross.

[LevitatingBusinessMan]

@Arcterus

However, expecting someone to manually install a hook every time they clone the repo is foolish.

That is what we are doing now though.

A script could help enforce using 1.40.

The hook I made does actually enforce that. You could even configure it to ask the user if they want to automatically install the required toolchain.

In any case, the easiest way to force the installation of the hook is probably to install it using build.rs

In the CONTRIBUTING.MD you can just put tip: copy the file pre-commit to .git/hooks to make sure rustfmt is run on every commit (cp pre-commit .git/hooks) And I think that's the best way

But apparently I am the only one, seeing as sylvestre already merged the pre-commit script without even responding to my feedback.

[Arcterus]

That is what we are doing now though.

Yeah, I don't think the current setup solves the problem either.

The hook I made does actually enforce that. You could even configure it to ask the user if they want to automatically install the required toolchain.

If it did ask about installing the toolchain, that'd be pretty close to what I want then.

In the CONTRIBUTING.MD you can just put tip: copy the file pre-commit to .git/hooks to make sure rustfmt is run on every commit (cp pre-commit .git/hooks)

Given that that's worded as a suggestion, it would definitely be ignored by many people. Even if it were worded as a requirement, people would likely ignore it. The only way to truly enforce the hook is to install it automatically (either as part of the build itself or as part of some script that needs to be run beforehand to get the code to build).

[LevitatingBusinessMan]

If it did ask about installing the toolchain, that'd be pretty close to what I want then.

If you want anything like that you'll have to talk with @sylvestre because pre-commit won't let you do any types of scripting it can just run a couple of predefined tests.

[marvin-bitterlich]

Adding my experience as a new contributor:

What I would like is some kind of command/shell script that can run format for me. Running cargo fmt does not format tests and having a clear command to do all the auto fixing that is possible would be great.

In js land there is often a yarn ready or yarn ci that then calls all the relevant checks. Would there be a rusty way to achieve something similar?

format.sh: cargo fmt && format-tests.sh && cargo clippy && cargo test && anything else?

Installing a pre-commit hook can be helpful, but making it as easy as possible to manually comply with the formatting seems to me a first step in that direction.

[marvin-bitterlich]

I created a small script to run all formatters and tests and linters in one go: https://github.com/uutils/coreutils/pull/2003

While that is not a solution to this problem, it can offer contributors a self serve version and reference for what checks might be expected to be ran

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[rakshith-ravi]

Might I recommend https://github.com/autofix-ci/action. Happy to implement this for the repo (if this issue is still valid)