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

Switch to pre-commit for Git hooks #46235

Closed
wants to merge 1 commit into from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Feb 19, 2021

Marking as a draft since this needs to be tested on Windows and macOS.

This replaces our custom Git hook solution for pre-commit, a framework for pre-commit hooks written in Python.

It should have better Windows support than our existing implementation, on top of being able to handle CI situations and partial commits nicely. By default, it will only run on files modified since the last commit.

The minimum required version is Python 3.6.

See https://pre-commit.com/ for more information.

Note: Java and Objective-C++ files won't be formatted by this hook until pocc/pre-commit-hooks#25 is merged and a release of the clang-format pre-commit hooks is tagged.

This closes #46224.

This replaces our custom Git hook solution for pre-commit,
a framework for pre-commit hooks written in Python.

It should have better Windows support than our existing implementation,
on top of being able to handle CI situations and partial commits nicely.
By default, it will only run on files modified since the last commit.

The minimum required version is Python 3.6.

See https://pre-commit.com/ for more information.
@Calinou Calinou added cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:buildsystem labels Feb 19, 2021
@Calinou Calinou added this to the 4.0 milestone Feb 19, 2021
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

On macOS everything except makerst hook is working (fixed by changing command to python3, on macOS python points to 2.7.16).

name: Check XML class reference syntax and validity
language: system
pass_filenames: false
entry: python doc/tools/makerst.py doc/classes modules --dry-run
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
entry: python doc/tools/makerst.py doc/classes modules --dry-run
entry: python3 doc/tools/makerst.py doc/classes modules --dry-run

Copy link
Member

Choose a reason for hiding this comment

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

Might need testing on Windows as typically the installed binary there is python.exe for Python 3.
And apparently @BastiaanOlij has a setup where it's py.exe 🙀

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, Windows seems to use python.exe, at least when it's installed from www.python.org. And probably won't run the script directly if python is removed from the command (which is also a valid option on macOS). I'll check whether it works on Windows at all a bit later.

Copy link
Member

Choose a reason for hiding this comment

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

I'll check whether it works on Windows at all a bit later.

Seems to work fine on Windows.

probably won't run the script directly if python is removed

Windows shell is capable of running python scripts directly, but pre-commit is not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might need testing on Windows as typically the installed binary there is python.exe for Python 3.
And apparently @BastiaanOlij has a setup where it's py.exe 🙀

Yeah but the thing is this stupid STUB thing that Windows does so if you try and run python, instead of it running your installed python, it will run this little program that redirects you to MSVC and tells it to install its version of Python. Like WTF..

py.exe worked because that didn't have a stub.

Can't remember the suggestion but someone told me how to turn that off and now the python executable in my python install works

Copy link
Contributor

Choose a reason for hiding this comment

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

pre-commit has a config option default_language_version. Setting it to python3 is enough to use Python 3 even with python executable name

minimal reproducible example:

default_language_version:
  python: python3

repos:
-   repo: local
    hooks:
    -   id: check-version
        name: check python version
        language: python
        entry: python --version
        files: ".*"
$ pre-commit run --verbose
>>> Python 3.10.6

output with python: python2:

>>> Python 2.7.17

@BastiaanOlij
Copy link
Contributor

Worked fine for me in windows. Fixed up a bunch of format things I mangled. Only test it failed was adding a newline at the end of the file.

@aaronfranke
Copy link
Member

I think it would be a good idea if the pre-commit hooks were added without removing the Bash formatting scripts. This would allow the pre-commit hooks to be more broadly tested with little risk.

Additionally, if the CI was using the pre-commit hooks, what we could do is revert #46050 such that the Bash formatting scripts are reserved for occasional manual use and would do more than the typical scripts (like what we currently do with clang-tidy).

@aaronfranke
Copy link
Member

What's the reason that this is still a draft? Also, my comment above still applies, we don't need to remove the Bash scripts in order to add the pre-commit Git hooks.

@Calinou
Copy link
Member Author

Calinou commented Sep 30, 2022

What's the reason that this is still a draft? Also, my comment above still applies, we don't need to remove the Bash scripts in order to add the pre-commit Git hooks.

Hooks have received many updates since I opened this PR, so this needs to be salvaged. I don't have time to do it currently.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 10, 2023
@Calinou
Copy link
Member Author

Calinou commented Jan 9, 2024

@Calinou Calinou closed this Jan 9, 2024
@Calinou Calinou added archived and removed salvageable cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Jan 9, 2024
@Calinou Calinou removed this from the 4.x milestone Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve CI experience for contributors
7 participants