Skip to content

Added cargo dev setup git-hook and updated cargo dev setup intellij including a remove command #7361

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

Merged
merged 8 commits into from
Jun 25, 2021

Conversation

xFrednet
Copy link
Member

@xFrednet xFrednet commented Jun 16, 2021

This PR enables our dev tool to install a git hook that formats the code before each commit and also runs update_lints to make sure that everything is registered correctly. The script is located at util/etc/pre-commit.sh. I found it reasonable to locate it in the util folder and decided to add a etc in correlation to the main rust repo and to bring a bit of structure into it.

  • The hook can be installed via: cargo dev setup git-hook
  • And removed via: cargo dev remove git-hook

cc: #5394

The refactoring of src/ide_setup.rs to src/setup/intellij.rs is an extra commit to simplify the review.


Changes:

  • Added cargo dev setup git-hook for formatting before every commit
  • Added cargo dev remove git-hook to remove the hook again
  • Added cargo dev remove intellij to remove rustc source path dependencies
  • Changed cargo dev ide_setup to cargo dev setup intellij

changelog: none

This is only an internal change and therefore not worth an entry in the general change log.


Tested on:

  • Linux (by @xFrednet)
  • Windows (All used commands run inside the git bash, so it's very likely to work as well @xFrednet)
  • macOS

@rust-highfive
Copy link

r? @giraffate

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 16, 2021
@flip1995
Copy link
Member

How much work would it be to add a cargo dev remove intellij?

@xFrednet
Copy link
Member Author

xFrednet commented Jun 16, 2021

Well, not much I believe, but I haven't looked into it, as I don't use IntelliJ. I also plan to add cargo dev setup vscode-tasks and the removal of it. I can take a look at the IntelliJ removal as well if you like. 🙃 (It would make sense to add that)

@flip1995
Copy link
Member

That would be great! If you find that it would be too much work for this PR, let me know and I'll review it in the current state.

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 17, 2021
@xFrednet xFrednet marked this pull request as draft June 17, 2021 21:58
@xFrednet xFrednet changed the title Added cargo dev setup git-hook and changed cargo dev ide_setup to cargo dev setup intellij WIP: Added cargo dev setup git-hook and changed cargo dev ide_setup to cargo dev setup intellij Jun 17, 2021
@xFrednet
Copy link
Member Author

Converted to draft until I've reviewed the work required to undo the IntelliJ setup command. The current version should still be fully working.

@xFrednet
Copy link
Member Author

I've looked into adding the cargo dev remove intellij command. It looks pretty simple, however I noticed that the current implementation also seems to be a bit broken:

  • It doesn't add the new source paths to the clippy_util/Cargo.toml
  • It also adds clippy_util as a dependency in clippy_lints/Cargo.toml
    clippy_utils = { path = "/home/xfrednet/workspace/rust/rust/compiler/clippy_utils" }
    

I want to fix them as well, I guess that this will be about 150 additional changes.

@xFrednet xFrednet force-pushed the 5394-expand-setup-command branch from 0fd01ba to bbf7bed Compare June 21, 2021 22:24
@xFrednet
Copy link
Member Author

I'm not done yet, this commit only cleans up some message and makes the setup more expandable. The rest of the work should still be reasonably straight forward. 🙃

@xFrednet xFrednet force-pushed the 5394-expand-setup-command branch from 4d10e4d to 4ce37d9 Compare June 22, 2021 16:26
@xFrednet
Copy link
Member Author

Okay, I've updated the dev tool to also include a remove command for intellij and give some nicer error messages when calling the setup command.

# Inject dependencies
cargo dev setup intellij
# Remove dependencies again
cargo dev remove intellij

I was debating if it should be renamed to something like cargo dev setup rustc-sources --repo-path ... and cargo dev remove rustc-sources but that also sounded a bit weird. I'm open to suggestions, or we leave it as it is 🙃. I'll try to test the git-hook on Windows soon, but I don't have access to a mac to test the hook on there as well.

@xFrednet xFrednet marked this pull request as ready for review June 22, 2021 19:03
@xFrednet xFrednet changed the title WIP: Added cargo dev setup git-hook and changed cargo dev ide_setup to cargo dev setup intellij Added cargo dev setup git-hook and updated cargo dev setup intellij including a remove command Jun 22, 2021
@flip1995
Copy link
Member

Code LGTM now. I'll give those commands a try locally and then this should be good to merge.

@flip1995 flip1995 force-pushed the 5394-expand-setup-command branch from 21e2f71 to 8e969cd Compare June 25, 2021 09:21
@flip1995
Copy link
Member

Rebased and good to merge. Thanks, great work!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 25, 2021

📌 Commit 8e969cd has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jun 25, 2021

⌛ Testing commit 8e969cd with merge a03dfb9...

@bors
Copy link
Contributor

bors commented Jun 25, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing a03dfb9 to master...

@bors bors merged commit a03dfb9 into rust-lang:master Jun 25, 2021
@xFrednet
Copy link
Member Author

xFrednet commented Jun 25, 2021

You're welcome! I plan to also add a setup to add vs-code tasks to compile and test Clippy. Could you check the git-hook suggestion in the linked ticket as that has been implemented in this PR #5394.

Was there a reason for the rebase? 😅 (just interested :))

@flip1995
Copy link
Member

flip1995 commented Jun 25, 2021

I had to rebase locally anyway (since this branch still used the old nightly), so I figured, why not push it. I don't think this was necessary though.

I checked it off in #5394

@xFrednet
Copy link
Member Author

Ahh makes sense. Thank you :)

@xFrednet xFrednet deleted the 5394-expand-setup-command branch July 28, 2021 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants