Add pre-commit hooks and git config for libc++ & git-clang-format. #73798
Conversation
This patch aims to ease the entry into libc++'s codebase by (A) Setting up the users clangFormat.extensions values in the .git/config. libc++ needs this to format extensionless files. (2) Install a pre-commit hook which checks for files which are misformatted and offers to format them. These can be installed by running ./libcxx/utils/formatting/setup-formatting-config.sh Documentation is incoming
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-github-workflow @llvm/pr-subscribers-libcxx Author: Eric (EricWF) ChangesThis patch aims to ease the entry into libc++'s codebase by (A) Setting up the users clangFormat.extensions values in the (2) Install a pre-commit hook which checks for files which are These can be installed by running Documentation is incoming Full diff: https://github.com/llvm/llvm-project/pull/73798.diff 2 Files Affected:
diff --git a/libcxx/utils/formatting/libcxx-pre-commit-formatting-hook.sh b/libcxx/utils/formatting/libcxx-pre-commit-formatting-hook.sh
new file mode 100755
index 000000000000000..741b686e5e5a402
--- /dev/null
+++ b/libcxx/utils/formatting/libcxx-pre-commit-formatting-hook.sh
@@ -0,0 +1,31 @@
+#!/bin/bash
+
+
+# Define the staged files
+staged_files=$(git diff --cached --name-only --diff-filter=d -- libcxx/ libcxxabi/ libunwind/)
+
+# Check if any of the staged files have not been properly formatted with git-clang-format
+
+echo "Checking $file..."
+formatting_diff=$(git-clang-format --diff -- "$staged_files") # quotes are used here
+if [[ "$formatting_diff" != "no modified files to format" && "$formatting_diff" != "clang-format did not modify any files" ]]; then
+ echo "$file has not been formatted with git-clang-format."
+ git-clang-format --diff -- "$staged_files"
+ read -p "Format the files [Y/n]? " -n 1 -r
+ echo
+ if [[ $REPLY =~ ^[Yy]$ ]]
+ then
+ git-clang-format -- "$staged_files"
+ git add "$staged_files"
+ exit 0
+ else
+ echo "No changes were made to the git config."
+ exit 1
+ fi
+
+fi
+
+
+# Everything checks out
+echo "All staged files have been formatted with git-clang-format."
+exit 0
diff --git a/libcxx/utils/formatting/setup-formatting-config.sh b/libcxx/utils/formatting/setup-formatting-config.sh
new file mode 100755
index 000000000000000..9332ee9e9adebac
--- /dev/null
+++ b/libcxx/utils/formatting/setup-formatting-config.sh
@@ -0,0 +1,75 @@
+#!/bin/bash
+
+
+# Jump up to the root directory of the repository
+
+git_repo_path="$(git rev-parse --show-toplevel)"
+cd "$git_repo_path"
+
+# Set up git-clang-format config so that it formats files without extensions as needed by libc++
+function check_formatting_config() {
+ # Set the desired git config key and value
+ CONFIG_KEY="clangFormat.extensions"
+ CONFIG_VALUE=" c,h,m,mm,cpp,cxx,hpp,,"
+
+ # Get the actual value of the config key
+ ACTUAL_VALUE=$(git config --local --get $CONFIG_KEY)
+
+ # Check if the actual value is the same as the desired value
+ if [[ "$ACTUAL_VALUE" == "$CONFIG_VALUE" ]]; then
+ echo "The git config value for $CONFIG_KEY is correctly set to $CONFIG_VALUE"
+ else
+ echo "Setting up git-clang-format config for libc++..."
+ # Prompt the user to set the git config key to the desired value
+ echo "Git config key $CONFIG_KEY is not set or incorrect."
+ read -p "Would you like to set it to $CONFIG_VALUE [Y/n]? " -n 1 -r
+ echo
+ if [[ $REPLY =~ ^[Yy]$ ]]
+ then
+ git config --local $CONFIG_KEY "$CONFIG_VALUE"
+ echo "Git config key $CONFIG_KEY has been set to $CONFIG_VALUE"
+ else
+ echo "No changes were made to the git config."
+ fi
+ fi
+}
+
+# Check for an installation of git-clang-format
+function check_clang_format() {
+ # Check if git-clang-format is installed
+ GIT_CLANG_FORMAT_COMMAND="git-clang-format"
+ if command -v $GIT_CLANG_FORMAT_COMMAND >/dev/null 2>&1; then
+ echo "git-clang-format is installed in your system."
+ else
+ echo "Warning: git-clang-format is not installed in your system."
+ fi
+}
+# ...
+# Existing script here
+# ...
+
+# Check if libcxx-formatting.sh is installed in pre-commit hook
+function check_pre_commit_hooks() {
+ # Check if libcxx-formatting.sh is present in pre-commit hook
+ PRE_COMMIT_FILE=".git/hooks/pre-commit"
+ EXPECTED_COMMIT_SCRIPT=". ./libcxx/utils/formatting/libcxx-pre-commit-formatting-hook.sh"
+ if grep -q -F "$EXPECTED_COMMIT_SCRIPT" "$PRE_COMMIT_FILE"; then
+ echo "libcxx-pre-commit-formatting-hook.sh is already installed in pre-commit hook."
+ else
+ # Offer to install it
+ read -p "libcxx-pre-commit-formatting-hook.sh is not installed. Would you like to install it [Y/n]? " -n 1 -r
+ echo
+ if [[ $REPLY =~ ^[Yy]$ ]]
+ then
+ echo "Installing libcxx-pre-commit-formatting-hook.sh..."
+ echo "$EXPECTED_COMMIT_SCRIPT" >> "$PRE_COMMIT_FILE"
+ echo "Installed libcxx-pre-commit-formatting-hook.sh to pre-commit hook."
+ else
+ echo "No changes were made to the pre-commit hook."
+ fi
+ fi
+}
+
+check_formatting_config
+check_clang_format
+check_pre_commit_hooks
|
| @@ -0,0 +1,31 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Could we reuse the same script we use in the CI job instead? In .github/workflows/pr-code-format.yml, we use code-format-tools/llvm/utils/git/code-format-helper.py. If we reused it, it would also solve the problem of formatting our Python files and would ensure that they don't get out of sync.
I would also suggest that this can be done LLVM-wide -- this really shouldn't be libc++ specific since we're doing the same thing as the rest of the monorepo. CC @tru
There was a problem hiding this comment.
Yeah - I think it should use the python script because of two reasons 1) bash doesn't work on windows 2) we would get the same experience locally and in the CI.
There was a problem hiding this comment.
Already working on a change to make github actions use this.
I'll make this a little more configurable, but lets not block this on making everyone come with us.
There was a problem hiding this comment.
No I think it's fine to start with libc++, but I think we should have the discussion and see if the rest of the community would like to follow libc++ here and have the commit hook for everyone. I think it's beneficial.
There was a problem hiding this comment.
Sure.
Regarding the python script... It doesn't support formatting code outside of github actions. This is meant for our users to use. Ideally we would reconcile the two of these, but I don't think we can make that happen here.
There was a problem hiding this comment.
We don't have to block making this change on the rest of the community. We're not forcing anyone to use the scripts, we're merely providing them and who wants to use them will use them. I really think we should put those scripts in a libc++ agnostic location and make them apply exactly the same thing as pr-code-format.yml so that this becomes a way to essentially run the same checks as that workflow, but locally.
There was a problem hiding this comment.
It doesn't support formatting code outside of github actions. This is meant for our users to use. Ideally we would reconcile the two of these, but I don't think we can make that happen here.
Happy to work on this, I rather we fix this now than have two different scripts floating around. I will look at this asap.
There was a problem hiding this comment.
Sorry I misunderstood the initial question, so I got a bit confused. These scripts do separate things.
This script should be installed under .git/hooks/pre-commit, and it will auto-run whenever the user types git commit in their checkout of LLVM.
The code-format-tools/llvm/utils/git/code-format-helper.py is a utility for creating check-runs in a github actions workflow. It also acts as a shallow wrapper around git-clang-format, but it does not run locally nor is it meant for formatting code.
That said, we probably don't need this second github workflow, since these two tools should produce almost the same outcome (ours will format files without extensions that are outside of libcxx/include, but the code-format-helper.py limits itself to allowing no-extension files under libcxx/include.
If we want to fully merge their behaviors, we need to make git-clang-format handle libc++ non-extension headers better. I would be happy taking the same approach code-format-helper.py takes
Thank you, @EricWF , for making this PR. I wanted to have something similar, but it looks like you beat me to it. Would there be interest in a set of scripts (in addition to this hook) that would allow a developer to manually invoke If there is interest, I would be happy to work on it! (Edit: I do see |
| @@ -9,16 +9,24 @@ permissions: | |||
|
|
|||
| jobs: | |||
| check_generated_files: | |||
| runs-on: ubuntu-latest | |||
| runs-on: | |||
There was a problem hiding this comment.
I think there's a bit of confusion here. This job doesn't actually check for clang-format anymore. We should just stop installing clang-format in this job, I guess I missed that when we stopped enforcing clang-format via this job.
We exclusively enforce clang-format via the pr-code-format.yml workflow now.
There was a problem hiding this comment.
Yeah, I was trying to hijack it to test the added hooks, but it's trickier that I would have liked.
|
Personally, I think what we want is a combination of this PR and @tru 's #73957. The pre-commit hook as written in #73957 seems like a better approach since it uses exactly the same logic as the pre-commit CI, it will run Darker too, and it will work on Windows since it's Python. It's also LLVM wide instead of being libc++ specific. However, this PR contains documentation and a script to install the pre-commit hook (which we might want to write in Python just for the sake of Windows). Personally, I think the best of all worlds would be:
That way, all of LLVM will have a nicely documented way of installing this hook, and the hook will mirror exactly what we do for all of LLVM in the pre-commit CI. That would really rock. |
| function check_clang_format() { | ||
| # Check if git-clang-format is installed | ||
| GIT_CLANG_FORMAT_COMMAND="git-clang-format" | ||
| if command -v $GIT_CLANG_FORMAT_COMMAND >/dev/null 2>&1; then |
There was a problem hiding this comment.
This allows every version of clang-format right? Not sure how we can force the latest stable clang-format here.
| Setting Up the Formatting Hooks | ||
| ------------------------------- | ||
|
|
||
| Run the ``install-formatting-hooks.sh`` script. This will check if ``git-clang-format`` is installed in your system, |
| @@ -132,6 +133,10 @@ RUN <<EOF | |||
| sudo /tmp/llvm.sh $(($LLVM_HEAD_VERSION - 1)) all # latest release | |||
| sudo /tmp/llvm.sh $LLVM_HEAD_VERSION all # current ToT | |||
| sudo apt-get install -y libomp5-$LLVM_HEAD_VERSION | |||
|
|
|||
| # Install the latest git-clang-format | |||
| sudo ln -s $(which git-clang-format-$LLVM_HEAD_VERSION) /usr/bin/clang-format | |||
There was a problem hiding this comment.
Did we change our policy? Shouldn't this be the latest stable version, $(($LLVM_HEAD_VERSION - 1)), instead?
| fi | ||
| } | ||
|
|
||
| check_formatting_config "clangFormat.extensions" "c,h,m,mm,cpp,cxx,hpp,," 0 |
There was a problem hiding this comment.
| check_formatting_config "clangFormat.extensions" "c,h,m,mm,cpp,cxx,hpp,," 0 | |
| check_formatting_config "clangFormat.extensions" "c,h,m,mm,cpp,cxx,hpp,inc," 0 |
The extension is used for modules and in under src.
…t hook As part of llvm#73798 there was some discussion about using the format helper to run from a git-hook. That was not possible for a number of reasons, but with these changes it can now be installed as a hook and then run on the local cache in git instead of a diff between revisions. This also checks for two environment variables DARKER_FORMAT_PATH and CLANG_FORMAT_PATH where you can specify the path to the program you want to use.
…t hook (#73957) As part of #73798 there was some discussion about using the format helper to run from a git-hook. That was not possible for a number of reasons, but with these changes it can now be installed as a hook and then run on the local cache in git instead of a diff between revisions. This also checks for two environment variables DARKER_FORMAT_PATH and CLANG_FORMAT_PATH where you can specify the path to the program you want to use.
|
#73957 has landed now! |
This patch aims to ease the entry into libc++'s codebase by
(A) Setting up the users clangFormat.extensions values in the
.git/config. libc++ needs this to format extensionless files.
(2) Install a pre-commit hook which checks for files which are
misformatted and offers to format them.
These can be installed by running
./libcxx/utils/formatting/setup-formatting-config.sh
Documentation is incoming