Skip to content
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

CI: Overhaul static checks to use pre-commit #91597

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
64 changes: 5 additions & 59 deletions .github/workflows/static_checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,28 +19,13 @@ jobs:
- name: Install APT dependencies
uses: awalsh128/cache-apt-pkgs-action@latest
with:
packages: dos2unix libxml2-utils moreutils
packages: libxml2-utils

- name: Install Python dependencies and general setup
run: |
pip3 install pytest==7.1.2 mypy==0.971
pip3 install pytest==7.1.2
git config diff.wsErrorHighlight all

- name: Get changed files
id: changed-files
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
if [ "${{ github.event_name }}" == "pull_request" ]; then
files=$(git diff-tree --no-commit-id --name-only -r HEAD^1..HEAD 2> /dev/null || true)
elif [ "${{ github.event_name }}" == "push" -a "${{ github.event.forced }}" == "false" -a "${{ github.event.created }}" == "false" ]; then
files=$(git diff-tree --no-commit-id --name-only -r ${{ github.event.before }}..${{ github.event.after }} 2> /dev/null || true)
fi
echo "$files" >> changed.txt
cat changed.txt
files=$(echo "$files" | grep -v 'thirdparty' | xargs -I {} sh -c 'echo "./{}"' | tr '\n' ' ')
echo "CHANGED_FILES=$files" >> $GITHUB_ENV

# This needs to happen before Python and npm execution; it must happen before any extra files are written.
- name: .gitignore checks (gitignore_check.sh)
run: |
Expand All @@ -49,51 +34,12 @@ jobs:
- name: Style checks via pre-commit
uses: pre-commit/action@v3.0.1
with:
extra_args: --verbose --files ${{ env.CHANGED_FILES }}

- name: File formatting checks (file_format.sh)
run: |
bash ./misc/scripts/file_format.sh changed.txt

- name: Header guards formatting checks (header_guards.sh)
run: |
bash ./misc/scripts/header_guards.sh changed.txt

- name: Python scripts static analysis (mypy_check.sh)
run: |
if grep -qE '\.py$|SConstruct|SCsub' changed.txt || [ -z "$(cat changed.txt)" ]; then
bash ./misc/scripts/mypy_check.sh
else
echo "Skipping Python static analysis as no Python files were changed."
fi

- name: Python builders checks via pytest (pytest_builders.sh)
run: |
bash ./misc/scripts/pytest_builders.sh
extra_args: --verbose

- name: JavaScript style and documentation checks via ESLint and JSDoc
- name: Python builders checks via pytest
run: |
if grep -q "\.js" changed.txt || [ -z "$(cat changed.txt)" ]; then
cd platform/web
npm ci
npm run lint
npm run docs -- -d dry-run
else
echo "Skipping JavaScript formatting as no Web/JS files were changed."
fi
pytest ./tests/python_build

- name: Class reference schema checks
run: |
xmllint --noout --schema doc/class.xsd doc/classes/*.xml modules/*/doc_classes/*.xml platform/*/doc_classes/*.xml

- name: Documentation checks
run: |
doc/tools/doc_status.py doc/classes modules/*/doc_classes platform/*/doc_classes

- name: Spell checks via codespell
if: github.event_name == 'pull_request' && env.CHANGED_FILES != ''
uses: codespell-project/actions-codespell@v2
Comment on lines -93 to -95
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

with:
skip: "./bin,./thirdparty,*.desktop,*.gen.*,*.po,*.pot,*.rc,./AUTHORS.md,./COPYRIGHT.txt,./DONORS.md,./core/input/gamecontrollerdb.txt,./core/string/locales.h,./editor/project_converter_3_to_4.cpp,./misc/scripts/codespell.sh,./platform/android/java/lib/src/com,./platform/web/node_modules,./platform/web/package-lock.json"
ignore_words_list: "breaked,colour,curvelinear,doubleclick,expct,findn,gird,hel,inout,lod,mis,nd,numer,ot,requestor,te,thirdparty,vai"
path: ${{ env.CHANGED_FILES }}
152 changes: 139 additions & 13 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
exclude: |
(?x)^(
.*thirdparty/.*|
.*-so_wrap\.(h|c)$
)

repos:
- repo: https://github.com/pre-commit/mirrors-clang-format
rev: v17.0.6
Expand All @@ -7,10 +13,8 @@ repos:
types_or: [text]
exclude: |
(?x)^(
tests/python_build.*|
.*thirdparty.*|
.*platform/android/java/lib/src/com.*|
.*-so_wrap.*
tests/python_build/.*|
platform/android/java/lib/src/com/.*
)

- repo: https://github.com/psf/black-pre-commit-mirror
Expand All @@ -19,33 +23,155 @@ repos:
- id: black
files: (\.py$|SConstruct|SCsub)
types_or: [text]
exclude: .*thirdparty.*
args:
- --line-length=120

- repo: https://github.com/pre-commit/mirrors-mypy
rev: v0.971
hooks:
- id: mypy
files: \.py$
types_or: [text]
args: [--config-file=./misc/scripts/mypy.ini]

- repo: https://github.com/codespell-project/codespell
rev: v2.2.6
hooks:
- id: codespell
types_or: [text]
exclude: |
(?x)^(
.*\.desktop$|
.*\.gitignore$|
.*\.po$|
.*\.pot$|
.*\.rc$|
\.mailmap$|
AUTHORS.md$|
COPYRIGHT.txt$|
DONORS.md$|
core/input/gamecontrollerdb.txt$|
core/string/locales.h$|
editor/project_converter_3_to_4.cpp$|
platform/android/java/lib/src/com/.*|
platform/web/package-lock.json$
)
args:
- --enable-colors
- --write-changes
- --check-hidden
- --quiet-level
- '3'
- --ignore-words-list
- aesthetic,aesthetics,breaked,cancelled,colour,curvelinear,doubleclick,expct,findn,gird,hel,inout,lod,mis,nd,numer,ot,requestor,te,thirdparty,vai
- --builtin
- clear,rare,en-GB_to_en-US

### Requires Docker; look into alternative implementation.
# - repo: https://github.com/comkieffer/pre-commit-xmllint.git
# rev: 1.0.0
# hooks:
# - id: xmllint
# language: docker
# types_or: [text]
# files: ^(doc/classes|.*/doc_classes)/.*\.xml$
# args: [--schema, doc/class.xsd]

- repo: https://github.com/pre-commit/mirrors-eslint
rev: v8.46.0
hooks:
- id: eslint
name: eslint-engine
files: ^(platform/web/js/engine/|js/jsdoc2rst/).*\.js$
args: [--fix, --no-eslintrc, --config, platform/web/.eslintrc.engine.js]
additional_dependencies: &eslint-deps
- eslint@8.46.0
- eslint-config-airbnb-base@15.0.0
- eslint-plugin-import@2.28.0
- eslint-plugin-html@7.1.0
- '@html-eslint/eslint-plugin@0.19.1'
- '@html-eslint/parser@0.19.1'
- id: eslint
name: eslint-libs
files: ^(platform/web/js/libs/|modules/).*\.js$
args: [--fix, --no-eslintrc, --config, platform/web/.eslintrc.libs.js]
additional_dependencies: *eslint-deps
- id: eslint
name: eslint-sw
files: ^misc/dist/html/service-worker\.js$
args: [--fix, --no-eslintrc, --config, platform/web/.eslintrc.sw.js]
additional_dependencies: *eslint-deps
- id: eslint
name: eslint-html
files: ^misc/dist/html/.*\.html$
types: [html]
args: [--fix, --no-eslintrc, --config, platform/web/.eslintrc.html.js]
additional_dependencies: *eslint-deps

- repo: local
hooks:
- id: make-rst
name: make-rst
language: python
entry: python3 doc/tools/make_rst.py doc/classes modules platform --dry-run --color
pass_filenames: false
files: ^(doc/classes|.*/doc_classes)/.*\.xml$

- id: doc-status
name: doc-status
language: python
files: ^(doc|modules|platform).*xml$
entry: python3 doc/tools/doc_status.py
files: ^(doc/classes|.*/doc_classes)/.*\.xml$

- id: jsdoc
name: jsdoc
language: node
entry: jsdoc
files: ^platform/web/js/engine/(engine|config|features)\.js$
args: [--template, platform/web/js/jsdoc2rst/, --destination, '', -d, dry-run]
additional_dependencies: ["jsdoc@4.0.2"]

- id: copyright-headers
name: copyright-headers
language: python
files: \.(c|h|cpp|hpp|cc|cxx|m|mm|inc|java)$
entry: python3 misc/scripts/copyright_headers.py
files: \.(c|h|cpp|hpp|cc|cxx|m|mm|inc|java)$
exclude: |
(?x)^(
.*thirdparty.*|
.*-so_wrap.*|
core/math/bvh_.*\.inc$|
platform/android/java/lib/src/com.*|
platform/android/java/lib/src/org/godotengine/godot/gl/GLSurfaceView.*|
platform/android/java/lib/src/org/godotengine/godot/gl/EGLLogWrapper.*|
platform/android/java/lib/src/org/godotengine/godot/utils/ProcessPhoenix.*
platform/android/java/lib/src/com/.*|
platform/android/java/lib/src/org/godotengine/godot/gl/GLSurfaceView\.java$|
platform/android/java/lib/src/org/godotengine/godot/gl/EGLLogWrapper\.java$|
platform/android/java/lib/src/org/godotengine/godot/utils/ProcessPhoenix\.java$
)

- id: header-guards
name: header-guards
language: python
entry: python3 misc/scripts/header_guards.py
files: \.(h|hpp)$
exclude: |
(?x)^(
.*/thread\.h$|
.*/platform_config\.h$|
.*/platform_gl\.$h
)

- id: file-format
name: file-format
language: python
entry: python3 misc/scripts/file_format.py
types_or: [text]
exclude: |
(?x)^(
.*\.test\.txt$|
.*\.svg$|
.*\.patch$|
.*\.out$|
modules/gdscript/tests/scripts/parser/features/mixed_indentation_on_blank_lines\.gd$|
modules/gdscript/tests/scripts/parser/warnings/empty_file_newline_comment\.notest\.gd$|
modules/gdscript/tests/scripts/parser/warnings/empty_file_newline\.notest\.gd$|
platform/android/java/lib/src/com/google/.*
)

- id: dotnet-format
Expand Down
8 changes: 0 additions & 8 deletions misc/scripts/codespell.sh

This file was deleted.

16 changes: 10 additions & 6 deletions misc/scripts/dotnet_format.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@
import os
import sys

# Create dummy generated files.
if len(sys.argv) < 2:
print("Invalid usage of dotnet_format.py, it should be called with a path to one or multiple files.")
sys.exit(1)

# Create dummy generated files, if needed.
for path in [
"modules/mono/SdkPackageVersions.props",
]:
if os.path.exists(path):
continue
os.makedirs(os.path.dirname(path), exist_ok=True)
with open(path, "w", encoding="utf-8", newline="\n") as f:
f.write("<Project />")
Expand All @@ -17,14 +23,12 @@
os.environ["GodotSkipGenerated"] = "true"

# Match all the input files to their respective C# project.
input_files = [os.path.normpath(x) for x in sys.argv]
projects = {
path: [f for f in sys.argv if os.path.commonpath([f, path]) == path]
path: " ".join([f for f in sys.argv[1:] if os.path.commonpath([f, path]) == path])
for path in [os.path.dirname(f) for f in glob.glob("**/*.csproj", recursive=True)]
}

# Run dotnet format on all projects with more than 0 modified files.
for path, files in projects.items():
if len(files) > 0:
command = f"dotnet format {path} --include {' '.join(files)}"
os.system(command)
if files:
os.system(f"dotnet format {path} --include {files}")
51 changes: 51 additions & 0 deletions misc/scripts/file_format.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#!/usr/bin/env python
# -*- coding: utf-8 -*-

import sys

if len(sys.argv) < 2:
print("Invalid usage of file_format.py, it should be called with a path to one or multiple files.")
sys.exit(1)

BOM = b"\xef\xbb\xbf"

changed = []
invalid = []

for file in sys.argv[1:]:
try:
with open(file, "rt", encoding="utf-8") as f:
original = f.read()
except UnicodeDecodeError:
invalid.append(file)
continue

if original == "":
continue

EOL = "\r\n" if file.endswith((".csproj", ".sln", ".bat")) or file.startswith("misc/msvs") else "\n"
WANTS_BOM = file.endswith((".csproj", ".sln"))

revamp = EOL.join([line.rstrip("\n\r\t ") for line in original.splitlines(True)]).rstrip(EOL) + EOL

new_raw = revamp.encode(encoding="utf-8")
if not WANTS_BOM and new_raw.startswith(BOM):
new_raw = new_raw[len(BOM) :]
elif WANTS_BOM and not new_raw.startswith(BOM):
new_raw = BOM + new_raw

with open(file, "rb") as f:
old_raw = f.read()

if old_raw != new_raw:
changed.append(file)
with open(file, "wb") as f:
f.write(new_raw)

if changed:
for file in changed:
print(f"FIXED: {file}")
if invalid:
for file in invalid:
print(f"REQUIRES MANUAL CHANGES: {file}")
sys.exit(1)