MagicBeanDev/Longshot
MagicBeanDev/Longshot
Watch
24
30
Fork
You've already forked Longshot
 43
Code Issues 62 Pull requests 127 Projects Releases 8 Packages Wiki Activity Actions 312

fix(setup): the setup copies the Builder from a zip unpacked deeper than MAX_PATH #151

Open
d4nkn3ss wants to merge 3 commits from d4nkn3ss/Longshot:fix/setup-long-paths into main
pull from: d4nkn3ss/Longshot:fix/setup-long-paths
merge into: MagicBeanDev:main
Conversation 4 Commits 3 Files changed 9 +184 -43
d4nkn3ss commented 2026-11-13 07:30:02 +00:00
Contributor

Why

The Windows setup can't install the Builder from a zip unpacked a little deep: it stops with "Could not copy the Longshot Builder from … (unpack the whole zip again)", and unpacking again doesn't help. The zip's deepest file is 163 characters below its folder, so any folder of about 100 characters or more is enough. Since #171 and #181 the setup copies the Builder only for --cli (RecompLauncher, and the portable command line in docs/windows-portable-builder.md), and that's where it still applies.

What changed

  • setup: copy_part_tree() in builder/setup/setup.c works on \\?\ paths (\\?\UNC\ for a share), which Windows' file calls take up to 32767 characters, built once per call in heap buffers (builder/setup/long_path.h); a path already in that form is kept. (dankman42)
  • setup: rmtree uses the same \\?\ paths, so it removes anything the copy creates, and no longer stops the setup with "too long". (HumanBean)
  • setup: a copy into itself is refused: a destination equal to or under the source, compared in the \\?\ form without case and without a trailing \ (…\LongshotBuilder2 isn't under …\LongshotBuilder). Both walks stop at 256 levels. (HumanBean)
  • setup: the portable home is now LongshotBuilder-data, beside the zip's LongshotBuilder folder, so --portable from an unpacked zip no longer copies that folder into itself. A zip folder an earlier run marked as a home stops the setup with a message to unpack into a new folder. docs/windows-portable-builder.md says so. (HumanBean)
  • tests: test_portable_setup_transaction.py copies a tree more than 300 characters deep and removes it whole, refuses a self-copy in four spellings, and copies a sibling with the same prefix. test_setup_tool_merge.py, test_portable_setup_start.py, test_portable_builder_home.py and portable_setup_home_test.c follow the new home name.

Flags and settings

None

Measurements

main: fbd42024 (2026-11-13); the setup there is the same as nightly-34's
this PR: fix/setup-long-paths @ e9ade335
Hardware: Windows 11, Python 3.12, llvm-mingw clang 23 (the Longshot Builder's)
Scene: n/a (no game run)
Method: nightly-34's zip unpacked under a folder deep enough that its longest file path is 306-310 characters; the zip's own setup and this PR's (built with build-windows.yml's windres and clang commands), each run as LongshotBuilder.exe --portable --cli --version with LONGSHOT_SETUP_BASE set to a scratch folder, and once more for this PR with no base (the portable home beside the zip); each stopped once the copy had finished and the tool downloads began
How to reproduce: unpack a release zip into a folder of 100+ characters and run its LongshotBuilder.exe --portable --cli --version (or any --cli …)

main @ fbd42024 (nightly-34's setup) this PR @ e9ade335
deep folder, scratch base: Builder files copied 328 of 1605, then error Could not copy the Longshot Builder from <folder>\LongshotBuilder (unpack the whole zip again). 1605 of 1605, then the tool downloads start
--portable from the unpacked zip, no base the self-copy HumanBean found (not run here: it fills the disk) home <zip folder>\LongshotBuilder-data, 1605 of 1605 copied; the zip's LongshotBuilder still 1605 files

Testing

Tested: the runs above on Windows 11. The Windows-only setup checks, on Windows: test_portable_setup_transaction.py, test_portable_setup_start.py, test_portable_builder_home.py, plus test_portable_setup_paths.py, test_setup_tool_merge.py, test_setup_runtime.py, test_setup_runtime_paths.py: 7 passed.

Full suite: python -I -B tests/run_all.py on this branch @ e9ade335, Windows 11: 494 passed, 2 failed, 33 skipped: tests/test_check_third_party.py (#80) and tests/test_release.py (#119, #144, #145), both failing on main the same way. HumanBean ran the suite on Linux: no new failures.

Not tested:

  • a share (\\server\...) as the unpack folder;
  • the rest of the install from a deep folder (the tool downloads, the Builder's start): the runs were stopped once the copy had passed;
  • the same folder reached another way: the guard compares spellings only, so a subst or mapped drive, an 8.3 name or a junction isn't caught. This needs a hand-set LONGSHOT_SETUP_BASE, and the depth cap still bounds it;
  • an older setup's portable home named LongshotBuilder: it isn't picked up again.

Needs other testers: None

Screenshots

Nothing on screen

Risk and rollback

The Builder copy, the portable part copy (reuse_part) and rmtree change; the portable home's name changes for new portable installs. Reverting the three commits is enough.

Issues

None

Checklist

For everyone

  • One topic.
  • The title is a Conventional Commit (type(scope): description).
  • Rebased on current main.
  • python -I -B tests/run_all.py run and passing (any failure that also fails on main is named above).
  • Tested in the game, or what wasn't is named under Testing. (The setup only: nothing in the game.)
  • Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests).
  • Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. (An install's outcome, the same on every run.)
  • No game files, dumps, build output or generated files.
  • third_party/ untouched (a new folder with a PROVENANCE.md instead).
  • The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled.

If applicable

  • AI assistance recorded with Assisted-by: / Co-Authored-By: trailers.
  • A game-free test added for the fix, failing on main and passing here.
  • A check under tests/ that reads changed source updated in this PR, not weakened.
  • Docs updated.

🤖 Generated with Claude Code

## Why The Windows setup can't install the Builder from a zip unpacked a little deep: it stops with "Could not copy the Longshot Builder from … (unpack the whole zip again)", and unpacking again doesn't help. The zip's deepest file is 163 characters below its folder, so any folder of about 100 characters or more is enough. Since #171 and #181 the setup copies the Builder only for `--cli` (RecompLauncher, and the portable command line in `docs/windows-portable-builder.md`), and that's where it still applies. ## What changed - setup: `copy_part_tree()` in `builder/setup/setup.c` works on `\\?\` paths (`\\?\UNC\` for a share), which Windows' file calls take up to 32767 characters, built once per call in heap buffers (`builder/setup/long_path.h`); a path already in that form is kept. (dankman42) - setup: `rmtree` uses the same `\\?\` paths, so it removes anything the copy creates, and no longer stops the setup with "too long". (HumanBean) - setup: a copy into itself is refused: a destination equal to or under the source, compared in the `\\?\` form without case and without a trailing `\` (`…\LongshotBuilder2` isn't under `…\LongshotBuilder`). Both walks stop at 256 levels. (HumanBean) - setup: **the portable home is now `LongshotBuilder-data`**, beside the zip's `LongshotBuilder` folder, so `--portable` from an unpacked zip no longer copies that folder into itself. A zip folder an earlier run marked as a home stops the setup with a message to unpack into a new folder. `docs/windows-portable-builder.md` says so. (HumanBean) - tests: `test_portable_setup_transaction.py` copies a tree more than 300 characters deep and removes it whole, refuses a self-copy in four spellings, and copies a sibling with the same prefix. `test_setup_tool_merge.py`, `test_portable_setup_start.py`, `test_portable_builder_home.py` and `portable_setup_home_test.c` follow the new home name. ## Flags and settings None ## Measurements main: fbd42024 (2026-11-13); the setup there is the same as nightly-34's this PR: fix/setup-long-paths @ e9ade335 Hardware: Windows 11, Python 3.12, llvm-mingw clang 23 (the Longshot Builder's) Scene: n/a (no game run) Method: nightly-34's zip unpacked under a folder deep enough that its longest file path is 306-310 characters; the zip's own setup and this PR's (built with `build-windows.yml`'s windres and clang commands), each run as `LongshotBuilder.exe --portable --cli --version` with `LONGSHOT_SETUP_BASE` set to a scratch folder, and once more for this PR with no base (the portable home beside the zip); each stopped once the copy had finished and the tool downloads began How to reproduce: unpack a release zip into a folder of 100+ characters and run its `LongshotBuilder.exe --portable --cli --version` (or any `--cli …`) | | main @ fbd42024 (nightly-34's setup) | this PR @ e9ade335 | |---|---|---| | deep folder, scratch base: Builder files copied | 328 of 1605, then `error Could not copy the Longshot Builder from <folder>\LongshotBuilder (unpack the whole zip again).` | 1605 of 1605, then the tool downloads start | | `--portable` from the unpacked zip, no base | the self-copy HumanBean found (not run here: it fills the disk) | home `<zip folder>\LongshotBuilder-data`, 1605 of 1605 copied; the zip's `LongshotBuilder` still 1605 files | ## Testing **Tested:** the runs above on Windows 11. The Windows-only setup checks, on Windows: `test_portable_setup_transaction.py`, `test_portable_setup_start.py`, `test_portable_builder_home.py`, plus `test_portable_setup_paths.py`, `test_setup_tool_merge.py`, `test_setup_runtime.py`, `test_setup_runtime_paths.py`: 7 passed. **Full suite:** `python -I -B tests/run_all.py` on this branch @ e9ade335, Windows 11: 494 passed, 2 failed, 33 skipped: `tests/test_check_third_party.py` (#80) and `tests/test_release.py` (#119, #144, #145), both failing on `main` the same way. HumanBean ran the suite on Linux: no new failures. **Not tested:** - a share (`\\server\...`) as the unpack folder; - the rest of the install from a deep folder (the tool downloads, the Builder's start): the runs were stopped once the copy had passed; - the same folder reached another way: the guard compares spellings only, so a subst or mapped drive, an 8.3 name or a junction isn't caught. This needs a hand-set `LONGSHOT_SETUP_BASE`, and the depth cap still bounds it; - an older setup's portable home named `LongshotBuilder`: it isn't picked up again. **Needs other testers:** None ## Screenshots Nothing on screen ## Risk and rollback The Builder copy, the portable part copy (`reuse_part`) and `rmtree` change; the portable home's name changes for new portable installs. Reverting the three commits is enough. ## Issues None ## Checklist ### For everyone - [x] One topic. - [x] The title is a Conventional Commit (`type(scope): description`). - [x] Rebased on current `main`. - [x] `python -I -B tests/run_all.py` run and passing (any failure that also fails on `main` is named above). - [x] Tested in the game, or what wasn't is named under Testing. (The setup only: nothing in the game.) - [x] Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests). - [x] Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. (An install's outcome, the same on every run.) - [x] No game files, dumps, build output or generated files. - [x] `third_party/` untouched (a new folder with a `PROVENANCE.md` instead). - [x] The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled. ### If applicable - [x] AI assistance recorded with `Assisted-by:` / `Co-Authored-By:` trailers. - [x] A game-free test added for the fix, failing on `main` and passing here. - [x] A check under `tests/` that reads changed source updated in this PR, not weakened. - [x] Docs updated. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
fix(setup): the setup copies the Builder from a zip unpacked deeper than MAX_PATH
Some checks failed
PR description / Description follows the template (pull_request_target) Successful in 5s
Details
CI / Conventional Commit title (pull_request) Has been cancelled
Details
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled
Details
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled
Details
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled
Details
CI / Workflow lint (actionlint) (pull_request) Has been cancelled
Details
Third-party parts / As their notes say (pull_request) Has been cancelled
Details
Third-party parts / The same as upstream (pull_request) Has been cancelled
Details
56335697f9 
copy_part_tree() joined paths into MAX_PATH buffers, and the builder zip's deepest file is 163 characters below its
folder, so the setup failed with "Could not copy the Longshot Builder ... (unpack the whole zip again)" whenever a file
path passed 260 characters (nightly-34 unpacked under a long folder: 328 of 1605 files copied). It now works on \?\
paths (\?\UNC\ for a share), which Windows' file calls take up to 32767 characters, in heap buffers, and stays one
self-contained function for the source checks that cut it.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
HumanBean requested changes 2026-11-13 16:19:04 +00:00
Dismissed
HumanBean left a comment

The diagnosis is right and the fix is small. copy_part_tree() was the only place where the deep source tree hit MAX_PATH, since the default install under %LOCALAPPDATA% is short. The \\?\ conversion is right for every path that can actually reach the function: g_app comes from GetModuleFileNameW, and the home from hb_home_full(), which calls GetFullPathNameW. Both are absolute, use \, and have ., .., trailing dots and spaces already resolved. The UNC form (\\server\share to \\?\UNC\server\share) is right. A path that already has the prefix is kept, so the recursion's own \\?\ paths pass through unchanged. A relative path behaves as before. Every call on the copy path is the W version and accepts \\?\ (GetFileAttributesW, CopyFileW, CreateDirectoryW, FindFirstFileW/FindNextFileW). There's no SHFileOperation in the setup, and the PR adds no UTF-8 to UTF-16 conversion. Moving the buffers to the heap also keeps the recursion's stack small. tests/test_portable_setup_transaction.py still compiles the function as it stands, and on Windows it calls it with absolute temp paths, so the drive-letter branch runs.

This still matters on main. builder/setup/setup.c and copy_part_tree() are unchanged there since 39647e1a. After #171 and #181, the setup copies the Builder only for --cli: RecompLauncher, and the portable command line in docs/windows-portable-builder.md. Started by hand, it now only points to the launcher. The Longshot Launcher installs the Builder itself and doesn't run this setup. The patch applies to current main without conflicts (offset -23 lines).

Must change

  1. With --portable started from the unpacked zip, the copy now fills the disk instead of failing fast. With no LONGSHOT_SETUP_BASE and no marked folder above it, init_home(1) makes the portable home <setup folder>\LongshotBuilder. That is the same folder as g_app, the zip's own LongshotBuilder\. work() then makes <home>\versions\<tag>.part, inside g_app, and calls copy_part_tree(g_app, g_app\versions\<tag>.part\LongshotBuilder), which copies the folder into itself. The recursion walks versions\<tag>.part\LongshotBuilder over and over, and at each level it copies everything sorted (on exFAT, created) before versions, which is nearly the whole Builder.

    • On main this fails at MAX_PATH after one or two levels with "Could not copy the Longshot Builder". That mode is already broken there.
    • With this PR the limit is 32767 characters, about 750 levels of roughly 42 characters each. So it writes up to about 750 copies of the Builder until the disk is full.
    • Every later portable run then stops in rmtree(install), because home_path() calls fail() ("too long for the Windows setup") on the over-long leftovers. Explorer can't easily delete them either.

    This is exactly the documented portable SSD use (docs/windows-portable-builder.md). The PR's runs avoided it because they set LONGSHOT_SETUP_BASE.

    • Minimum fix: refuse the copy when dst is src or lies under src\ (compare the prefixed full paths case-insensitively, with the trailing \).
    • Better: also keep the portable home out of the zip's builder folder, or skip the home's own entries (versions, downloads, .longshot-portable) in that copy, so portable mode works at all.
    • Add a check to test_portable_setup_transaction.py (a copy into its own subfolder returns 0 quickly).

  2. The "Rebased on current main" box is ticked but false. The branch forks at 39647e1a, and main is about 30 commits ahead. The PR merges cleanly, and nothing it touches changed, so this is about the box only: rebase (and re-run the checks), or untick it.

Should change

  • rmtree() can't remove what the copy can now create. It still builds MAX_PATH paths through home_path(). If a deep copy is stopped or fails (in reuse_part's cleanup, or in a .part left for the next run), cleanup either can't reach the files (standard home) or stops the setup with "too long" (portable home). Give rmtree() the same \\?\ handling, or say in the PR that this is left for later.
  • There's no check of the case this fixes. The transaction test only copies short trees. Add a source tree deeper than 260 characters (Python on Windows can create one with a \\?\ path) and assert the copy returns 1 and the files match. Otherwise a later edit can quietly bring back the MAX_PATH limit.
  • "How to reproduce" says to run the zip's LongshotBuilder.exe. On main, started by hand, it only shows the launcher message. Say LongshotBuilder.exe --cli ... (as the measurements did).
  • Nit: each call allocates 256 KiB, including the call for each file (1605 files). This is harmless, but the two prefixed paths could be built once at the top level and passed down.

I didn't build or run anything (review only). Not checked on hardware: a UNC unpack folder, and exFAT ordering.

The diagnosis is right and the fix is small. `copy_part_tree()` was the only place where the deep source tree hit `MAX_PATH`, since the default install under `%LOCALAPPDATA%` is short. The `\\?\` conversion is right for every path that can actually reach the function: `g_app` comes from `GetModuleFileNameW`, and the home from `hb_home_full()`, which calls `GetFullPathNameW`. Both are absolute, use `\`, and have `.`, `..`, trailing dots and spaces already resolved. The UNC form (`\\server\share` to `\\?\UNC\server\share`) is right. A path that already has the prefix is kept, so the recursion's own `\\?\` paths pass through unchanged. A relative path behaves as before. Every call on the copy path is the W version and accepts `\\?\` (`GetFileAttributesW`, `CopyFileW`, `CreateDirectoryW`, `FindFirstFileW`/`FindNextFileW`). There's no `SHFileOperation` in the setup, and the PR adds no UTF-8 to UTF-16 conversion. Moving the buffers to the heap also keeps the recursion's stack small. `tests/test_portable_setup_transaction.py` still compiles the function as it stands, and on Windows it calls it with absolute temp paths, so the drive-letter branch runs. This still matters on main. `builder/setup/setup.c` and `copy_part_tree()` are unchanged there since 39647e1a. After https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/171 and https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/181, the setup copies the Builder only for `--cli`: RecompLauncher, and the portable command line in `docs/windows-portable-builder.md`. Started by hand, it now only points to the launcher. The Longshot Launcher installs the Builder itself and doesn't run this setup. The patch applies to current main without conflicts (offset -23 lines). ## Must change 1. **With `--portable` started from the unpacked zip, the copy now fills the disk instead of failing fast.** With no `LONGSHOT_SETUP_BASE` and no marked folder above it, `init_home(1)` makes the portable home `<setup folder>\LongshotBuilder`. That is the same folder as `g_app`, the zip's own `LongshotBuilder\`. `work()` then makes `<home>\versions\<tag>.part`, inside `g_app`, and calls `copy_part_tree(g_app, g_app\versions\<tag>.part\LongshotBuilder)`, which copies the folder into itself. The recursion walks `versions\<tag>.part\LongshotBuilder` over and over, and at each level it copies everything sorted (on exFAT, created) before `versions`, which is nearly the whole Builder. - On main this fails at `MAX_PATH` after one or two levels with "Could not copy the Longshot Builder". That mode is already broken there. - With this PR the limit is 32767 characters, about 750 levels of roughly 42 characters each. So it writes up to about 750 copies of the Builder until the disk is full. - Every later portable run then stops in `rmtree(install)`, because `home_path()` calls `fail()` ("too long for the Windows setup") on the over-long leftovers. Explorer can't easily delete them either. This is exactly the documented portable SSD use (`docs/windows-portable-builder.md`). The PR's runs avoided it because they set `LONGSHOT_SETUP_BASE`. - Minimum fix: refuse the copy when `dst` is `src` or lies under `src\` (compare the prefixed full paths case-insensitively, with the trailing `\`). - Better: also keep the portable home out of the zip's builder folder, or skip the home's own entries (`versions`, `downloads`, `.longshot-portable`) in that copy, so portable mode works at all. - Add a check to `test_portable_setup_transaction.py` (a copy into its own subfolder returns 0 quickly). 2. **The "Rebased on current `main`" box is ticked but false.** The branch forks at 39647e1a, and main is about 30 commits ahead. The PR merges cleanly, and nothing it touches changed, so this is about the box only: rebase (and re-run the checks), or untick it. ## Should change - **`rmtree()` can't remove what the copy can now create.** It still builds `MAX_PATH` paths through `home_path()`. If a deep copy is stopped or fails (in `reuse_part`'s cleanup, or in a `.part` left for the next run), cleanup either can't reach the files (standard home) or stops the setup with "too long" (portable home). Give `rmtree()` the same `\\?\` handling, or say in the PR that this is left for later. - **There's no check of the case this fixes.** The transaction test only copies short trees. Add a source tree deeper than 260 characters (Python on Windows can create one with a `\\?\` path) and assert the copy returns 1 and the files match. Otherwise a later edit can quietly bring back the `MAX_PATH` limit. - "How to reproduce" says to run the zip's `LongshotBuilder.exe`. On main, started by hand, it only shows the launcher message. Say `LongshotBuilder.exe --cli ...` (as the measurements did). - Nit: each call allocates 256 KiB, including the call for each file (1605 files). This is harmless, but the two prefixed paths could be built once at the top level and passed down. I didn't build or run anything (review only). Not checked on hardware: a UNC unpack folder, and exFAT ordering.
builder/setup/setup.c Outdated
@ -403,0 +394,4 @@
/* (paths past MAX_PATH: the builder zip's deepest file is 163 characters below its folder, so a zip unpacked a little
deep failed here with "Could not copy the Longshot Builder". An absolute path gets the \\?\ form (\\?\UNC\ for a
share), which Windows' file calls take up to 32767 characters, in heap buffers; a path already in it is kept) */
enum { CAP = 32768 };
HumanBean commented 2026-11-13 16:19:04 +00:00
Owner

With the limit raised to 32767, this function needs a guard against copying a folder into itself. In --portable mode started from the unpacked zip (no LONGSHOT_SETUP_BASE), init_home makes the home <setup folder>\LongshotBuilder, which is the same folder as g_app. work() then calls copy_part_tree(g_app, g_app\versions\<tag>.part\LongshotBuilder). On main that fails at MAX_PATH after a level or two. With this change it nests about 750 levels and copies most of the Builder at each one, until the disk is full. Later portable runs then stop in rmtree with "too long". Please refuse the copy when dst equals src or starts with src plus \ (case-insensitive, on the prefixed paths), and add a check for it to test_portable_setup_transaction.py.

With the limit raised to 32767, this function needs a guard against copying a folder into itself. In `--portable` mode started from the unpacked zip (no `LONGSHOT_SETUP_BASE`), `init_home` makes the home `<setup folder>\LongshotBuilder`, which is the same folder as `g_app`. `work()` then calls `copy_part_tree(g_app, g_app\versions\<tag>.part\LongshotBuilder)`. On main that fails at MAX_PATH after a level or two. With this change it nests about 750 levels and copies most of the Builder at each one, until the disk is full. Later portable runs then stop in `rmtree` with "too long". Please refuse the copy when `dst` equals `src` or starts with `src` plus `\` (case-insensitive, on the prefixed paths), and add a check for it to `test_portable_setup_transaction.py`.
builder/setup/setup.c Outdated
@ -412,0 +405,4 @@
const wchar_t *p = in[i], *prefix = L"", *rest = p;
if (wcsncmp(p, L"\\\\?\\", 4)) {
if (p[0] && p[1] == L':' && p[2] == L'\\') prefix = L"\\\\?\\";
else if (p[0] == L'\\' && p[1] == L'\\') prefix = L"\\\\?\\UNC", rest = p + 1;
HumanBean commented 2026-11-13 16:19:04 +00:00
Owner

The UNC form is right (\\server\share becomes \\?\UNC\server\share). Every input here comes from GetModuleFileNameW or GetFullPathNameW (through hb_home_full), so /, ./.. and trailing dots and spaces are already resolved before the prefix turns normalization off. Maybe say so in the comment, because C:/... or \\.\ would be handled wrongly if a future caller passed one.

The UNC form is right (`\\server\share` becomes `\\?\UNC\server\share`). Every input here comes from `GetModuleFileNameW` or `GetFullPathNameW` (through `hb_home_full`), so `/`, `.`/`..` and trailing dots and spaces are already resolved before the prefix turns normalization off. Maybe say so in the comment, because `C:/...` or `\\.\` would be handled wrongly if a future caller passed one.
builder/setup/setup.c Outdated
@ -412,0 +421,4 @@
else {
do {
if (!wcscmp(entry.cFileName, L".") || !wcscmp(entry.cFileName, L"..")) continue;
if (!hb_home_join(source, CAP, src, entry.cFileName) ||
HumanBean commented 2026-11-13 16:19:04 +00:00
Owner

The copy can now create paths past MAX_PATH, but rmtree(), which cleans up after a failed reuse_part copy and removes a leftover .part at the next run, still uses home_path() with MAX_PATH. In a portable home that calls fail() ("too long"). In the standard home the files can't be removed. Please give it the same \\?\ handling, or note it as follow-up work.

The copy can now create paths past MAX_PATH, but `rmtree()`, which cleans up after a failed `reuse_part` copy and removes a leftover `.part` at the next run, still uses `home_path()` with MAX_PATH. In a portable home that calls `fail()` ("too long"). In the standard home the files can't be removed. Please give it the same `\\?\` handling, or note it as follow-up work.
HumanBean added this to the v3.0.0 milestone 2026-11-13 16:55:16 +00:00
HumanBean requested changes 2026-11-13 17:10:20 +00:00
Dismissed
HumanBean left a comment

Re-review. The follow-ups to my review are on a branch, pr151-review on HumanBean/Longshot (199923a1): this PR rebased onto main f647a072 (your commit unchanged, as e1b3b013), plus two commits. Changes requested only until the PR head has them; with them in and a Windows test run, I approve.

Resolved on that branch

  • The --portable self-copy:
    • copy_part_tree refuses a destination equal to or under the source. It compares the \\?\ forms without case and ignores a trailing \, and …\LongshotBuilder2 doesn't count as under …\LongshotBuilder.
    • The portable home is now LongshotBuilder-data, so it's no longer the zip's Builder folder, and portable mode works from an unpacked zip.
    • If an earlier run already marked the zip's Builder folder as a home, the setup stops and says to unpack into a new folder, instead of copying it.
    • docs/windows-portable-builder.md is updated.
  • rmtree uses the same \\?\ paths, so it can remove anything the copy creates. In portable mode it no longer stops the setup with "too long".
  • The long paths are built once per call, in one heap buffer per side, with names added in place (new builder/setup/long_path.h). Both walks are capped at 256 levels.
  • The normalization caveat is documented in long_path.h: inputs come from GetModuleFileNameW/GetFullPathNameW, and C:/x or \\.\ would be mishandled.
  • Tests:
    • a self-copy in 4 spellings returns 0 and writes nothing;
    • a sibling with the same prefix copies;
    • a tree more than 300 characters deep copies with matching files and is removed whole.

Edge cases checked: UNC (\\server\share → \\?\UNC\…, and an existing \\?\UNC\ is kept), a drive root (\\?\C:\ keeps its \, and joins don't double it), a shared prefix, case, a trailing backslash, and the C:\ and \\?\ spellings of one path comparing equal.

Remaining, not blocking

  • The guard compares spellings only. The same folder reached through a subst or mapped drive, an 8.3 name or a junction isn't caught. The setup's own paths all come from GetModuleFileNameW/GetFullPathNameW, so this needs a hand-set LONGSHOT_SETUP_BASE, and the depth cap still bounds it.
  • New portable homes are LongshotBuilder-data, so a folder that an older setup made a home isn't picked up again.

Checks:

  • setup.c builds with tools.lock's llvm-mingw, CI's flags plus -Wall -Wextra, with only the 5 -Wmissing-field-initializers warnings that main has too.
  • Every Windows test compiles.
  • tests/run_all.py on Linux has no new failures; test_setup_tool_merge runs the new copy and rmtree through the POSIX stand-in.
  • Not run: the Windows-only tests (test_portable_setup_transaction, test_portable_setup_start, test_portable_builder_home). They need one Windows run before the merge.

PR body:

  • tick "Rebased on current main" once the branch is taken;
  • in "How to reproduce", use LongshotBuilder.exe --cli …;
  • mention the portable home rename.
Re-review. The follow-ups to my review are on a branch, `pr151-review` on HumanBean/Longshot (199923a1): this PR rebased onto main f647a072 (your commit unchanged, as e1b3b013), plus two commits. Changes requested only until the PR head has them; with them in and a Windows test run, I approve. **Resolved on that branch** - **The `--portable` self-copy:** - `copy_part_tree` refuses a destination equal to or under the source. It compares the `\\?\` forms without case and ignores a trailing `\`, and `…\LongshotBuilder2` doesn't count as under `…\LongshotBuilder`. - The portable home is now `LongshotBuilder-data`, so it's no longer the zip's Builder folder, and portable mode works from an unpacked zip. - If an earlier run already marked the zip's Builder folder as a home, the setup stops and says to unpack into a new folder, instead of copying it. - docs/windows-portable-builder.md is updated. - **`rmtree`** uses the same `\\?\` paths, so it can remove anything the copy creates. In portable mode it no longer stops the setup with "too long". - **The long paths** are built once per call, in one heap buffer per side, with names added in place (new `builder/setup/long_path.h`). Both walks are capped at 256 levels. - **The normalization caveat** is documented in `long_path.h`: inputs come from GetModuleFileNameW/GetFullPathNameW, and `C:/x` or `\\.\` would be mishandled. - **Tests:** - a self-copy in 4 spellings returns 0 and writes nothing; - a sibling with the same prefix copies; - a tree more than 300 characters deep copies with matching files and is removed whole. **Edge cases checked:** UNC (`\\server\share` → `\\?\UNC\…`, and an existing `\\?\UNC\` is kept), a drive root (`\\?\C:\` keeps its `\`, and joins don't double it), a shared prefix, case, a trailing backslash, and the `C:\` and `\\?\` spellings of one path comparing equal. **Remaining, not blocking** - The guard compares spellings only. The same folder reached through a subst or mapped drive, an 8.3 name or a junction isn't caught. The setup's own paths all come from GetModuleFileNameW/GetFullPathNameW, so this needs a hand-set LONGSHOT_SETUP_BASE, and the depth cap still bounds it. - New portable homes are `LongshotBuilder-data`, so a folder that an older setup made a home isn't picked up again. **Checks:** - setup.c builds with tools.lock's llvm-mingw, CI's flags plus `-Wall -Wextra`, with only the 5 `-Wmissing-field-initializers` warnings that main has too. - Every Windows test compiles. - `tests/run_all.py` on Linux has no new failures; test_setup_tool_merge runs the new copy and rmtree through the POSIX stand-in. - **Not run:** the Windows-only tests (test_portable_setup_transaction, test_portable_setup_start, test_portable_builder_home). They need one Windows run before the merge. **PR body:** - tick "Rebased on current main" once the branch is taken; - in "How to reproduce", use `LongshotBuilder.exe --cli …`; - mention the portable home rename.
HumanBean commented 2026-11-13 17:10:21 +00:00
Owner

@d4nkn3ss thanks for the fix! Maintainer edits are off on your branch, so the review follow-ups are on a branch of mine: pr151-review on https://git.magicbean.dev/HumanBean/Longshot.

It has your commit rebased onto current main (same author, message and content), plus two commits on top:

  • the copy refuses to copy into itself;
  • rmtree gets the same \\?\ handling;
  • the long paths are built once;
  • --portable's home becomes LongshotBuilder-data, with docs and tests.

To take them:

git fetch https://git.magicbean.dev/HumanBean/Longshot.git pr151-review
git checkout fix/setup-long-paths
git reset --hard FETCH_HEAD
git push --force-with-lease origin fix/setup-long-paths

The force push is only needed because of the rebase. Then tick "Rebased on current main", and change "How to reproduce" to LongshotBuilder.exe --cli ....

Your PC runs Windows, so python -I -B tests/run_all.py there would also cover the three portable setup tests that haven't run anywhere yet: test_portable_setup_transaction, test_portable_setup_start and test_portable_builder_home.

@d4nkn3ss thanks for the fix! Maintainer edits are off on your branch, so the review follow-ups are on a branch of mine: `pr151-review` on https://git.magicbean.dev/HumanBean/Longshot. It has your commit rebased onto current main (same author, message and content), plus two commits on top: - the copy refuses to copy into itself; - rmtree gets the same `\\?\` handling; - the long paths are built once; - `--portable`'s home becomes `LongshotBuilder-data`, with docs and tests. To take them: ``` git fetch https://git.magicbean.dev/HumanBean/Longshot.git pr151-review git checkout fix/setup-long-paths git reset --hard FETCH_HEAD git push --force-with-lease origin fix/setup-long-paths ``` The force push is only needed because of the rebase. Then tick "Rebased on current main", and change "How to reproduce" to `LongshotBuilder.exe --cli ...`. Your PC runs Windows, so `python -I -B tests/run_all.py` there would also cover the three portable setup tests that haven't run anywhere yet: test_portable_setup_transaction, test_portable_setup_start and test_portable_builder_home.
d4nkn3ss force-pushed fix/setup-long-paths from 56335697f9
Some checks failed
PR description / Description follows the template (pull_request_target) Successful in 5s
Details
CI / Conventional Commit title (pull_request) Has been cancelled
Details
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled
Details
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled
Details
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled
Details
CI / Workflow lint (actionlint) (pull_request) Has been cancelled
Details
Third-party parts / As their notes say (pull_request) Has been cancelled
Details
Third-party parts / The same as upstream (pull_request) Has been cancelled
Details
to e9ade33527
Some checks are pending
CI / Conventional Commit title (pull_request) Blocked by required conditions
Details
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions
Details
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions
Details
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions
Details
CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions
Details
Third-party parts / As their notes say (pull_request) Blocked by required conditions
Details
Third-party parts / The same as upstream (pull_request) Blocked by required conditions
Details
Website / Website on Cloudflare (pull_request) Blocked by required conditions
Details
PR description / Description follows the template (pull_request_target) Successful in 3s
Details
2026-11-13 17:46:05 +00:00 Compare
d4nkn3ss commented 2026-11-13 17:49:04 +00:00
Author
Contributor

Thanks for the review and for doing the follow-ups. Your branch is now this PR's head: pr151-review (199923a1) rebased onto main fbd42024 without conflicts, as e9ade335 (your two commits on mine, authors kept).

The Windows run you asked for, on Windows 11 with the Longshot Builder's llvm-mingw:

  • test_portable_setup_transaction.py, test_portable_setup_start.py, test_portable_builder_home.py, with test_portable_setup_paths.py, test_setup_tool_merge.py, test_setup_runtime.py and test_setup_runtime_paths.py: 7 passed.
  • The full suite: 494 passed; the 2 failures are the ones main has here too (#80, and #119/#144/#145).
  • The setup built from e9ade335 with build-windows.yml's commands, on nightly-34's zip:
    • unpacked with 306-character paths, --portable --cli --version with a scratch LONGSHOT_SETUP_BASE: all 1605 Builder files copied, then the tool downloads (stopped there);
    • --portable from the unpacked zip with no base: the home is <zip folder>\LongshotBuilder-data, 1605 files copied into it, and the zip's own LongshotBuilder still has its 1605, so no self-copy.

The description has your three points: Rebased ticked, "How to reproduce" with LongshotBuilder.exe --portable --cli …, and the portable home's rename (in What changed, Not tested and Risk), plus your commits and the remaining spelling-only limit.

Thanks for the review and for doing the follow-ups. Your branch is now this PR's head: `pr151-review` (199923a1) rebased onto main fbd42024 without conflicts, as e9ade335 (your two commits on mine, authors kept). The Windows run you asked for, on Windows 11 with the Longshot Builder's llvm-mingw: - `test_portable_setup_transaction.py`, `test_portable_setup_start.py`, `test_portable_builder_home.py`, with `test_portable_setup_paths.py`, `test_setup_tool_merge.py`, `test_setup_runtime.py` and `test_setup_runtime_paths.py`: **7 passed**. - The full suite: 494 passed; the 2 failures are the ones main has here too (#80, and #119/#144/#145). - The setup built from e9ade335 with `build-windows.yml`'s commands, on nightly-34's zip: - unpacked with 306-character paths, `--portable --cli --version` with a scratch `LONGSHOT_SETUP_BASE`: all 1605 Builder files copied, then the tool downloads (stopped there); - `--portable` from the unpacked zip with no base: the home is `<zip folder>\LongshotBuilder-data`, 1605 files copied into it, and the zip's own `LongshotBuilder` still has its 1605, so no self-copy. The description has your three points: Rebased ticked, "How to reproduce" with `LongshotBuilder.exe --portable --cli …`, and the portable home's rename (in What changed, Not tested and Risk), plus your commits and the remaining spelling-only limit.
HumanBean approved these changes 2026-11-13 17:57:21 +00:00
HumanBean left a comment

Approved. The head e9ade335 is the review branch rebased onto main with no changes (range-diff: all three commits identical). The Windows run covered what nothing else could:

  • the 7 setup tests pass;
  • a 306-character unpack copies all 1605 files;
  • --portable from the unpacked zip makes LongshotBuilder-data and leaves the zip's own LongshotBuilder intact.

The description has the three points. The remaining spelling-only limit (a subst drive or junction) is documented and not blocking. Thanks!

Approved. The head e9ade335 is the review branch rebased onto main with no changes (range-diff: all three commits identical). The Windows run covered what nothing else could: - the 7 setup tests pass; - a 306-character unpack copies all 1605 files; - `--portable` from the unpacked zip makes `LongshotBuilder-data` and leaves the zip's own `LongshotBuilder` intact. The description has the three points. The remaining spelling-only limit (a subst drive or junction) is documented and not blocking. Thanks!
Some checks are pending
CI / Conventional Commit title (pull_request) Blocked by required conditions
Details
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions
Details
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions
Details
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions
Details
CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions
Details
Third-party parts / As their notes say (pull_request) Blocked by required conditions
Details
Third-party parts / The same as upstream (pull_request) Blocked by required conditions
Details
Website / Website on Cloudflare (pull_request) Blocked by required conditions
Details
PR description / Description follows the template (pull_request_target) Successful in 3s
Details
This pull request can be merged automatically.
Some workflows are waiting to be reviewed.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u 'fix/setup-long-paths:d4nkn3ss-fix/setup-long-paths'
git switch 'd4nkn3ss-fix/setup-long-paths'
Sign in to join this conversation.
Reviewers
No reviewers
HumanBean
Labels
Clear labels   
area/accessibility
Barriers for people with disabilities

  
area/audio
Sound and music

  
area/input
Controllers, keyboard, mouse

  
area/install
Installing, updating, building from your game

  
area/menu
In-game menu and settings

  
area/renderer
Vulkan renderer, shaders, effects

  
area/saves
Save files

  
area/upscaling
DLSS, FSR, frame generation

  
good first issue
A good start for a new contributor

  
gpu/amd
AMD GPUs

  
gpu/igpu
Integrated GPUs

  
gpu/intel
Intel GPUs

  
gpu/nvidia
NVIDIA GPUs

  
help wanted
Extra hands or knowledge wanted

  
kind
bug
 Something doesn't work as it should

  
kind
chore
 Build, CI, dependencies, tidying

  
kind
docs
 Documentation

  
kind
feature
 A new feature or an improvement

  
kind
performance
 Frame rate, frame time, loading, memory

  
kind
question
 A question rather than a change

  
needs measurements
Waiting for before/after measurements

  
platform/linux
Linux only

  
platform/steam-deck
Steam Deck

  
platform/windows
Windows only

  
preview
Maintainers: deploy a preview of this PR's website (read its changes first). Removed after the run.

  
priority
critical
 Crashes, data loss, or blocks most players

  
priority
high
 Important: next in line

  
priority
low
 Nice to have

  
status
blocked
 Waits on something else first

  
status
confirmed
 Reproduced or agreed on

  
status
duplicate
 Already reported or proposed

  
status
invalid
 Not a problem in this project

  
status
needs-info
 Waiting for more information from the reporter

  
status
wontfix
 Will not be worked on

  
testing wanted
Wanted: a test on other hardware (posts a test request in #test-requests)

No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
good first issue
gpu/amd
gpu/igpu
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
needs measurements
platform/linux
platform/steam-deck
platform/windows
preview
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
Milestone
Clear milestone
No items
No milestone
v3.0.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
MagicBeanDev/Longshot!151
No description provided.