AOT publish passes with long paths on Windows#121956
Conversation
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
There was a problem hiding this comment.
Pull request overview
This PR addresses the fix for issue #103625 by removing the workaround for long path failures on Windows during AOT publish and adding a dedicated test to verify the fix.
- Removed commented-out active issue annotation and explanatory comments about long path limitations
- Added a new test
DefaultTemplate_AOT_WithLongPaththat explicitly creates a project with a long path and performs AOT publish to ensure it works correctly
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Is this about the whole command line that we pass to See also #123548 |
No, only the path. As you can see in changes list, the problem is not only in C code, managed code also has holes when it comes to long path handling. Changing eglib to response files sounds like a much more serious change than appending prefixes. I don't believe we should overcomplicate it. |
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
| @@ -140,6 +143,7 @@ g_fopen (const gchar *path, const gchar *mode) | |||
| g_free (wPath); | |||
| g_free (wMode); | |||
| } | |||
| g_free (path_mod); | |||
| char current_dir[MAX_PATH]; | ||
| if (GetCurrentDirectoryA(MAX_PATH, current_dir) > 0 && current_dir[1] == ':') { | ||
| gchar *result = g_malloc(strlen(path) + 3); | ||
| result[0] = current_dir[0]; | ||
| result[1] = ':'; | ||
| strcpy(result + 2, path); | ||
| return result; | ||
| } | ||
|
|
||
| return g_strdup(path); |
| // The problematic path is in a form of: | ||
| // {_projectDir}\obj\..\Microsoft_Extensions_DependencyInjection_dll_compiled_methods.txt | ||
| // Its length is at least 100 characters, so we can subtract it from the nesting target | ||
| const int longestBuildSubpathLength = 100; | ||
| const int windowsMaxPath = 260; | ||
|
|
||
| int currentLength = _projectDir.Length; | ||
| int targetPathLength = windowsMaxPath - longestBuildSubpathLength; | ||
| int additionalLength = Math.Max(0, targetPathLength - currentLength); | ||
|
|
||
| if (additionalLength == 0) | ||
| return info; | ||
|
|
||
| string dirName = new string('x', additionalLength); | ||
| string nestedPath = Path.Combine(baseDir, dirName); | ||
| Directory.CreateDirectory(nestedPath); | ||
|
|
||
| string newTestProjectDir = Path.Combine(nestedPath, testProjectDirName); | ||
| Directory.Move(testProjectDir, newTestProjectDir); | ||
|
|
||
| _projectDir = Path.Combine(newTestProjectDir, "App"); | ||
| string nestedProjectFilePath = Path.Combine(_projectDir, "BlazorBasicTestApp.csproj"); | ||
|
|
||
| return new ProjectInfo(info.ProjectName, nestedProjectFilePath, info.LogPath, info.NugetDir); | ||
| } |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "d49dc0334a3025c4e606f44ab69e6784c6cc9f1e",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "ac220bb785a8db28a9a46517a648ad14f1d6e2f4",
"last_reviewed_commit": "d49dc0334a3025c4e606f44ab69e6784c6cc9f1e",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "ac220bb785a8db28a9a46517a648ad14f1d6e2f4",
"last_recorded_worker_run_id": "29682989073",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "d49dc0334a3025c4e606f44ab69e6784c6cc9f1e",
"review_id": 4730600511
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. Windows AOT/WASM builds genuinely fail once intermediate assembly paths exceed the 260-char MAX_PATH limit (issue #103625), and the PR removes a real ActiveIssue workaround, so the problem and value are concrete.
Approach: Reasonable direction — add the \\?\ long-path prefix at the eglib file-IO boundary plus pass fully-qualified paths from the AOT task. But the implementation has a compile-breaking API choice on the .NET Framework target leg, a memory leak, a potentially unsafe GetCurrentDirectoryA read, and a cross-platform behavior change to image->name that reaches beyond the long-Windows-path scenario it targets.
Summary: Path.IsPathFullyQualified call will not compile for the task's net472 target, and there is a new leak on the g_fopen error path — both should be fixed before merge. Additional concerns (fixed 260-byte CWD buffer, unconditional canonicalization of image->name on all platforms, guard-macro G_OS_WIN32 vs HOST_WIN32 inconsistency, and a test that silently no-ops when the base path is already long) warrant author attention. I could not build/test in this environment, so please verify the CI Windows and .NET Framework legs.
Detailed Findings
❌ Correctness — Path.IsPathFullyQualified breaks the .NET Framework build leg
MonoAOTCompiler.csproj multi-targets $(NetFrameworkToolCurrent) (net472). Path.IsPathFullyQualified was never added to .NET Framework, so the net472 compile will fail. See inline comment on MonoAOTCompiler.cs.
❌ Resource management — leak on g_fopen early return
The new path_mod allocation is not freed on the if (!wPath || !wMode) return NULL; path. See inline comment on gfile.c.
⚠️ Correctness — GetCurrentDirectoryA into fixed 260-byte buffer
A CWD longer than MAX_PATH leaves current_dir unfilled while the code still reads it. Given the feature targets long paths, this is a plausible uninitialized-read. See inline comment on gpath.c.
⚠️ Cross-cutting — image.c canonicalizes image->name for all paths/platforms
Passing absfname into do_mono_image_open changes image->name/image->filename to the resolved/canonicalized (and on Windows, possibly \\?\-prefixed) path unconditionally, not just for long Windows paths. This can affect assembly-name comparisons and diagnostics on every platform. See inline comment on image.c.
⚠️ Portability — guard-macro inconsistency (G_OS_WIN32 vs HOST_WIN32)
Declaration/definition use G_OS_WIN32; all call sites use HOST_WIN32. It compiles on Windows where both are defined, but the mismatch is fragile. See inline comment on glib.h.
⚠️ Test quality — long-path test can silently no-op
NestProjectInLongPath returns early when the base path is already long, so the reproduced path length depends on CI checkout depth rather than being deterministic. See inline comment on BuildPublishTests.cs.
💡 Minor — trailing whitespace / style
Several new lines contain trailing whitespace (e.g., after the g_free/blank lines in gfile-win32.c and gfile.c), and calls like g_path_make_long_compatible(path) omit the space before ( used elsewhere in these files. CI/style may flag these; align with the surrounding file style.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 325.4 AIC · ⌖ 21.7 AIC · ⊞ 10K
Comments that could not be inline-anchored
src/tasks/AotCompilerTask/MonoAOTCompiler.cs:1017
❌ Path.IsPathFullyQualified does not exist on .NET Framework. This task multi-targets $(NetFrameworkToolCurrent) (net472) as well as $(NetCoreAppToolCurrent) (see MonoAOTCompiler.csproj <TargetFrameworks>), and Path.IsPathFullyQualified was introduced in .NET Core 2.1 / netstandard2.1 and was never added to .NET Framework. This will fail to compile (CS0117) for the net472 build leg. Use Path.IsPathRooted combined with a drive/UNC check, or guard the API behind a TFM #if NET, t…
src/mono/mono/eglib/gfile.c:140
❌ Memory leak on the early-return error path. path_mod is allocated by g_path_make_long_compatible(path) at line 131, but this return NULL; (when wPath/wMode allocation fails) returns before reaching the g_free (path_mod) at line 146, and also leaks wPath/wMode themselves. This early-return leak of wPath/wMode pre-existed, but the new path_mod allocation now adds a further leak on this path. Please free path_mod (and ideally wPath/wMode) before returning.
src/mono/mono/eglib/gpath.c:92
GetCurrentDirectoryA writes into a fixed MAX_PATH (260) buffer. If the current directory itself is longer than 260 chars, the call returns the required size (greater than MAX_PATH) and does not fill the buffer, but the code still reads current_dir[1] and current_dir[0] - reading uninitialized stack memory. Since the whole feature targets long paths, the CWD can plausibly be long. Consider checking the return value against MAX_PATH (retry with a heap buffer sized to the returned length) or use t…
src/mono/mono/metadata/image.c:1779
This changes image loading to use absfname (mono_path_resolve_symlinks, which on Windows now returns a long-path-prefixed, canonicalized path) as the fname passed into do_mono_image_open, instead of the original fname. do_mono_image_open sets image->name/image->filename from this fname. This means image->name now becomes the canonicalized (and possibly \?-prefixed) path on ALL platforms/lengths, not just long Windows paths - a behavioral change that affects assembly name comparisons, diagnost…
src/mono/mono/eglib/glib.h:902
Guard-macro inconsistency worth double-checking: the declaration here and the definition in gpath.c are guarded by G_OS_WIN32, while every caller (gfile.c line 131, gfile-win32.c, mono-path.c line 103) invokes the function under HOST_WIN32. On the Windows build both macros are defined so this compiles, but the mismatch is fragile - a caller compiled with HOST_WIN32 set but G_OS_WIN32 unset would reference an undeclared/undefined symbol. Recommend using a single consistent macro (HOST_WIN32) for…
src/mono/wasm/Wasm.Build.Tests/Blazor/BuildPublishTests.cs:91
When the existing project path is already long enough (additionalLength == 0) the test silently skips the nesting and proceeds. That means on machines/CI with short base paths the test does exercise long paths, but on any environment where the base already exceeds the target it becomes a near-no-op without signalling that the long-path scenario was (or wasn't) reached. Consider making the target length deterministic (always nest to a fixed length beyond MAX_PATH regardless of the starting lengt…
Fixes #103625.
This PR fixes Windows MAX_PATH (260 character) limitation in WASM AOT compilation by adding
\\?\prefix support to long file paths across the Mono runtime's file operations.Long path prefix comes from Unicode “W” APIs. Windows versions from Windows NT on support that API.
OSs that do not support this change:
Changes:
gpath.c: Adds
g_path_make_long_compatible()helper function that conditionally adds\\?\prefix to Windows paths ≥260 characters, supporting both regular absolute paths (C:\path→\\?\C:\path) and UNC network paths (\\server\share→\\?\UNC\server\share).mono-path.c: Uses
g_path_make_long_prefix()inmono_path_canonicalize()to ensure canonicalized paths support long path operations.image.c: Uses the canonicalized absolute path (
absfname) instead of the original filename when loading assemblies to ensure long paths are handled correctly throughout the image loading process.MonoAOTCompiler.cs: Passes full absolute paths to the AOT compiler instead of potentially relative paths to ensure Windows long path support (
\\?\prefix from canonicalization) works correctly.gfile-win32.c: Calls
g_path_make_long_compatible()ing_file_test()before callingGetFileAttributesW()to enable existence checks for files with long paths.gfile.c: Calls
g_path_make_long_compatible()ing_fopen()before opening files to support long paths when the runtime needs to open assembly files.BuildPublishTests.cs: Adds
DefaultTemplate_AOT_WithLongPathtest with 300+ character path prefix to verify long path support works correctly with AOT compilation.