Repository navigation
Conversation
Pass overwrite flags (`-aoa` to 7-Zip, `-Force` to Expand-Archive, and 16 to COM copyhere) in `update_dart_sdk.ps1` to prevent interactive overwrite confirmation prompts when expanding downloaded SDK archives on Windows. Generalize post-extraction artifact movement in both `update_dart_sdk.sh` and `update_dart_sdk.ps1` to iterate over all files and directories extracted into the temporary directory and move them into `bin/cache/`, removing hardcoded single-file assumptions. Add comprehensive automated test `dev/tools/test/update_dart_sdk_test.dart`. Fixes flutter#132702
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Dart SDK update scripts (update_dart_sdk.ps1 and update_dart_sdk.sh) to move all extracted files and directories from the temporary directory to the cache directory, and introduces a new integration test suite in update_dart_sdk_test.dart. The review feedback highlights a compilation error in the new test file where FileSystemEntity.basename is accessed incorrectly, and suggests using LineSplitter.split for robust line splitting. Additionally, the feedback recommends specifying an execution policy when invoking PowerShell on Windows, and improving error handling in both the PowerShell and Shell update scripts to prevent silent failures during file operations.
| if (Test-Path $dest) { | ||
| Remove-Item $dest -Recurse -Force -ErrorAction SilentlyContinue | ||
| } |
There was a problem hiding this comment.
Using -ErrorAction SilentlyContinue when removing the destination path is risky. If the deletion fails (e.g., due to a file lock or permission issue), the subsequent Move-Item will either fail with a confusing error or silently move the directory inside the existing one, leading to a corrupted cache. It is safer to use -ErrorAction Stop to ensure any deletion failure is caught and handled.
if (Test-Path $dest) {
Remove-Item $dest -Recurse -Force -ErrorAction Stop
}
Check the exit code of `mv` when moving extracted items to `bin/cache/` in `update_dart_sdk.sh` to prevent continuing on failure. Use `LineSplitter.split()` in `update_dart_sdk_test.dart` to robustly split process output lines across all platforms.
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
2 participants
Fixes #132702
Summary
When downloading and extracting the Dart SDK archive during Flutter bootstrap/upgrade (
update_dart_sdk.shandupdate_dart_sdk.ps1), archives containing files outsidedart-sdk/(e.g.LICENSE.dart_sdk_archive.mdor future metadata/artifacts) previously caused extraction issues:Extraction prompt hangs on Windows:
In #132592, adding
LICENSE.dart_sdk_archive.mdto the archive root caused 7-Zip (7z/7za) to prompt interactively on stdout/stdin when overwriting an existing file, which PowerShell hides from background processes, resulting in an indefinite hang.Workaround in #132777 only targeted the single license file without addressing the general overwrite conflict during extraction.
Discarded archive artifacts:
Both
update_dart_sdk.shandupdate_dart_sdk.ps1previously hardcoded moving onlyLICENSE.dart_sdk_archive.md. Any other top-level files or directories in the Dart SDK archive were discarded during cleanup of the temporary extraction directory.Solution
bin/internal/update_dart_sdk.ps1:-aoaoverwrite switch to7zand7za.-Forceswitch toMicrosoft.PowerShell.Archive\Expand-Archive.16(FOF_NOCONFIRMATION) to COMcopyhere($item, 16).bin/cache/.bin/internal/update_dart_sdk.sh:$DART_SDK_PATH_TEMP/*, safely removing preexisting destination items and moving extracted artifacts intobin/cache/.Tests
dev/tools/test/update_dart_sdk_test.darttesting mock SDK download, multi-file/directory extraction, version upgrades, and non-interactive overwrite handling.Pre-launch Checklist
///).