CI: Overhaul static checks to use pre-commit

[Repiteo]

¹

Consolidates (almost) every single static_checks function/command into pre-commit directly. This has the benefit of adding these static checks as proper pre-commit hooks to the repo itself, so more issues have the potential to be caught well ahead of time. If possible, existing misc scripts were either replaced with a more parsable python equivalent, or removed entirely if a hook repo exists to achieve the same goal.

Only three exceptions remain that are still checked via static_checks: gitignore_check.sh, pytest, and xmllint. gitignore_check.sh is pretty specific to the workflow of that file specifically, so it's most sensible to keep as-is. xmllint technically has a hook available, but it requires docker & that's not nearly as likely for a user to have compared to npm/dotnet. pytest actually would've worked just fine, but based off a conversation² when I attempted to add it to pre-commit, it isn't a good candidate for a pre-commit so I'll leave it as-is.

Footnotes

1. https://github.com/godotengine/godot/pull/91417#pullrequestreview-2034011566 ↩

2. https://github.com/pre-commit/pre-commit.com/pull/960 ↩

[akien-mga]

Thanks for working on this! That's amazing.

I have yet to review the code properly, but some minor issues found when testing on Linux with pre-commit run --all:

• file_format seems to find a bunch of stuff to fix that we missed in the master branch. I made Style: Trim trailing whitespace and ensure newline at EOF #91696 with those changes.

• The eslint-html check errors out:

eslint-html..............................................................Failed
- hook id: eslint
- exit code: 2

Oops! Something went wrong! :(

ESLint: 8.46.0

ESLint couldn't find the config "airbnb-base" to extend from. Please check that the name of the config is correct.

The config "airbnb-base" was referenced from the config file in "/home/akien/Godot/godot/platform/web/.eslintrc.js".

If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.

[akien-mga]

Reviewed the code, looks really good.

[akien-mga]

Just noting that we'll miss one feature here, which is that this action provides a problem matcher in the GitHub diff view.

But in my experience most contributors don't seem to notice it (it's only visible in diff view, not on the PR itself), and the action was lacking in other configuration aspects so I think the tradeoffs are fine.

[Repiteo]

pre-commit should be set to output a git-diff log if any checks fail or changes are made, so it won't be that much of a loss.

[akien-mga]

All these hooks have the same id, is that intended?

[Repiteo]

It is intentional, yes. They're all pulling from the same original hook implementing eslint, but it's called multiple times to replicate the multiple eslint checks we use. That's why name is different for each one, to differentiate the different checks during output.

[akien-mga]

BTW, if you want to have a look at how we could solve the problem those two PRs aimed to address (users not having python3 in PATH):

• Change pre-commit entry exe from python3 to python. #90634
• Pre-commit: Specify default python version, rely on shebang #90662

[akien-mga]

BTW, if you want to have a look at how we could solve the problem those two PRs aimed to address (users not having python3 in PATH):

• Change pre-commit entry exe from python3 to python. #90634
• Pre-commit: Specify default python version, rely on shebang #90662

I meant to tackle this possibly in a follow up PR, none of the two proposed solutions seem to work well cross-platform currently.

The current commit needs adding executable permission to files to work on Linux:

header-guards............................................................Failed
- hook id: header-guards
- exit code: 1

Executable `misc/scripts/header_guards.py` is not executable

file-format..............................................................Failed
- hook id: file-format
- exit code: 1

Executable `misc/scripts/file_format.py` is not executable

dotnet-format............................................................Failed
- hook id: dotnet-format
- exit code: 1

Executable `misc/scripts/dotnet_format.py` is not executable

But based on #90662 (review) this might not work on macOS.

[Repiteo]

I'll leave the python3 as-is for now then, and do some digging on how it's implemented in pre-commit. I think it can be setup such that it won't rely on shbang whatsoever, but I'm fine tackling that in a followup.

[akien-mga]

Beautiful:

clang-format.............................................................Passed
black....................................................................Passed
mypy.....................................................................Passed
codespell................................................................Passed
eslint-engine............................................................Passed
eslint-libs..............................................................Passed
eslint-sw................................................................Passed
eslint-html..............................................................Passed
make-rst.................................................................Passed
doc-status...............................................................Passed
jsdoc....................................................................Passed
copyright-headers........................................................Passed
header-guards............................................................Passed
file-format..............................................................Passed
dotnet-format............................................................Passed

[akien-mga]

Thanks! Great refactor 🎉

[AThousandShips]

It looks like this broke CI and no style checks are run at all, see:
https://github.com/godotengine/godot/actions/runs/9043846245/job/24851885660?pr=91829#step:6:82

Which has changed files but skips them

I suspect this is because it can't detect the files since there are no changed files

[Repiteo]

Then it probably needs that git diff logic passed to it after all, or something similar. Should be an easy fix either way, I'll get something up later today

[patwork]

@Repiteo Hi, would you be so kind as to take another look at .pre-commit-config.yaml? I've been working on upgrading eslint to version 9 and I see now that the automatic checks are returning errors:

https://github.com/godotengine/godot/pull/91863/checks

• eslint-* because pre-commit is using eslint 8
• jsdoc propably because some arguments are missing?

Is it possible for pre-commit to run npm run docs and npm run lint directly from /platform/web? Especially for eslint this is important, due to the fact that for version 9 you have to change the working directory first (the npm script does this).