Fix resource paths under Windows extended-length paths - #4007
SulimanAbdulrazzaq wants to merge 4 commits into
Conversation
Windows uses a path with the \\?\ prefix verbatim, so a "/" inside it is an invalid name (WinError 123). Dash joined "/"-separated package paths, and a nested favicon's asset path, onto a directory and passed the result to os.stat, so every index render failed with a 500 when Dash was imported from such a path, as in JupyterLab Desktop's site-packages. Split those paths on "/" before joining them in _relative_url_path, _get_worker_url and the favicon mtime lookup. Fixes plotly#3002
T4rk1n
left a comment
T4rk1n
left a comment
There was a problem hiding this comment.
Fix is right for #3002. Splitting on / before os.path.join is also what pkgutil.get_data does inside, so the stat paths now match the serve path exactly. Not blocking. There are two notes inline: the favicon part only gets index past the 500 (I think the asset itself still 404s), and the new test won't run in CI because the Windows job doesn't run pytest.
The Windows extended-length path test in tests/unit/test_resources.py is skipped on Linux, and the build-windows job only built the project, so nothing ran it. Add a pytest step for that file to build-windows. Assets are still not served when the assets folder itself is under an extended-length path (Flask's static route joins with '/'), so drop the favicon wording from the changelog entry.
dash/dcc, dash/html and dash/dash_table are build outputs, and build-windows only builds the renderer and html components into their own folders, so 'import dash' fails there. Download the dash-components artifact from the build job before running the tests, as lint-unit does.
|
@T4rk1n both of your notes are addressed: df45f7e trims the CHANGELOG entry so it no longer claims nested assets are served, and df45f7e and da8ef9d add a pytest step for |
|
2 participants
Fixes #3002
On Windows, Python treats a path with the
\\?\prefix verbatim, so a/inside it is an invalid name. JupyterLab Desktop's bundled Python loads packages from such a path. There, every page load returned a 500 error:(Message from a local reproduction on current
dev, withdashimported from a\\?\path.)_relative_url_pathand_get_worker_urljoin a/-separated package path onto the package directory and pass the result toos.stat. The favicon mtime lookup does the same with the asset path whenfavicon.icosits in an assets subfolder, and that one fails whenever the app's assets folder is under a\\?\path. All three now split on/before joining, so the path uses the OS separator. Paths without the prefix, and all non-Windows paths, give the same result as before.Contributor Checklist
/-separated package paths before joining them in_relative_url_pathand_get_worker_urlindextests/unit/test_resources.py::test_index_with_windows_extended_length_pathspointsdash.__file__and the assets folder at\\?\paths, putsfavicon.icoinassets/icons/, and checks thatGET /returns 200 with the polyfill, stream-worker and favicon URLs in the page. It fails ondevwith theWinError 123above. Reverting any one of the three changes on its own makes it fail again. It only runs on Windows, since the prefix does not exist elsewhere.Local runs (Windows 11, Python 3.11):
tests/unit/test_resources.pypasses (10 passed). The fulltests/unitgives 701 passed and 13 failed. The same 13 fail on untoucheddevon this machine (700 passed), so none of them comes from this change. black and flake8 pass.pylint dash/dash.pyand thelint.pylint-testscommand both score 10.00/10. The fulllint.pylint-dashrun reports the same five errors in untouched files (dash/testing/browser.py,dash/_shared_storage/local.py) ondevand on this branch.optionals
CHANGELOG.md