Auto merge of #7361 - xFrednet:5394-expand-setup-command, r=flip1995
Added `cargo dev setup git-hook` and updated `cargo dev setup intellij` including a `remove` command
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:
* [x] Linux (by `@xFrednet)`
* [ ] Windows (All used commands run inside the git bash, so it's very likely to work as well `@xFrednet)`
* [ ] macOS
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 7265d1b..b202fc4 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -115,7 +115,7 @@
`git clone https://github.com/rust-lang/rust/`.
Then you can run a `cargo dev` command to automatically make Clippy use the rustc-repo via path-dependencies
which `IntelliJ Rust` will be able to understand.
-Run `cargo dev ide_setup --repo-path <repo-path>` where `<repo-path>` is a path to the rustc repo
+Run `cargo dev setup intellij --repo-path <repo-path>` where `<repo-path>` is a path to the rustc repo
you just cloned.
The command will add path-dependencies pointing towards rustc-crates inside the rustc repo to
Clippys `Cargo.toml`s and should allow `IntelliJ Rust` to understand most of the types that Clippy uses.
diff --git a/clippy_dev/src/fmt.rs b/clippy_dev/src/fmt.rs
index edc8e6a..c81eb40 100644
--- a/clippy_dev/src/fmt.rs
+++ b/clippy_dev/src/fmt.rs
@@ -86,7 +86,7 @@
},
CliError::RaSetupActive => {
eprintln!(
- "error: a local rustc repo is enabled as path dependency via `cargo dev ide_setup`.
+ "error: a local rustc repo is enabled as path dependency via `cargo dev setup intellij`.
Not formatting because that would format the local repo as well!
Please revert the changes to Cargo.tomls first."
);
diff --git a/clippy_dev/src/ide_setup.rs b/clippy_dev/src/ide_setup.rs
deleted file mode 100644
index defb113..0000000
--- a/clippy_dev/src/ide_setup.rs
+++ /dev/null
@@ -1,103 +0,0 @@
-use std::fs;
-use std::fs::File;
-use std::io::prelude::*;
-use std::path::{Path, PathBuf};
-
-// This module takes an absolute path to a rustc repo and alters the dependencies to point towards
-// the respective rustc subcrates instead of using extern crate xyz.
-// This allows rust analyzer to analyze rustc internals and show proper information inside clippy
-// code. See https://github.com/rust-analyzer/rust-analyzer/issues/3517 and https://github.com/rust-lang/rust-clippy/issues/5514 for details
-
-/// # Panics
-///
-/// Panics if `rustc_path` does not lead to a rustc repo or the files could not be read
-pub fn run(rustc_path: Option<&str>) {
- // we can unwrap here because the arg is required by clap
- let rustc_path = PathBuf::from(rustc_path.unwrap())
- .canonicalize()
- .expect("failed to get the absolute repo path");
- assert!(rustc_path.is_dir(), "path is not a directory");
- let rustc_source_basedir = rustc_path.join("compiler");
- assert!(
- rustc_source_basedir.is_dir(),
- "are you sure the path leads to a rustc repo?"
- );
-
- let clippy_root_manifest = fs::read_to_string("Cargo.toml").expect("failed to read ./Cargo.toml");
- let clippy_root_lib_rs = fs::read_to_string("src/driver.rs").expect("failed to read ./src/driver.rs");
- inject_deps_into_manifest(
- &rustc_source_basedir,
- "Cargo.toml",
- &clippy_root_manifest,
- &clippy_root_lib_rs,
- )
- .expect("Failed to inject deps into ./Cargo.toml");
-
- let clippy_lints_manifest =
- fs::read_to_string("clippy_lints/Cargo.toml").expect("failed to read ./clippy_lints/Cargo.toml");
- let clippy_lints_lib_rs =
- fs::read_to_string("clippy_lints/src/lib.rs").expect("failed to read ./clippy_lints/src/lib.rs");
- inject_deps_into_manifest(
- &rustc_source_basedir,
- "clippy_lints/Cargo.toml",
- &clippy_lints_manifest,
- &clippy_lints_lib_rs,
- )
- .expect("Failed to inject deps into ./clippy_lints/Cargo.toml");
-}
-
-fn inject_deps_into_manifest(
- rustc_source_dir: &Path,
- manifest_path: &str,
- cargo_toml: &str,
- lib_rs: &str,
-) -> std::io::Result<()> {
- // do not inject deps if we have aleady done so
- if cargo_toml.contains("[target.'cfg(NOT_A_PLATFORM)'.dependencies]") {
- eprintln!(
- "cargo dev ide_setup: warning: deps already found inside {}, doing nothing.",
- manifest_path
- );
- return Ok(());
- }
-
- let extern_crates = lib_rs
- .lines()
- // get the deps
- .filter(|line| line.starts_with("extern crate"))
- // we have something like "extern crate foo;", we only care about the "foo"
- // ↓ ↓
- // extern crate rustc_middle;
- .map(|s| &s[13..(s.len() - 1)]);
-
- let new_deps = extern_crates.map(|dep| {
- // format the dependencies that are going to be put inside the Cargo.toml
- format!(
- "{dep} = {{ path = \"{source_path}/{dep}\" }}\n",
- dep = dep,
- source_path = rustc_source_dir.display()
- )
- });
-
- // format a new [dependencies]-block with the new deps we need to inject
- let mut all_deps = String::from("[target.'cfg(NOT_A_PLATFORM)'.dependencies]\n");
- new_deps.for_each(|dep_line| {
- all_deps.push_str(&dep_line);
- });
- all_deps.push_str("\n[dependencies]\n");
-
- // replace "[dependencies]" with
- // [dependencies]
- // dep1 = { path = ... }
- // dep2 = { path = ... }
- // etc
- let new_manifest = cargo_toml.replacen("[dependencies]\n", &all_deps, 1);
-
- // println!("{}", new_manifest);
- let mut file = File::create(manifest_path)?;
- file.write_all(new_manifest.as_bytes())?;
-
- println!("Dependency paths injected: {}", manifest_path);
-
- Ok(())
-}
diff --git a/clippy_dev/src/lib.rs b/clippy_dev/src/lib.rs
index 69f42ac..72bdaf8 100644
--- a/clippy_dev/src/lib.rs
+++ b/clippy_dev/src/lib.rs
@@ -14,9 +14,9 @@
pub mod bless;
pub mod fmt;
-pub mod ide_setup;
pub mod new_lint;
pub mod serve;
+pub mod setup;
pub mod stderr_length_check;
pub mod update_lints;
diff --git a/clippy_dev/src/main.rs b/clippy_dev/src/main.rs
index 7040c25..f5bd086 100644
--- a/clippy_dev/src/main.rs
+++ b/clippy_dev/src/main.rs
@@ -2,8 +2,8 @@
// warn on lints, that are included in `rust-lang/rust`s bootstrap
#![warn(rust_2018_idioms, unused_lifetimes)]
-use clap::{App, Arg, ArgMatches, SubCommand};
-use clippy_dev::{bless, fmt, ide_setup, new_lint, serve, stderr_length_check, update_lints};
+use clap::{App, AppSettings, Arg, ArgMatches, SubCommand};
+use clippy_dev::{bless, fmt, new_lint, serve, setup, stderr_length_check, update_lints};
fn main() {
let matches = get_clap_config();
@@ -36,7 +36,20 @@
("limit_stderr_length", _) => {
stderr_length_check::check();
},
- ("ide_setup", Some(matches)) => ide_setup::run(matches.value_of("rustc-repo-path")),
+ ("setup", Some(sub_command)) => match sub_command.subcommand() {
+ ("intellij", Some(matches)) => setup::intellij::setup_rustc_src(
+ matches
+ .value_of("rustc-repo-path")
+ .expect("this field is mandatory and therefore always valid"),
+ ),
+ ("git-hook", Some(matches)) => setup::git_hook::install_hook(matches.is_present("force-override")),
+ _ => {},
+ },
+ ("remove", Some(sub_command)) => match sub_command.subcommand() {
+ ("git-hook", Some(_)) => setup::git_hook::remove_hook(),
+ ("intellij", Some(_)) => setup::intellij::remove_rustc_src(),
+ _ => {},
+ },
("serve", Some(matches)) => {
let port = matches.value_of("port").unwrap().parse().unwrap();
let lint = matches.value_of("lint");
@@ -48,6 +61,7 @@
fn get_clap_config<'a>() -> ArgMatches<'a> {
App::new("Clippy developer tooling")
+ .setting(AppSettings::ArgRequiredElseHelp)
.subcommand(
SubCommand::with_name("bless")
.about("bless the test output changes")
@@ -140,16 +154,42 @@
.about("Ensures that stderr files do not grow longer than a certain amount of lines."),
)
.subcommand(
- SubCommand::with_name("ide_setup")
- .about("Alter dependencies so Intellij Rust can find rustc internals")
- .arg(
- Arg::with_name("rustc-repo-path")
- .long("repo-path")
- .short("r")
- .help("The path to a rustc repo that will be used for setting the dependencies")
- .takes_value(true)
- .value_name("path")
- .required(true),
+ SubCommand::with_name("setup")
+ .about("Support for setting up your personal development environment")
+ .setting(AppSettings::ArgRequiredElseHelp)
+ .subcommand(
+ SubCommand::with_name("intellij")
+ .about("Alter dependencies so Intellij Rust can find rustc internals")
+ .arg(
+ Arg::with_name("rustc-repo-path")
+ .long("repo-path")
+ .short("r")
+ .help("The path to a rustc repo that will be used for setting the dependencies")
+ .takes_value(true)
+ .value_name("path")
+ .required(true),
+ ),
+ )
+ .subcommand(
+ SubCommand::with_name("git-hook")
+ .about("Add a pre-commit git hook that formats your code to make it look pretty")
+ .arg(
+ Arg::with_name("force-override")
+ .long("force-override")
+ .short("f")
+ .help("Forces the override of an existing git pre-commit hook")
+ .required(false),
+ ),
+ ),
+ )
+ .subcommand(
+ SubCommand::with_name("remove")
+ .about("Support for undoing changes done by the setup command")
+ .setting(AppSettings::ArgRequiredElseHelp)
+ .subcommand(SubCommand::with_name("git-hook").about("Remove any existing pre-commit git hook"))
+ .subcommand(
+ SubCommand::with_name("intellij")
+ .about("Removes rustc source paths added via `cargo dev setup intellij`"),
),
)
.subcommand(
diff --git a/clippy_dev/src/setup/git_hook.rs b/clippy_dev/src/setup/git_hook.rs
new file mode 100644
index 0000000..f27b69a
--- /dev/null
+++ b/clippy_dev/src/setup/git_hook.rs
@@ -0,0 +1,79 @@
+use std::fs;
+use std::path::Path;
+
+/// Rusts setup uses `git rev-parse --git-common-dir` to get the root directory of the repo.
+/// I've decided against this for the sake of simplicity and to make sure that it doesn't install
+/// the hook if `clippy_dev` would be used in the rust tree. The hook also references this tool
+/// for formatting and should therefor only be used in a normal clone of clippy
+const REPO_GIT_DIR: &str = ".git";
+const HOOK_SOURCE_FILE: &str = "util/etc/pre-commit.sh";
+const HOOK_TARGET_FILE: &str = ".git/hooks/pre-commit";
+
+pub fn install_hook(force_override: bool) {
+ if !check_precondition(force_override) {
+ return;
+ }
+
+ // So a little bit of a funny story. Git on unix requires the pre-commit file
+ // to have the `execute` permission to be set. The Rust functions for modifying
+ // these flags doesn't seem to work when executed with normal user permissions.
+ //
+ // However, there is a little hack that is also being used by Rust itself in their
+ // setup script. Git saves the `execute` flag when syncing files. This means
+ // that we can check in a file with execution permissions and the sync it to create
+ // a file with the flag set. We then copy this file here. The copy function will also
+ // include the `execute` permission.
+ match fs::copy(HOOK_SOURCE_FILE, HOOK_TARGET_FILE) {
+ Ok(_) => {
+ println!("info: the hook can be removed with `cargo dev remove git-hook`");
+ println!("git hook successfully installed");
+ },
+ Err(err) => eprintln!(
+ "error: unable to copy `{}` to `{}` ({})",
+ HOOK_SOURCE_FILE, HOOK_TARGET_FILE, err
+ ),
+ }
+}
+
+fn check_precondition(force_override: bool) -> bool {
+ // Make sure that we can find the git repository
+ let git_path = Path::new(REPO_GIT_DIR);
+ if !git_path.exists() || !git_path.is_dir() {
+ eprintln!("error: clippy_dev was unable to find the `.git` directory");
+ return false;
+ }
+
+ // Make sure that we don't override an existing hook by accident
+ let path = Path::new(HOOK_TARGET_FILE);
+ if path.exists() {
+ if force_override {
+ return delete_git_hook_file(path);
+ }
+
+ eprintln!("error: there is already a pre-commit hook installed");
+ println!("info: use the `--force-override` flag to override the existing hook");
+ return false;
+ }
+
+ true
+}
+
+pub fn remove_hook() {
+ let path = Path::new(HOOK_TARGET_FILE);
+ if path.exists() {
+ if delete_git_hook_file(path) {
+ println!("git hook successfully removed");
+ }
+ } else {
+ println!("no pre-commit hook was found");
+ }
+}
+
+fn delete_git_hook_file(path: &Path) -> bool {
+ if let Err(err) = fs::remove_file(path) {
+ eprintln!("error: unable to delete existing pre-commit git hook ({})", err);
+ false
+ } else {
+ true
+ }
+}
diff --git a/clippy_dev/src/setup/intellij.rs b/clippy_dev/src/setup/intellij.rs
new file mode 100644
index 0000000..bf741e6
--- /dev/null
+++ b/clippy_dev/src/setup/intellij.rs
@@ -0,0 +1,223 @@
+use std::fs;
+use std::fs::File;
+use std::io::prelude::*;
+use std::path::{Path, PathBuf};
+
+// This module takes an absolute path to a rustc repo and alters the dependencies to point towards
+// the respective rustc subcrates instead of using extern crate xyz.
+// This allows IntelliJ to analyze rustc internals and show proper information inside Clippy
+// code. See https://github.com/rust-lang/rust-clippy/issues/5514 for details
+
+const RUSTC_PATH_SECTION: &str = "[target.'cfg(NOT_A_PLATFORM)'.dependencies]";
+const DEPENDENCIES_SECTION: &str = "[dependencies]";
+
+const CLIPPY_PROJECTS: &[ClippyProjectInfo] = &[
+ ClippyProjectInfo::new("root", "Cargo.toml", "src/driver.rs"),
+ ClippyProjectInfo::new("clippy_lints", "clippy_lints/Cargo.toml", "clippy_lints/src/lib.rs"),
+ ClippyProjectInfo::new("clippy_utils", "clippy_utils/Cargo.toml", "clippy_utils/src/lib.rs"),
+];
+
+/// Used to store clippy project information to later inject the dependency into.
+struct ClippyProjectInfo {
+ /// Only used to display information to the user
+ name: &'static str,
+ cargo_file: &'static str,
+ lib_rs_file: &'static str,
+}
+
+impl ClippyProjectInfo {
+ const fn new(name: &'static str, cargo_file: &'static str, lib_rs_file: &'static str) -> Self {
+ Self {
+ name,
+ cargo_file,
+ lib_rs_file,
+ }
+ }
+}
+
+pub fn setup_rustc_src(rustc_path: &str) {
+ let rustc_source_dir = match check_and_get_rustc_dir(rustc_path) {
+ Ok(path) => path,
+ Err(_) => return,
+ };
+
+ for project in CLIPPY_PROJECTS {
+ if inject_deps_into_project(&rustc_source_dir, project).is_err() {
+ return;
+ }
+ }
+
+ println!("info: the source paths can be removed again with `cargo dev remove intellij`");
+}
+
+fn check_and_get_rustc_dir(rustc_path: &str) -> Result<PathBuf, ()> {
+ let mut path = PathBuf::from(rustc_path);
+
+ if path.is_relative() {
+ match path.canonicalize() {
+ Ok(absolute_path) => {
+ println!("info: the rustc path was resolved to: `{}`", absolute_path.display());
+ path = absolute_path;
+ },
+ Err(err) => {
+ eprintln!("error: unable to get the absolute path of rustc ({})", err);
+ return Err(());
+ },
+ };
+ }
+
+ let path = path.join("compiler");
+ println!("info: looking for compiler sources at: {}", path.display());
+
+ if !path.exists() {
+ eprintln!("error: the given path does not exist");
+ return Err(());
+ }
+
+ if !path.is_dir() {
+ eprintln!("error: the given path is not a directory");
+ return Err(());
+ }
+
+ Ok(path)
+}
+
+fn inject_deps_into_project(rustc_source_dir: &Path, project: &ClippyProjectInfo) -> Result<(), ()> {
+ let cargo_content = read_project_file(project.cargo_file)?;
+ let lib_content = read_project_file(project.lib_rs_file)?;
+
+ if inject_deps_into_manifest(rustc_source_dir, project.cargo_file, &cargo_content, &lib_content).is_err() {
+ eprintln!(
+ "error: unable to inject dependencies into {} with the Cargo file {}",
+ project.name, project.cargo_file
+ );
+ Err(())
+ } else {
+ Ok(())
+ }
+}
+
+/// `clippy_dev` expects to be executed in the root directory of Clippy. This function
+/// loads the given file or returns an error. Having it in this extra function ensures
+/// that the error message looks nice.
+fn read_project_file(file_path: &str) -> Result<String, ()> {
+ let path = Path::new(file_path);
+ if !path.exists() {
+ eprintln!("error: unable to find the file `{}`", file_path);
+ return Err(());
+ }
+
+ match fs::read_to_string(path) {
+ Ok(content) => Ok(content),
+ Err(err) => {
+ eprintln!("error: the file `{}` could not be read ({})", file_path, err);
+ Err(())
+ },
+ }
+}
+
+fn inject_deps_into_manifest(
+ rustc_source_dir: &Path,
+ manifest_path: &str,
+ cargo_toml: &str,
+ lib_rs: &str,
+) -> std::io::Result<()> {
+ // do not inject deps if we have already done so
+ if cargo_toml.contains(RUSTC_PATH_SECTION) {
+ eprintln!(
+ "warn: dependencies are already setup inside {}, skipping file",
+ manifest_path
+ );
+ return Ok(());
+ }
+
+ let extern_crates = lib_rs
+ .lines()
+ // only take dependencies starting with `rustc_`
+ .filter(|line| line.starts_with("extern crate rustc_"))
+ // we have something like "extern crate foo;", we only care about the "foo"
+ // extern crate rustc_middle;
+ // ^^^^^^^^^^^^
+ .map(|s| &s[13..(s.len() - 1)]);
+
+ let new_deps = extern_crates.map(|dep| {
+ // format the dependencies that are going to be put inside the Cargo.toml
+ format!(
+ "{dep} = {{ path = \"{source_path}/{dep}\" }}\n",
+ dep = dep,
+ source_path = rustc_source_dir.display()
+ )
+ });
+
+ // format a new [dependencies]-block with the new deps we need to inject
+ let mut all_deps = String::from("[target.'cfg(NOT_A_PLATFORM)'.dependencies]\n");
+ new_deps.for_each(|dep_line| {
+ all_deps.push_str(&dep_line);
+ });
+ all_deps.push_str("\n[dependencies]\n");
+
+ // replace "[dependencies]" with
+ // [dependencies]
+ // dep1 = { path = ... }
+ // dep2 = { path = ... }
+ // etc
+ let new_manifest = cargo_toml.replacen("[dependencies]\n", &all_deps, 1);
+
+ // println!("{}", new_manifest);
+ let mut file = File::create(manifest_path)?;
+ file.write_all(new_manifest.as_bytes())?;
+
+ println!("info: successfully setup dependencies inside {}", manifest_path);
+
+ Ok(())
+}
+
+pub fn remove_rustc_src() {
+ for project in CLIPPY_PROJECTS {
+ remove_rustc_src_from_project(project);
+ }
+}
+
+fn remove_rustc_src_from_project(project: &ClippyProjectInfo) -> bool {
+ let mut cargo_content = if let Ok(content) = read_project_file(project.cargo_file) {
+ content
+ } else {
+ return false;
+ };
+ let section_start = if let Some(section_start) = cargo_content.find(RUSTC_PATH_SECTION) {
+ section_start
+ } else {
+ println!(
+ "info: dependencies could not be found in `{}` for {}, skipping file",
+ project.cargo_file, project.name
+ );
+ return true;
+ };
+
+ let end_point = if let Some(end_point) = cargo_content.find(DEPENDENCIES_SECTION) {
+ end_point
+ } else {
+ eprintln!(
+ "error: the end of the rustc dependencies section could not be found in `{}`",
+ project.cargo_file
+ );
+ return false;
+ };
+
+ cargo_content.replace_range(section_start..end_point, "");
+
+ match File::create(project.cargo_file) {
+ Ok(mut file) => {
+ file.write_all(cargo_content.as_bytes()).unwrap();
+ println!("info: successfully removed dependencies inside {}", project.cargo_file);
+ true
+ },
+ Err(err) => {
+ eprintln!(
+ "error: unable to open file `{}` to remove rustc dependencies for {} ({})",
+ project.cargo_file, project.name, err
+ );
+ false
+ },
+ }
+}
diff --git a/clippy_dev/src/setup/mod.rs b/clippy_dev/src/setup/mod.rs
new file mode 100644
index 0000000..3834f5a
--- /dev/null
+++ b/clippy_dev/src/setup/mod.rs
@@ -0,0 +1,2 @@
+pub mod git_hook;
+pub mod intellij;
diff --git a/doc/basics.md b/doc/basics.md
index e2e307c..89d572a 100644
--- a/doc/basics.md
+++ b/doc/basics.md
@@ -90,8 +90,10 @@
cargo dev update_lints
# create a new lint and register it
cargo dev new_lint
+# automatically formatting all code before each commit
+cargo dev setup git-hook
# (experimental) Setup Clippy to work with IntelliJ-Rust
-cargo dev ide_setup
+cargo dev setup intellij
```
## lintcheck
diff --git a/util/etc/pre-commit.sh b/util/etc/pre-commit.sh
new file mode 100755
index 0000000..528f895
--- /dev/null
+++ b/util/etc/pre-commit.sh
@@ -0,0 +1,21 @@
+#!/bin/sh
+
+# hide output
+set -e
+
+# Update lints
+cargo dev update_lints
+git add clippy_lints/src/lib.rs
+
+# Formatting:
+# Git will not automatically add the formatted code to the staged changes once
+# fmt was executed. This collects all staged files rs files that are currently staged.
+# They will later be added back.
+#
+# This was proudly stolen and adjusted from here:
+# https://medium.com/@harshitbangar/automatic-code-formatting-with-git-66c3c5c26798
+files=$( (git diff --cached --name-only --diff-filter=ACMR | grep -Ei "\.rs$") || true)
+if [ ! -z "${files}" ]; then
+ cargo dev fmt
+ git add $(echo "$files" | paste -s -d " " -)
+fi
