engineering: use tar+zstd for win32 node_modules cache#260131
engineering: use tar+zstd for win32 node_modules cache#260131tmm1 wants to merge 7 commits intomicrosoft:mainfrom
Conversation
deepak1556
left a comment
There was a problem hiding this comment.
Thanks! should also mirror this in .github/workflows/pr-node-modules.yml and .github/workflows/pr-win32-test.yml that is used for pr builds
Those paths already use tar but may see added speedups from zstd. For some reason whatever 7z does in the azure-pipeline is very slow. |
The windows jobs use 7z andvscode/.github/workflows/pr-node-modules.yml Line 293 in d5e0905 |
|
@joaomoreno actually wrote the archive cache steps, I couldn't find from commit history why 7z was preferred for windows, maybe it was one of the reasons you had pointed out but lets wait for his review before pushing this forward. He is oof this week :)
|
There was a problem hiding this comment.
Pull request overview
Speeds up Win32 node_modules caching by switching from 7z archives to tar+zstd, aiming to reduce cache extraction time in CI.
Changes:
- Replace
7z.exearchive creation/extraction withtar.exe+zstdon Win32 (Azure Pipelines + GitHub Actions). - Update cache artifact name from
cache.7ztocache.tzst. - Bump
build/.cachesaltto invalidate existing caches.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| build/azure-pipelines/win32/steps/product-build-win32-compile.yml | Switch restore/extract + create cache archive steps to tar+zstd for win32 builds. |
| build/azure-pipelines/win32/product-build-win32-node-modules.yml | Switch node-modules cache archive creation to tar+zstd. |
| build/.cachesalt | Cache invalidation bump to force regeneration under new archive format. |
| .github/workflows/pr-win32-test.yml | Update Win32 PR workflow cache extract/create steps to tar+zstd. |
| .github/workflows/pr-node-modules.yml | Update Win32 node-modules workflow cache archive creation to tar+zstd. |
Comments suppressed due to low confidence (2)
build/azure-pipelines/win32/steps/product-build-win32-compile.yml:113
- The archive creation uses tar’s
-P(preserve absolute paths). SincelistNodeModules.tsemits relative paths,-Pisn’t needed and can make the resulting archive unsafe to extract if absolute/path-traversal entries ever get introduced. Prefer omitting-Pso the archive contains only relative paths.
exec { & "C:\Program Files\Git\usr\bin\tar.exe" --posix -cf .build/node_modules_cache/cache.tzst --exclude cache.tzst -P -C . --files-from .build/node_modules_list.txt --force-local --use-compress-program "zstd -T0" }
.github/workflows/pr-win32-test.yml:89
- The create-archive tar command includes
-P(preserve absolute paths). Given the archive inputs are relative,-Pis unnecessary and increases the risk of writing outside the workspace if unexpected entries ever make it into the cache archive. Prefer omitting-Pfor safer archives.
exec { & "C:\Program Files\Git\usr\bin\tar.exe" --posix -cf .build/node_modules_cache/cache.tzst --exclude cache.tzst -P -C . --files-from .build/node_modules_list.txt --force-local --use-compress-program "zstd -T0" }
build/azure-pipelines/win32/steps/product-build-win32-compile.yml
Outdated
Show resolved
Hide resolved
build/azure-pipelines/win32/product-build-win32-node-modules.yml
Outdated
Show resolved
Hide resolved
build/azure-pipelines/win32/product-build-win32-node-modules.yml
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Moving to #291624 due to engineering checks protections |
this is significantly faster in my tests, down to 60s vs 120-180s before in the Extract node_modules phase
GHA and Azure Pipelines images have tar.exe and zstandard bundled since a few years ago
(will need to bump cachesalt if merging, to invalidate old cache)
cc @lszomoru @deepak1556