Add a command to install a git hook to automatically run `x.py test tidy --bless` #76356

caass · 2020-09-05T01:04:32Z

Some folks (such as myself) would probably find a lot of convenience in a pre-commit hook that automatically runs tidy before committing, to avoid burning CI time learning that your commit wasn't tidy.

I'm absolutely positive I have missed some stuff. I basically just got this to where you can run ./x.py run install-git-hook and then clicked the commit button. Please let me know what else you'd like me to add before this can be merged!

rustc-dev-guide companion PR

rust-highfive · 2020-09-05T01:04:34Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

caass · 2020-09-05T01:06:17Z

r? @jyn514

hello! tagging you because you responded to my thread on zulip.

jyn514

r=me on the idea, but r? @Mark-Simulacrum for the implementation

src/bootstrap/run.rs

src/tools/install-git-hook/src/pre-commit.sh

jyn514 · 2020-09-05T01:23:21Z

More discussion about this in https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/git.20hooks.

src/tools/install-git-hook/src/pre-commit.sh

jyn514 · 2020-09-08T02:16:40Z

Hi @caass, this is waiting on you to address some review comments. Let me know if you're having trouble and I'm happy to help!

jyn514 · 2020-09-08T02:19:21Z

Oh I guess I should also update the documentation.

You could add run install-git-hook to the usage:

rust/src/bootstrap/flags.rs

Line 424 in 9e5f7d5

Arguments:

Other than that I saw you already opened rust-lang/rustc-dev-guide#848 so no need to document it further I think. It would be nice to test that x.py run -h -v shows install-git-hook though.

petrochenkov · 2020-09-08T08:28:46Z

This probably doesn't need introduction of a new compiled tool.
Since the git hook already assumes that shell is available, it can be installed using a shell script as well (both scripts can be put into src/etc).

jyn514 · 2020-09-08T12:09:55Z

Hmm, that doesn't seem very discoverable - not many people look at src/etc. If you want to get rid of the compiled tool I'd rather just document the command to set this up: cp src/etc/pre-commit .git/hooks.

That said I do like having it in x.py run because it shows up in the help message.

petrochenkov · 2020-09-08T18:02:15Z

Actually, this is the first time I hear about x.py run, so it's probably also not too discoverable (it also doesn't list available tools).
I admit, I haven't read CONTRIBUTING.md or https://rustc-dev-guide.rust-lang.org/getting-started.html for a long time, though (even if they don't mention x.py run).
So, perhaps it would be more effective to put cp src/etc/pre-commit .git/hooks to the landing doc (which seems to be https://rustc-dev-guide.rust-lang.org/getting-started.html right now).

jyn514 · 2020-09-08T18:20:42Z

Adding the script to src/etc and only documenting it in the dev guide sounds reasonable to me.

mark-i-m · 2020-10-04T01:05:42Z

What's the status of this?

jyn514 · 2020-10-04T01:12:52Z

This is waiting on the author to address review comments.

caass · 2020-10-05T15:15:10Z

yes i accidentally broke my fork of rust somehow so it's been slow going :( soon i'll probably just rm -rf and clone again

It's ok, now I'm writing enough Rust that i'm able to get my fix elsewhere

caass · 2020-10-05T23:18:54Z

Ok! Based on the discussion above, I removed the integration with x.py and made it a stand-alone shell script living in src/etc. If any other changes need to be made, please let me know as I have thoroughly cleaned out my fork of rust 😅

jyn514

This looks like the right approach :) A few nits but mostly looks good.

Are you interested in adding some logic to set this up in x.py setup? That seems like a good place for this to go and wouldn't require reading the dev guide. I would expect it to prompt the user before installing a git hook:

$ x.py setup
... all the current output ...
Set up a git hook that runs `x.py tidy` [y]/n? y
Linked `src/etc/pre-commit.sh` to `.git/hooks/pre-commit`

Ideally it would use a hard link so the script doesn't get out of date.

No problem if not, this is fine to leave for a follow-up PR.

src/etc/pre-commit.sh

jyn514 · 2020-10-05T23:13:56Z

src/etc/pre-commit.sh

+# For help running bash scripts on Windows,
+# see https://stackoverflow.com/a/6413405/6894799


Hmm, maybe a good follow-up would be to have a version of this for powershell too. No need to add it now though.

From what I can tell, git only understands unix-like scripts, and won't run e.g. pre-commit.ps1. I suspect that if the user has git installed, they'll be able to run git scripts as well, so actually this line is redundant...I think...I haven't used git on windows in a while.

cc @mati865 - do you know if this will work on windows, and if not, how to make it work?

@jyn514 Git for Windows ships bash shell.
I haven't tried using hooks there but they should work.

I hate powershell so personally I use bash/zsh everywhere, even on Windows.

src/etc/pre-commit.sh

src/bootstrap/setup.rs

jyn514 · 2020-10-06T02:17:01Z

src/bootstrap/setup.rs

+
+    Ok(if should_install {
+        let src = src_path.join("src").join("etc").join("pre-commit.sh");
+        let dst = src_path.join(".git").join("hooks").join("pre-commit");


Small nit: this plays badly with worktrees. Do you mind changing it to use git rev-parse --git-common-dir instead?

x.py encountered an error -- do you already have the git hook installed? Not a directory (os error 20)

Although I guess the user might not want to install it globally for all worktrees ... I'm not sure what the right behavior is here. Let's leave it for now and we can fix it in a follow-up.

yeah i also noticed a similar error trying to run a merge commit...i'm kind of starting to get out of my league at this point tho so i'm more than happy to move it to a follow up 😅

jyn514 · 2020-10-06T02:18:52Z

@bors r+

Thanks for working on this! I think windows and worktree support can go in a follow-up PR if necessary.

bors · 2020-10-06T02:18:53Z

📌 Commit 0845627 has been approved by jyn514

bors · 2020-10-06T03:33:42Z

⌛ Testing commit 0845627 with merge 962fe4442fb751b1b115aeb8042f9da65530219b...

bors · 2020-10-06T07:48:43Z

💥 Test timed out

jyn514 · 2020-10-06T13:26:19Z

Timed out on x86_64 apple :(

@bors retry

FYI @rust-lang/infra aarch-gnu looks like it's been failing for a while (and no one noticed because it's auto-fallible).

--- expected_show_coverage.generics.txt	2020-10-06 03:36:29.936045671 +0000
+++ /checkout/obj/build/aarch64-unknown-linux-gnu/test/run-make-fulldeps/coverage-reports-deadcode/coverage-reports-deadcode/actual_show_coverage.generics.txt	2020-10-06 05:21:51.420743607 +0000
@@ -29,12 +29,12 @@
    18|      2|        println!("BOOM times {}!!!", self.strength);
    19|      2|    }
   ------------------
-  | <generics::Firework<i32> as core::ops::drop::Drop>::drop:
+  | <generics::Firework<i32> as core[a7a74cee373f048]::ops::drop::Drop>::drop:
   |   17|      1|    fn drop(&mut self) {
   |   18|      1|        println!("BOOM times {}!!!", self.strength);
   |   19|      1|    }
   ------------------
-  | <generics::Firework<f64> as core::ops::drop::Drop>::drop:
+  | <generics::Firework<f64> as core[a7a74cee373f048]::ops::drop::Drop>::drop:
   |   17|      1|    fn drop(&mut self) {
   |   18|      1|        println!("BOOM times {}!!!", self.strength);
   |   19|      1|    }

------------------------------------------
stderr:
------------------------------------------
Error: 1
Error: 1
diff failed, and not suppressed without `// ignore-llvm-cov-show-diffs` in ../coverage/generics.rs
make: *** [../coverage-reports-base/Makefile:45: generics] Error 1

------------------------------------------

failures:
    [run-make] run-make-fulldeps/coverage-reports-base
    [run-make] run-make-fulldeps/coverage-reports-deadcode

bors · 2020-10-06T14:51:57Z

⌛ Testing commit 0845627 with merge 9fdaeb3...

bors · 2020-10-06T17:06:49Z

☀️ Test successful - checks-actions, checks-azure
Approved by: jyn514
Pushing 9fdaeb3 to master...

This is a companion to [this PR](rust-lang/rust#76356), which deals with including functionality for automatically running `tidy --bless` on each commit. Undo editor auto-formatting and clarify git hook renaming a word Phrasing Apply suggestions from code review Co-authored-by: Camelid <37223377+camelid@users.noreply.github.com>

There isn't a way to pass --remove yet, but you can rm if u like

a009e28

rust-highfive assigned nikomatsakis Sep 5, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2020

rust-highfive assigned jyn514 and unassigned nikomatsakis Sep 5, 2020

caass mentioned this pull request Sep 5, 2020

Add section describing git hook functionality rust-lang/rustc-dev-guide#848

Merged

jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Sep 5, 2020

jyn514 reviewed Sep 5, 2020

View reviewed changes

src/bootstrap/run.rs Outdated Show resolved Hide resolved

src/tools/install-git-hook/src/pre-commit.sh Outdated Show resolved Hide resolved

jyn514 reviewed Sep 5, 2020

View reviewed changes

src/tools/install-git-hook/src/pre-commit.sh Outdated Show resolved Hide resolved

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 8, 2020

This comment has been minimized.

Sign in to view

jyn514 mentioned this pull request Sep 19, 2020

ICEs should print the top of the query stack #76920

Closed

jyn514 added the A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself label Sep 20, 2020

caass added 3 commits October 5, 2020 18:49

Merge branch 'master' into hooks

44af74f

Move script to src/etc

7de557b

Remove the rust stuff and just make it a simple shell script

f53d436

It's ok, now I'm writing enough Rust that i'm able to get my fix elsewhere

jyn514 reviewed Oct 5, 2020

View reviewed changes

caass added 2 commits October 5, 2020 19:35

Clean up pre-commit.sh

68ca474

Add install_git_hook_maybe to setup.rs

d585c96

jyn514 reviewed Oct 6, 2020

View reviewed changes

src/bootstrap/setup.rs Show resolved Hide resolved

src/bootstrap/setup.rs Outdated Show resolved Hide resolved

src/bootstrap/setup.rs Outdated Show resolved Hide resolved

src/bootstrap/setup.rs Outdated Show resolved Hide resolved

Make changes based on @jyn514's comments

0845627

jyn514 reviewed Oct 6, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 6, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 6, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2020

pietroalbini mentioned this pull request Oct 6, 2020

AArch64 Linux CI failure #77615

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 6, 2020

bors merged commit 9fdaeb3 into rust-lang:master Oct 6, 2020

rustbot added this to the 1.49.0 milestone Oct 6, 2020

This was referenced Oct 6, 2020

x.py breaks submodule state when run in a pre-commit hook from a worktree #77620

Closed

Add support for worktrees to x.py setup git hook #77684

Closed

Skepfyr mentioned this pull request Jun 14, 2021

Compile error: static lifetime not satisfied but it is #86172

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a command to install a git hook to automatically run `x.py test tidy --bless` #76356

Add a command to install a git hook to automatically run `x.py test tidy --bless` #76356

caass commented Sep 5, 2020 •

edited

Loading

rust-highfive commented Sep 5, 2020

caass commented Sep 5, 2020

jyn514 left a comment

jyn514 commented Sep 5, 2020

jyn514 commented Sep 8, 2020

jyn514 commented Sep 8, 2020 •

edited

Loading

petrochenkov commented Sep 8, 2020

jyn514 commented Sep 8, 2020

petrochenkov commented Sep 8, 2020

jyn514 commented Sep 8, 2020

This comment has been minimized.

mark-i-m commented Oct 4, 2020

jyn514 commented Oct 4, 2020

caass commented Oct 5, 2020

caass commented Oct 5, 2020

jyn514 left a comment •

edited

Loading

jyn514 Oct 5, 2020

caass Oct 5, 2020

jyn514 Oct 5, 2020

mati865 Oct 6, 2020

jyn514 Oct 6, 2020 •

edited

Loading

caass Oct 6, 2020

jyn514 commented Oct 6, 2020

bors commented Oct 6, 2020

bors commented Oct 6, 2020

bors commented Oct 6, 2020

jyn514 commented Oct 6, 2020

bors commented Oct 6, 2020

bors commented Oct 6, 2020

		# For help running bash scripts on Windows,
		# see https://stackoverflow.com/a/6413405/6894799

Add a command to install a git hook to automatically run x.py test tidy --bless #76356

Add a command to install a git hook to automatically run x.py test tidy --bless #76356

Conversation

caass commented Sep 5, 2020 • edited Loading

rust-highfive commented Sep 5, 2020

caass commented Sep 5, 2020

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 commented Sep 5, 2020

jyn514 commented Sep 8, 2020

jyn514 commented Sep 8, 2020 • edited Loading

petrochenkov commented Sep 8, 2020

jyn514 commented Sep 8, 2020

petrochenkov commented Sep 8, 2020

jyn514 commented Sep 8, 2020

This comment has been minimized.

mark-i-m commented Oct 4, 2020

jyn514 commented Oct 4, 2020

caass commented Oct 5, 2020

caass commented Oct 5, 2020

jyn514 left a comment • edited Loading

Choose a reason for hiding this comment

jyn514 Oct 5, 2020

Choose a reason for hiding this comment

caass Oct 5, 2020

Choose a reason for hiding this comment

jyn514 Oct 5, 2020

Choose a reason for hiding this comment

mati865 Oct 6, 2020

Choose a reason for hiding this comment

jyn514 Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

caass Oct 6, 2020

Choose a reason for hiding this comment

jyn514 commented Oct 6, 2020

bors commented Oct 6, 2020

bors commented Oct 6, 2020

bors commented Oct 6, 2020

jyn514 commented Oct 6, 2020

bors commented Oct 6, 2020

bors commented Oct 6, 2020

Add a command to install a git hook to automatically run `x.py test tidy --bless` #76356

Add a command to install a git hook to automatically run `x.py test tidy --bless` #76356

caass commented Sep 5, 2020 •

edited

Loading

jyn514 commented Sep 8, 2020 •

edited

Loading

jyn514 left a comment •

edited

Loading

jyn514 Oct 6, 2020 •

edited

Loading