Add a file formatting script for pre-commit

[aaronfranke]

This PR adds a script for file formatting, set up to be used with pre-commit, and formats the files in this repo using the script.

• The formatting script is cross-platform, works on Windows, MacOS, and Linux.
• The formatting script can run without installing any OS-level dependencies, except for Python. Once Python is installed you can install pre-commit via pip install pre-commit. Then pre-commit run --all-files in the repo to run.
• The formatting script can run automatically on changed files when making a commit using Git's pre-commit hooks feature, if you opt-in via pre-commit install in the repo.
• Due to the static_checks.yml file, the formatting script runs automatically on PRs via GitHub Actions CI checks.

The script ensures text files do not have trailing spaces, do not have BOM, do not have carriage returns, and are terminated with one \n character to ensure text files are POSIX-compliant.

I excluded 3rdparty/, *.bin.h, and *.ttf.h files from formatting because 3rdparty/ is third-party code and the headers are serialized text files which intentionally contain trailing spaces in the comments.

[bkaradzic]

The script ensures text files do not have trailing spaces, do not have BOM, do not have carriage returns, and are terminated with one \n character to ensure text files are POSIX-compliant.

Why do we care about this?!

[mcourteaux]

The script ensures text files do not have trailing spaces, do not have BOM, do not have carriage returns, and are terminated with one \n character to ensure text files are POSIX-compliant.

Why do we care about this?!

I understand that this might come across as arbitrary, but I do appreciate these properties of a text file: \n newlines, no BOM, and no trailing spaces. It generally is more compatible with text editors, tools, preprocessors, etc... Trailing spaces are just awkward: when you merge a line with the next line, but you have trailing spaces, it's obviously gonna look weird. I have even highlighting in vim that reveals trailing spaces in text files, to make sure I don't have them. I checked bgfx source code, and there are no trailing spaces except a handful of empty lines (containing only spaces) in c99/bgfx.h. Most changes indeed happen in examples/.

Lemme just state, that I am glad none of those issues exist in the main source, but are limited to the examples right now. Otherwise, working in the main source could be a pain, because you constantly modify lines you didn't touch manually, because your editor takes care of deleting trailing spaces, and normalizing newlines.

After all, still arbitrary, but I do like these standardization requirements.

[aaronfranke]

I'll go over them one by one:

• Trailing spaces: These are useless and do nothing. See what @mcourteaux wrote for more detail.
• No BOM: This ensures that files with only ASCII text are both valid ASCII and valid UTF-8, ensuring compatibility with all tools. The BOM is not required and is recommended to be excluded.
• No carriage returns: This ensures that line endings are consistent across platforms. BGFX already specifies this in the .gitattributes file using eol=lf, only now it's enforced by a CI check.
• One \n at end of file: The POSIX definition of a text file is a series of lines terminated by \n characters, which means that there must be a \n at the end. Many tools will not work correctly if this is missing, for example cat in a terminal, or older versions of C/C++ compilers. Most modern tools are able to handle bad files, or show a warning. GitHub itself shows a red 🚫 emoji on diffs where the terminating newline is missing.

However, the main motivation here is to avoid annoying formatting differences when bringing BGFX into an environment which enforces these rules. For another repo's formatting checks, it is fine to exclude third-party folders, but this also affects things like IDEs. I have my IDE configured to automatically fix these problems, so if I were to edit these files, I would end up with a bunch of unexpected diffs or I would need to temporarily disable this behavior.

[bkaradzic]

What I would prefer, is to run pre-commit check and then fail CI on bad formatting. I think it's undesirable that CI modifies files.

[aaronfranke]

@bkaradzic That's what it does. The CI will fail on bad formatting. It doesn't push any commits automatically.

The feature where it runs locally when making a commit, modifying the user's local files, is opt-in, so it won't happen unless users choose that.

[bkaradzic]

this action is in maintenance-only mode and will not be accepting new features.

generally you want to use pre-commit.ci which is faster and has more features.

From here:
https://github.com/pre-commit/action?tab=readme-ov-file

[bkaradzic]

Submit fixes as separate PR first.

[bkaradzic]

Also beware that double whitespace at the end of line in Markdown syntax for line break.

[mcourteaux]

That's what it does. The CI will fail on bad formatting. It doesn't push any commits automatically.

I don't think it does. I see the action clearly calling out to the formatting script, which rewrites files if old != new. The formatting script should check, and return exit code 1 to fail the git commit step I believe.

[aaronfranke]

I see the action clearly calling out to the formatting script, which rewrites files if old != new.

Yes, it does write the files. Then, because the CI runs in a temporary VM, the changes made by CI are immediately discarded.

[mcourteaux]

Then, because the CI runs in a temporary VM, the changes made by CI are immediately discarded.

Then how does it signal that the formatting is not good? I don't see it returning exit code 1 in case the file is not correctly formatted.

[aaronfranke]

Pre-commit returns 1 when the checks failed.

% pre-commit run --all-files
file-format..............................................................Failed
- hook id: file-format
- files were modified by this hook

FIXED: examples/39-assao/vs_assao.sc
FIXED: examples/39-assao/cs_assao_prepare_depths_half.sc
...
FIXED: examples/39-assao/cs_assao_generate_q3base.sc
FIXED: examples/44-sss/fs_sss_gbuffer.sc

% echo $?
1

If the files are all good already:

% pre-commit run --all-files
file-format..............................................................Passed

% echo $?                   
0

[bkaradzic]

So now you can rebase this PR, and run checks on current repo state, and it should pass.

bindings must not cause failure, since those auto generated files.

[aaronfranke]

Added the bindings/ folder and include/bgfx/c99/bgfx.h to the exclude list (files changed by a37185e).

[mcourteaux]

Pre-commit returns 1 when the checks failed.

I don't want to be annoying, but really, I don't see how that happens. When files are printed as FIXED, they are in the changed list of the script. The script ONLY seems to return 1 when there are undecodable files that go in the invalid list.

[aaronfranke]

@mcourteaux The return code of file_format.py is not the same as the return code of pre-commit run --all-files. The former works as you described, the latter is a wrapper around it.

[bkaradzic]

Why .pre-commit-config.yaml file is not tucked into .github directory?

[bkaradzic]

Also I don't see this actually running https://github.com/bkaradzic/bgfx/actions/runs/12251664115/job/34176731561?pr=3383

[aaronfranke]

@bkaradzic https://github.com/bkaradzic/bgfx/actions/runs/12251664128/job/34176730710?pr=3383

As for .pre-commit-config.yaml, the root of the repo is the only place pre-commit recognizes this: https://pre-commit.com/#2-add-a-pre-commit-configuration

[bkaradzic]

@aaronfranke Thanks!

The last thing is:

this action is in maintenance-only mode and will not be accepting new features.

generally you want to use pre-commit.ci which is faster and has more features.

Why not switch to pre-commit.ci since we already know this one is discontinued?

[aaronfranke]

I haven't used pre-commit.ci before. The GitHub Action still works fine, and it says it's in maintenance mode, which is not the same as discontinued. Also, pre-commit.ci has some features that I'm not sure are desired:

auto fixing pull requests: if tools make changes to files during a pull request, pre-commit.ci will automatically fix the pull request.

Anyway, if pre-commit.ci is desired, I think it is very easy to set up. Judging from their website, the only required setup is to enable it on their website, and then for this repo, delete the static_checks.yml file added by this PR to prevent it running twice (once on pre-commit.ci and one on GitHub Actions).

[bkaradzic]

The reason why I'm bringing up this is because I don't want to have to deal with it in case maintainer decides to stop maintaining it.

[aaronfranke]

I updated the PR to remove static_checks.yml, so now it won't run on GitHub Actions, instead it can be run manually, and you can enable it to run on pre-commit.ci.

[bkaradzic]

Actually it's preferable to run automatically on PR.

[aaronfranke]

Yes, that's what the pre-commit.ci website lets you do, run it automatically on each PR.