Repository navigation
Fix Windows script path normalization - #874
Conversation
craigbarratt
left a comment
craigbarratt
left a comment
There was a problem hiding this comment.
Thanks for fixing this! The change to os.path.relpath(...).replace(os.sep, "/") looks right: it gives the same result as before on POSIX, and on Windows it fixes the package/module name matching in glob_read_files. Most of my inline comments are about the test, which currently doesn't exercise the separator replacement.
A related Windows issue you could fix here or in a follow-up: the watchdog handler matches requirements files with "/" + REQUIREMENTS_FILE (around line 223 of __init__.py). That never matches a backslash path on Windows, so editing pyscript\requirements.txt doesn't trigger a reload. Comparing os.path.basename(path) == REQUIREMENTS_FILE would work on any platform.
Also, this now has a merge conflict in tests/test_init.py (master added a restore_state test and changed the imports nearby), so it will need a rebase onto current master.
|
|
||
| def _relative_script_path(path: str, root: str) -> str: | ||
| """Return a slash-separated path relative to the pyscript root.""" | ||
| return os.path.relpath(path, root).replace(os.sep, "/") |
There was a problem hiding this comment.
A small difference from the old prefix strip: on Windows, os.path.relpath raises ValueError if the two paths are on different drives (e.g. a mapped drive vs. a UNC path for the config dir). Nothing catches it here, so it would abort load_scripts entirely, whereas the old code just produced an odd name. This is probably rare given both paths come from hass.config.path, but it may be worth a try/except ValueError that logs and skips the file.
| rel_path = rel_path[len(pyscript_dir) :] | ||
| if rel_path.startswith("/"): | ||
| rel_path = rel_path[1:] | ||
| rel_path = _relative_script_path(this_path, pyscript_dir) |
There was a problem hiding this comment.
Optional suggestion: the code below still assumes / in several places (find("/#"), endswith("/__init__")), and so does global_ctx.module_import (f"{path}/{module_path}.py", path.find("/")). Normalizing once where the paths come from (e.g. pathlib.PurePath(this_path).relative_to(pyscript_dir).as_posix()) would keep every later use portable, so new code doesn't have to remember to normalize. What you have is fine for this fix, though.
| return await asyncio.wait_for(notify_q.get(), timeout=4) | ||
|
|
||
|
|
||
| def test_relative_script_path_normalizes_windows_paths(monkeypatch): |
There was a problem hiding this comment.
This only covers the helper. It would be good to also test glob_read_files with Windows-style paths, so that the resulting context names (file.X, apps.X, the /__init__ stripping, skipping # paths) are checked too. Otherwise a later change to how rel_path is used could bring back the Windows bug without any test failing.
| monkeypatch.setattr(pyscript_component.os.path, "relpath", ntpath.relpath) | ||
| monkeypatch.setattr(pyscript_component.os, "sep", "\\") |
There was a problem hiding this comment.
These patch os.path.relpath and os.sep for the whole process (they're the shared os/posixpath modules), so any other code running during the test, such as leftover executor threads from earlier hass fixtures, would see Windows behavior. Testing ntpath.relpath(...).replace(ntpath.sep, "/") directly, or having the helper take the path module as a parameter, would avoid changing global state.
|
|
||
| assert ( | ||
| pyscript_component._relative_script_path( # pylint: disable=protected-access | ||
| r"C:\config\pyscript\hello.py", r"C:\config\pyscript" |
There was a problem hiding this comment.
This case passes even without the .replace(os.sep, "/"), because a top-level file has no separator in its relative path. Could you add a nested case that actually exercises the fix, e.g.
r"C:\config\pyscript\apps\foo\__init__.py" -> "apps/foo/__init__.py"That way the test fails if the separator replacement is ever dropped.
147458b to
06dc3e4
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the PR! |
Since #874 relative script paths come from os.path.relpath, so the mocked "/hello.py" (outside the pyscript folder) loaded under a garbled name. Use a path under hass.config.path("pyscript"), as test_init.py does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On Windows,
load_scriptspreviously produced backslashes in module names. Normalize relative paths to slash separators for file, apps, modules, and scripts contexts.Follow-up f42d07d rebases onto current master, replaces process-wide path monkeypatching with an injected path module, adds a nested Windows case and a loader-context regression, skips paths when
relpathraisesValueError, and matches requirements-file events by basename. On Linux with Python 3.14,tests/test_init.pypassed all 14 tests; Ruff check and format andgit diff --checkpassed. The full pytest run had 182 passes and one failure:test_time_active_hold_off_send_lastreturned the earlier event instead of the last one. The same test failed identically on unmodified current base in this environment. Windows-style paths are simulated withntpath; no native Windows Home Assistant integration run is claimed. Earlier pylint and mypy results apply to the prior head only.