Skip to content

Conversation

jack60612
Copy link
Contributor

@jack60612 jack60612 commented Jul 7, 2025

Purpose:

Allows these files to run on windows

Current Behavior:

Pre-commit broken on windows

New Behavior:

Pre-commit working on windows

Testing Notes:

Change python3 to python in shebang in all files

@altendky altendky added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Exclude_Notes Use this label if the changes in the PR should be excluded from the release notes labels Jul 7, 2025
@jack60612 jack60612 changed the title Fix pre-commit on windows Change shebangs from python3 to python Jul 7, 2025
@altendky altendky self-requested a review July 7, 2025 23:27
@jack60612 jack60612 marked this pull request as ready for review July 8, 2025 00:03
@jack60612 jack60612 requested a review from a team as a code owner July 8, 2025 00:03
@jack60612
Copy link
Contributor Author

need to test on my macbook.

@jack60612
Copy link
Contributor Author

still works on my macbook

@jack60612 jack60612 requested a review from arvidn July 8, 2025 00:12
Copy link
Contributor

@altendky altendky left a comment

Choose a reason for hiding this comment

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

this might be a reasonable solution. i'm trying to understand the situation a bit more still.

@altendky
Copy link
Contributor

to document, the hassle here is that we have direct shebang usage on linux/macos and handling the variety there. we then have windows configurations allowing for associating .py files as executables and processing them via the py launcher (ftype, assoc, and PATHEXT are adjacent things) which does it's own processing of the shebang. it handles python3 fine because it doesn't treat it as a file name but rather as a python version specifier. it then goes and finds a python version that satisfies the requested version and is fine. but, pre-commit also does shebang handling and does not do python version aware parsing of the shebang, of course, and so it fails on python3 since the executables are python.exe. some devs have addressed this with a python3 -> python alias in powershell. useful to mention, but not exactly a general solution. pre-commit does have some PATHEXT awareness, though it's not entirely clear it works and it specifically does would not apply to ./ commands. and removing ./ doesn't work on not-windows anyways.

@jack60612 jack60612 requested a review from altendky August 11, 2025 06:42
Copy link
Contributor

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

@github-actions github-actions bot added the stale-pr Flagged as stale and in need of manual review label Sep 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Exclude_Notes Use this label if the changes in the PR should be excluded from the release notes stale-pr Flagged as stale and in need of manual review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants