Skip to content

SG-45189 Bypass Windows MAX_PATH in tk-core filesystem copy/delete - #1132

Open
stevelittlefish wants to merge 3 commits into
masterfrom
SG-45189-bypass-maxpath-in-filesystem-copy
Open

stevelittlefish wants to merge 3 commits into
masterfrom
SG-45189-bypass-maxpath-in-filesystem-copy

Conversation

@stevelittlefish

Copy link
Copy Markdown
Contributor

Problem

On Windows, the advanced project setup wizard fails part-way through with OSError / FileNotFoundError when copying bundles whose files are nested deeper than the 260-character MAX_PATH limit (e.g. tk-framework-desktopserver's bundled per-platform Python, and tk-framework-alias's Sphinx docs). The failing call chain is core_localize.do_localizedescriptor.clone_cachefilesystem.copy_foldershutil.copy.

This is the follow-up flagged in SG-44086 (PR #1118), which fixed the same limit for the unzip/download path only. As Joel noted there, "other tk-core filesystem calls (os.path.exists, copies, git descriptors) can still hit MAX_PATH elsewhere." This PR extends the same fix to the copy/delete path.

Fix

The _to_extended_path() helper (previously private to util/zip.py) is moved to util/filesystem.py and given a force flag. On Windows it prepends the \?\ extended-length prefix (\?\UNC\ for network paths); off Windows and for ineligible/short paths it is a no-op. This requires no LongPathsEnabled registry key or app manifest — it works on any Windows machine.

It is applied across the deep-traversal helpers:

  • copy_folder – extends each path at the point of use. Directory creation (os.mkdir) is force-extended because Windows caps a new directory at MAX_PATH - 12 (248), not 260.
  • safe_delete_folder / compute_folder_size – force-extend the root so shutil.rmtree / os.walk internal descent inherits the prefix.
  • move_folder – extends each deep file before os.stat/os.chmod/os.remove.
  • copy_file, touch_file, ensure_folder_exists – extend their file/dir paths (force for the dir-creating makedirs).

copy_folder's returned file list is unchanged (plain paths); callers re-extend as needed.

util/zip.py now imports the helper from filesystem (no behaviour change).

Tests

  • Moved TestToExtendedPath into tests/util_tests/test_filesystem.py and added force coverage.
  • Added TestCopyFolderLongPaths — a Windows-only integration test that builds a short-rooted tree with a leaf > 260 chars and verifies copy_folder + safe_delete_folder succeed (the exact SG-45189 scenario).
  • Verified as a mutation check: with the \?\ handling disabled, the new long-path tests fail (integration test errors, prefix-assertion tests fail) while all short-path tests still pass — confirming the tests genuinely exercise the fix.
  • util_tests package: 223 passed / 6 skipped on Windows (Python 3).

🤖 Generated with Claude Code

Move _to_extended_path from zip.py to filesystem.py (add a `force` flag)
and apply the \?\ extended-length prefix across the deep-traversal
helpers: copy_folder, move_folder, safe_delete_folder, compute_folder_size,
copy_file, touch_file and ensure_folder_exists. Directory creation is
force-extended because Windows caps new directories at MAX_PATH-12 (248).

Extends the SG-44086 unzip fix to the localize/clone_cache copy path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@stevelittlefish
stevelittlefish requested a review from a team September 10, 2026 14:21
@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.05882% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.11%. Comparing base (889c1cd) to head (3ed54c0).

Files with missing lines Patch % Lines
python/tank/util/filesystem.py 96.96% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1132      +/-   ##
==========================================
+ Coverage   80.09%   80.11%   +0.01%     
==========================================
  Files         203      203              
  Lines       19537    19543       +6     
==========================================
+ Hits        15649    15657       +8     
+ Misses       3888     3886       -2     
Flag Coverage Δ
Linux 79.53% <55.88%> (-0.01%) ⬇️
Python-3.10 79.93% <97.05%> (+0.02%) ⬆️
Python-3.11 79.81% <97.05%> (+<0.01%) ⬆️
Python-3.13 79.81% <97.05%> (+<0.01%) ⬆️
Python-3.9 79.88% <97.05%> (+<0.01%) ⬆️
Windows 79.57% <94.11%> (+0.01%) ⬆️
macOS 79.50% <55.88%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

stevelittlefish and others added 2 commits September 10, 2026 15:59
The extended-length prefix disables the automatic '.'/'..'/'/' normalization
Windows performs on normal paths, so a path containing os.pardir (e.g. the
os.path.join(dir, os.pardir) handed to safe_delete_folder) became an invalid
"...\config_install_backup\.." extended path that no longer resolved to its
parent - the folder was never deleted. This broke bootstrap_tests.test_backups
on Windows.

_to_extended_path now normpath-normalizes internally before prefixing (without
resolving relative paths against the cwd), and skips normalization for
already-extended paths. Adds a regression test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ruff flagged the "\?\ " backslash-space as a deprecated escape sequence in
the new regression test's docstring; double it so the string is valid.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The long-path handling is applied consistently at the relevant filesystem call sites and is backed by targeted Windows-only regression/integration tests for the reported failure mode.

Pull request overview

This PR extends tk-core’s Windows long-path support (\?\ prefix) beyond unzip/download to filesystem copy/delete operations, addressing MAX_PATH failures during deep bundle localization (SG-45189).

Changes:

  • Moved _to_extended_path() into tank.util.filesystem and added a force option for APIs that walk internally (e.g., os.walk, shutil.rmtree).
  • Applied extended-path handling across deep-traversal filesystem helpers (copy_folder, move_folder, safe_delete_folder, compute_folder_size, plus targeted updates to copy_file, touch_file, ensure_folder_exists).
  • Relocated and expanded tests into tests/util_tests/test_filesystem.py, including a Windows-only integration test that reproduces the >260-char leaf scenario.
File summaries
File Description
tests/util_tests/test_zip.py Removes now-relocated _to_extended_path unit tests from zip tests.
tests/util_tests/test_filesystem.py Adds _to_extended_path(force=...) coverage and a Windows-only long-path copy/delete integration test.
python/tank/util/zip.py Switches unzip path prefixing to use filesystem._to_extended_path (no behavior change intended).
python/tank/util/filesystem.py Introduces _to_extended_path(force=...) and applies it to copy/delete/deep traversal helpers to bypass MAX_PATH.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@carlos-villavicencio-adsk carlos-villavicencio-adsk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for this one!

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.

3 participants