Make pre-commit work on Windows#2327
Conversation
github-actions
Bot
added
CI/CD
|
There was a problem hiding this comment.
Touching this file seems unnecessary, why does pre-commit care on Windows?
There was a problem hiding this comment.
Because mypy is now checking this on Windows, where things like os.RTLD_NOW don't exist.
mypy doesn't/can't know that this is only imported on Linux. It does have special support for parsing if sys.platform == "linux" (which is why this works), but it can't follow that between modules. (And we use slightly different logic there anyway).
IME, this stuff is necessary to make type-checking complete, annoying as it is.
| windll = getattr(ctypes, "windll", None) | ||
| if windll is None: | ||
| raise RuntimeError("ctypes.windll is required on Windows") |
There was a problem hiding this comment.
This seems... dumb to me TBH. We guaranteed this file is only executed on Windows, no? How is it possible that windll is not available on Windows...?
There was a problem hiding this comment.
In the type information, it's specified as | None, because it doesn't exist in all places. mypy is a static analyzer, so it knows nothing about the runtime state of the program.
There was a problem hiding this comment.
I've refactored this to use sys.platform == "win32" (which mypy recognizes) rather than IS_WINDOWS (which it can't). It makes things simpler. The linux module changes are still required but are less dynamic and "dumb".
There was a problem hiding this comment.
I have simplified the type-checking somewhat, but this specific thing is required. You have to imagine "what happens if we load this code on any platform" and accommodate all cases.
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
Some gpt-5.6-sol generated comments.
It figured out that lychee actually runs on Windows, but only with a suitable bash:
https://github.com/NVIDIA/cuda-python/actions/runs/28974154791/job/85977090993#step:5:1
mdboom
force-pushed
the
pre-commit-windows
branch
from
3e2b94f to
25525cd
Compare
| - id: debug-statements | ||
| - id: end-of-file-fixer | ||
| exclude: &gen_exclude '^(?:cuda_python/README\.md|cuda_bindings/cuda/bindings/.*\.in?|cuda_bindings/docs/source/module/.*\.rst?|.*\.pyi)$' | ||
| exclude: &gen_exclude '^(?:cuda_python/README\.md|(?:^|/)CLAUDE\.md|(?:^|/)\.git_archival\.txt|cuda_bindings/cuda/bindings/.*\.in?|cuda_bindings/docs/source/module/.*\.rst?|.*\.pyi)$' |
There was a problem hiding this comment.
This still misses eight nested symlink placeholders. Because the complete expression is anchored with ^, the inner (?:^|/) alternatives are evaluated at offset 0 and cannot match paths such as cuda_core/CLAUDE.md or cuda_pathfinder/.git_archival.txt. In a core.symlinks=false checkout, end-of-file-fixer rewrote all eight nested placeholders. Could we use something like (?:.*/)?(?:CLAUDE\.md|\.git_archival\.txt) inside the outer anchored alternation (or list the tracked paths explicitly), and ideally exercise a core.symlinks=false checkout in CI?
| # `ctypes.WinDLL` exists on Windows at runtime. The ignore is only for | ||
| # Linux mypy runs, where the platform stubs do not define that attribute. | ||
| loader_cls: Callable[[str], ctypes.CDLL] = ctypes.WinDLL # type: ignore[attr-defined] | ||
| if sys.platform == "win32": |
There was a problem hiding this comment.
The direct sys.platform check is useful here because mypy understands the platform narrowing, but removing IS_WINDOWS leaves the existing tests patching a deleted attribute (test_utils_driver_info.py:49 and :96), which is why every current Windows test job fails with AttributeError.
I asked my agent about teaching tests to keep IS_WINDOWS and sys.platform synchronized, but it said that's brittle. As a safer alternative, could we make the selected implementation the injectable seam?
if sys.platform == "win32":
_DRIVER_LIB_LOADER: Callable[[str], ctypes.CDLL] = ctypes.WinDLL
else:
_DRIVER_LIB_LOADER = ctypes.CDLLand then:
driver_lib = _DRIVER_LIB_LOADER(loaded_cuda.abs_path)Unit tests can patch _DRIVER_LIB_LOADER directly, while a small native-platform test can assert that it is ctypes.WinDLL when IS_WINDOWS and ctypes.CDLL otherwise. This keeps direct sys.platform confined to the mypy-recognized selection boundary, avoids globally pretending the process is another OS, and lets Linux/Windows CI verify the real selection.
| 1. Run `pre-commit` it in Git Bash, rather directly in PowerShell or cmd | ||
|
|
||
| 2. Skip it by setting the environment variable `SKIP` to `lychee`. This would | ||
| be `$env:SKIP = "lychee"` in PowerShell or `SKIP=lychee` in cmd. |
There was a problem hiding this comment.
Command Prompt requires set SKIP=lychee (plain SKIP=lychee is not cmd syntax).
3 participants
Our pre-commit config didn't work on Windows, which makes things harder than it already is for Windows development.
The
.pyigeneration step would create line ending churn. This adds a wrapper around our use ofstubgen-pyxso normalize line endings. (Maybe someday that can be an upstream contribution tostubgen-pyxbut this is good enough for now).lycheedoesn't run on Windows. Unfortunately, there is no way to config a task to only run on certain platforms. The best I or my agent was able to come up with was to set anenvvarto skip it.Doing an end-of-file check on a symlink would fail (they are files without newlines at the end on Windows). This updates the pre-commit config to ignore all symlinks for that task.
mypyhas never run on Windows before, and it found a few typing bugs, fixed here.Lastly, this adds pre-commit CI on Windows so these fixes will stay in place. (And having Windows mypy checking seems useful).