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

Fix issue with pre-commit action on Windows #1844

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

CAM-Gerlach
Copy link
Contributor

Fixes #1843

Feature Summary and Justification

Test, confirm and (for now) work around/fix the issue with the pre-commit action on Windows identified in #1843 .

I'll be testing a few things, so I'll leave this as draft for now.

@CAM-Gerlach CAM-Gerlach force-pushed the fix-windows-lint branch 6 times, most recently from 28f19e1 to 565690f Compare February 2, 2022 04:06
@CAM-Gerlach
Copy link
Contributor Author

@LilSpazJoekp After a bunch of testing, I've pinned (heh) down the issue. Its not with windows-2022 after all, but rather indeed with the dependency install/upgrade command—at least, in a way. What happens is pip is upgraded to 22.0.2 from 21.3.1 (the last 21.x version), which is resulting in a hard crash of the pre-commit action on both windows-2022 and windows-2019 when it tries to call pip (presumably to install/update pre-comit); this has no apparent effect on the other platforms. If I pin the pip version installed for the lint jobs to 21.x, everything works (for now).

What I highly suggest you do is report this upstream to pre-commit/action so this can get fixed, since the workaround will not be viable forever without compromises. Thanks!

@CAM-Gerlach CAM-Gerlach marked this pull request as ready for review February 2, 2022 04:19
@LilSpazJoekp
Copy link
Member

Thanks for figuring this out and pinning the issue down 😉.

The problem with reporting the issue is that https://github.com/pre-commit/action is deprecated. I'm not sure this will get fixed.

@LilSpazJoekp
Copy link
Member

Could this be a problem pre-commit itself? I don't have a windows machine to test this.

.github/workflows/ci.yml Show resolved Hide resolved
@CAM-Gerlach
Copy link
Contributor Author

The problem with reporting the issue is that https://github.com/pre-commit/action is deprecated. I'm not sure this will get fixed.

It is in maintenance mode only, because the author is focusing future development on his own pre-commit.ci service instead, but I presume "maintenance" includes fixing the action being broken on one of the three GitHub Actions platforms (which is one of the many use cases that pre-commit.ci cannot replace the action for, by design).

Could this be a problem pre-commit itself? I don't have a windows machine to test this.

Nope, sorry—I meant to mention this above, but it seems I neglected to. The bug occurs on the pip install step of the action, before pre-commit ever runs. Furthermore, I ran pre-commit run --all-files locally on my Windows machine with pip 22.0.2 and the same pre-commit version as the CI, and everything worked fine.

@LilSpazJoekp LilSpazJoekp merged commit 849d2ed into praw-dev:master Feb 2, 2022
@LilSpazJoekp
Copy link
Member

Thanks again for figuring this out! 🎇

@LilSpazJoekp
Copy link
Member

@CAM-Gerlach
Copy link
Contributor Author

@LilSpazJoekp Well, clearly a normal pip install works fine as it was executed on the line before running the action, so it has something to do with how he's invoking it or how he's getting the Python executable, or an out of date dep he's using, and if this was really a widespread GitHub problem, there would probably be more than zero relevant Google hits for it. I have no idea where to start in terms of reporting this; since it could be a number of different places in the stack, and unsurprisingly given the maintainer involved, we've zero help from upstream.

In any case, the simplest long-term alternative unless things are resolved would probably be to just drop the action and run pre-commit directly instead; you'd loose out of caching and a few niceties, but not the end of the world.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI pre-commit failure
2 participants