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

[PSA for PR authors] Update your pre-commit hooks to use the newly merged pre-commit framework #88901

Closed
akien-mga opened this issue Feb 27, 2024 · 10 comments

Comments

@akien-mga
Copy link
Member

akien-mga commented Feb 27, 2024

We just replaced our hand-written shell scripts for pre-commit hooks with a set of hooks defined using the pre-commit Python framework:

Previous hooks should still work if you copied the scripts manually from misc/hooks/, but if you used to symlink them, they will be broken as of #88866 so you should remove them from .git/hooks/.

To use the new system, remove old hooks from .git/hooks/, and apply the new hooks with:

pip install pre-commit
pre-commit install

That's all, it should then automatically download dependencies (currently clang-format and black) the first time you try to commit, or when running pre-commit run manually.

If you want to apply the hooks to all your local changes before committing, you can use pre-commit run. You can also apply the hooks to all files in the repository regardless of their changed status by adding the -a option (pre-commit run -a).

The main workflow difference with the previous set of hooks is that if some changes are needed to pass the checks, they will be made automatically in place (at least for clang-format and black currently). Let's see if that workflow is fine, or if we need to figure out ways to bring back some interactivity.

@luevano
Copy link
Contributor

luevano commented Mar 6, 2024

We should also update the godot docs, as they point to the old way: https://docs.godotengine.org/en/latest/contributing/development/code_style_guidelines.html#pre-commit-hook and https://docs.godotengine.org/en/latest/contributing/development/code_style_guidelines.html#id3; when opening a PR it also points to one of these URLs

@roookeee
Copy link

roookeee commented Mar 24, 2024

I am getting the following errors about encoding on windows when running these:

  File "C:\Users\rookee\scoop\apps\python\current\Lib\encodings\cp1252.py", line 23, in decode
    return codecs.charmap_decode(input,self.errors,decoding_table)[0]
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
UnicodeDecodeError: 'charmap' codec can't decode byte 0x9d in position 5068: character maps to <undefined>

It seems to read files in cp1252 instead of utf8 which gives me this garbled mess when running pre-commit run -a:
img

Any ideaa how to solve this?

@AThousandShips
Copy link
Member

AThousandShips commented Mar 24, 2024

Might be either something wrong with your specific file, if you added one you might have accidentally added an encoding to it, check your editor, otherwise this is likely a configuration error in your terminal, are you running from powershell or cmd?

@roookeee
Copy link

I did not touch these files at all. I am running from powershell, I think it's reading the files in the standard platform encoding (which is cp1252 on windows if I recall):

Traceback (most recent call last):
  File "D:\dev\godot\misc\scripts\copyright_headers.py", line 92, in <module>
    line = fileread.readline()
           ^^^^^^^^^^^^^^^^^^^
  File "C:\Users\rookee\scoop\apps\python\current\Lib\encodings\cp1252.py", line 23, in decode
    return codecs.charmap_decode(input,self.errors,decoding_table)[0]
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

readline is not passed any encoding, so it makes sense its trying to use the platform encoding (I am not proficient in python though).

@AThousandShips
Copy link
Member

AThousandShips commented Mar 24, 2024

Assuming you're working on #89826, can you check that the new file you added is encoded in utf8? Just to double check that isn't throwing it off somehow

I don't know how to alter the encoding in python though, there is an encoding="utf-8" directive, this might be missing from our files, I'll take a look

Try adding that line to the top of the copyright_headers.py file

This is probably just missing from here:

with open(fname.strip(), "r") as fileread:

Try changing it to include the encoding directive and see

@roookeee
Copy link

I see you came to the same conclusion as me, that fixed the encoding issues but I am getting these changes added by copyright_headers now:
img
This doesn't look like an encoding issue and I manually checked said file: its UTF-8 without BOM.

Will dig deeper later, the encoding = "utf-8" directive should definitely be added in copyright_header.py

@AThousandShips
Copy link
Member

AThousandShips commented Mar 24, 2024

Could be that you changing that file broke some encoding in it so it has that error now, unsure, try resetting all the rest of the files and run it again so it hasn't accidentally broken those files when you ran it before

I tried fixing the encoding part and it works fine for me, though it also didn't break things when I ran it without that addition

Edit: Installed the pre-commit hooks properly now and it works fine with or without these changes

@roookeee
Copy link

roookeee commented Mar 24, 2024

I can confirm now that adding encoding="utf-8" at the specified location fixes my issues, you are probably right that I saved copyright_header.py in a wrong encoding / with a BOM - I re-added it from a fresh state and a proper editor, that solved all issues.
I rerean without encoding="utf-8" but this yields the errors I outlined above.

So I guess its save to say that encoding="utf-8" is needed, at least for some users on Windows (as is done in the later part of copyright_header.py anyways).

@AThousandShips
Copy link
Member

Now ran into this myself, unsure why it didn't happen before, will look at a fix while I'm working on other code

@aitorciki
Copy link
Contributor

Adding here for potential future reference (maybe worth adding a note in the documentation?):

When using msys2 on Windows, directly installing pre-commit inside msys2 (either as pre-packaged or using pip) won't work (see pre-commit/pre-commit#3234).

The workaround I've found is installing pre-commit under Windows and invoking that binary inside msys2. In a nutshell:

  • on Windows: pip install pre-commit (having installed Python on Windows first, of course)
  • on msys2 add alias pre-commit='/c/Users/foo/AppData/Local/Programs/Python/Python312/Scripts/pre-commit.exe' (update to its correct path for each setup) to ~/.bash_profile or similar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

5 participants