feat: add Dev Drive setup for Windows in CI workflow#5544
Conversation
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. |
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. |
There was a problem hiding this comment.
CI Workflow Review: Dev Drive Setup for Windows
This is a clean, focused CI optimization PR. The approach is well-established and the implementation is technically sound. Here are my observations:
What This Does Well
- Correct placement: The Dev Drive setup steps appear before
actions/checkout, which is the correct order — the workspace redirect needs to happen before the repo is cloned onto it. - NuGet redirect: Remapping
NUGET_PACKAGESto the Dev Drive viaenv-mappingis smart. NuGet's heavy random-read/write pattern benefits significantly from ReFS + Dev Drive's trust bypass, which avoids real-time antivirus scanning on trusted drives. - Uses
Dynamicdrive type: This avoids pre-allocating 10 GB of disk space on the runner, which is important given how space-constrained GitHub-hosted Windows runners are.
Concerns Worth Considering
1. NuGet cache step path mismatch
The workflow already has a Cache NuGet packages step that caches ~/.nuget/packages. After this PR, NUGET_PACKAGES is redirected to the Dev Drive, but the cache step's path still lists ~/.nuget/packages (the original location). This means:
- On cache restore: packages are written to
~/.nuget/packages, which is NOT the path NuGet will actually read from during build. - On cache save: the Dev Drive path is not saved, so the cache is effectively wasted on Windows.
The cleaner fix is to use the env var in the cache path directly, which works cross-platform:
- name: Cache NuGet packages
uses: actions/cache@v5
with:
path: |
${{ env.NUGET_PACKAGES }}
~/.local/share/NuGet
%LocalAppData%\NuGet\v3-cacheenv.NUGET_PACKAGES will resolve to ~/.nuget/packages on Linux/macOS (the default) and to the Dev Drive path on Windows, making the cache step self-correcting without any OS-specific branching.
2. No error handling on junction creation
The cmd /c "mklink /J ..." call can fail silently if permissions are denied. If it fails, the script continues but $env:GITHUB_WORKSPACE no longer exists — subsequent steps will fail with confusing path-not-found errors rather than a clear "junction creation failed" message.
Consider replacing the cmd shim with the PowerShell-native equivalent, which throws on failure:
New-Item -Path $env:GITHUB_WORKSPACE -ItemType Junction -Target $refsBuildDirThis avoids shelling out to cmd.exe and integrates cleanly with PowerShell error handling.
3. Workspace redirect fragility with git clean
The Remove-Item + junction pattern is a well-known approach for this use case, but if any future step runs git clean -ffdx (which follows junctions on Windows), it would wipe the Dev Drive directory rather than failing safely. This is low-risk for the current pipeline, but worth keeping in mind if a "clean workspace" step is ever added.
4. Action version pinning
samypr100/setup-dev-drive@v4 is pinned to a floating major-version tag rather than a commit SHA. This is consistent with how other actions in the workflow are pinned (e.g., actions/checkout@v6), so it is not a regression — just a general supply-chain hygiene note if the project ever moves toward SHA pinning.
Summary
The core approach is sound and the performance benefit on Windows is real — avoiding Defender scanning overhead on NuGet restore and build can meaningfully reduce CI times. The primary actionable item is keeping the Cache NuGet packages step in sync with the Dev Drive redirect; as written, the cache step may be a no-op on Windows after this change, which wastes runner time and negates part of the benefit.
Up to standards ✅🟢 Issues
|
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. |
There was a problem hiding this comment.
Follow-up Review
This is a follow-up to my previous review. The PR has not been updated since, so the concerns I raised earlier remain unaddressed. I want to highlight priority order and add one additional observation.
Issues Still Unaddressed
1. NuGet cache path mismatch (highest priority — functional correctness)
The Cache NuGet packages step (lines 99-109) still hardcodes ~/.nuget/packages:
- name: Cache NuGet packages
uses: actions/cache@v5
continue-on-error: true
with:
path: |
~/.nuget/packages # <-- hardcoded, doesn't match the redirected NUGET_PACKAGES on Windows
~/.local/share/NuGet
%LocalAppData%\NuGet\v3-cacheAfter this PR, on Windows, NUGET_PACKAGES is set to <DEV_DRIVE>/.nuget/packages. The cache step saves and restores ~/.nuget/packages, which NuGet will not use at all — it reads from the path in NUGET_PACKAGES. This means:
- Cache restore unpacks packages to the wrong directory; NuGet still downloads everything fresh.
- Cache save captures the old (empty)
~/.nuget/packagesdirectory.
The Windows NuGet cache becomes entirely non-functional with this PR as written. The fix is straightforward — use the env var in the cache path:
- name: Cache NuGet packages
uses: actions/cache@v5
continue-on-error: true
with:
path: |
${{ env.NUGET_PACKAGES }}
~/.local/share/NuGet
%LocalAppData%\NuGet\v3-cache
key: nuget-${{ runner.os }}-${{ hashFiles('**/Directory.Packages.props', '**/*.csproj') }}
restore-keys: |
nuget-${{ runner.os }}-On Linux/macOS, NUGET_PACKAGES is not set by the Dev Drive step, so it will be empty or default — you may want to set it explicitly to ~/.nuget/packages in the env-mapping or as a separate env: declaration so the cache path resolves correctly on all platforms. Alternatively, set a global env: at the job level:
env:
NUGET_PACKAGES: ~/.nuget/packages # overridden on Windows by setup-dev-driveand then the cache step just uses ${{ env.NUGET_PACKAGES }} everywhere.
2. cmd /c mklink /J instead of PowerShell-native junction creation
The junction is created via cmd /c "mklink /J ...". If mklink fails (e.g., due to permissions), the exit code is swallowed by the cmd /c shim and the script continues silently, making $env:GITHUB_WORKSPACE a dangling path. Subsequent steps fail with confusing path-not-found errors.
The PowerShell-native equivalent throws on failure and integrates cleanly with the $ErrorActionPreference setting:
New-Item -Path $env:GITHUB_WORKSPACE -ItemType Junction -Target $refsBuildDirThis is also cleaner — no reason to shell out to cmd.exe from a PowerShell script.
Additional Observation
The env-mapping syntax uses {{ DEV_DRIVE }} (double braces), which is the setup-dev-drive action's own template syntax — not GitHub Actions expression syntax. This is correct per that action's documentation, but it is worth a quick sanity check against the v4 release notes to confirm the syntax has not changed between major versions.
Summary
The core approach (Dev Drive + workspace junction) is still sound and the performance benefit is real. But the NuGet cache mismatch is a correctness issue that will silently neutralize most of the performance gain on Windows. Please address point 1 before merging — it is a functional regression for the cache hit rate.
…ction and adjusting cache paths
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. |
There was a problem hiding this comment.
Review: feat: add Dev Drive setup for Windows in CI workflow
This is a focused CI performance optimization. Across 4 commits the PR has evolved significantly from its original form — the workspace redirection step that was the source of most friction in earlier reviews has been dropped, leaving a simpler and more conservative change. The review below reflects the current state of the PR at HEAD.
What the Current PR Does
- Adds
samypr100/setup-dev-drive@v4afteractions/checkouton Windows runners, creating a 10 GB Dev Drive and remappingNUGET_PACKAGESto it. - Replaces the multi-path NuGet cache block with a single
${{ env.NUGET_PACKAGES || '~/.nuget/packages' }}expression that resolves to the Dev Drive path on Windows and the standard path everywhere else.
Good Choices
Previous reviews addressed. The NuGet cache path mismatch that would have made caching a no-op on Windows has been fixed — the path: expression now follows NUGET_PACKAGES correctly. The workspace redirection step (with mklink /J fragility) has been removed entirely. This is the right call; the workspace redirect added real failure modes for marginal gain, and the primary benefit of Dev Drive for CI comes from the NuGet restore path being on a trusted, ReFS-formatted volume anyway.
trusted-dev-drive: true is correctly set. Without this flag the drive would still be scanned by Defender on access, negating most of the performance benefit.
drive-size: 10GB is reasonable. The default is dynamic allocation, so this does not pre-reserve 10 GB of runner disk space, which matters on the space-constrained Windows GitHub runners.
Issues and Concerns
1. Checkout happens before the Dev Drive is created — NuGet packages are the only beneficiary
In the current workflow, actions/checkout runs first, then setup-dev-drive. The source tree lives on the standard NTFS runner volume. Only the NuGet package cache (via NUGET_PACKAGES) lands on the Dev Drive. This is still a meaningful win for restore speed, but it means the build output (obj/, bin/, intermediate files) is written to NTFS and scanned by Defender in real time.
If the intent was to move the workspace onto the Dev Drive (as earlier commits attempted), this PR no longer accomplishes that. If that was intentionally dropped, the PR title and description should make clear this is a NuGet-only optimization.
2. Cache path expression drops two previously cached paths
The old cache block was:
path: |
~/.nuget/packages
~/.local/share/NuGet
%LocalAppData%\NuGet\v3-cacheThe new block is:
path: ${{ env.NUGET_PACKAGES || '~/.nuget/packages' }}~/.local/share/NuGet (the NuGet HTTP cache on Linux) and %LocalAppData%\NuGet\v3-cache (the NuGet HTTP cache on Windows, distinct from the global packages folder) are no longer cached. These contain the raw downloaded .nupkg files before extraction. Losing this cache does not break builds, but it means HTTP round-trips to nuget.org for every cache miss rather than just extraction from the local copy.
Whether this matters in practice depends on the network environment, but it is a behavioural regression relative to the base branch and should be a deliberate choice rather than an accidental one.
3. setup-dev-drive@v4 is pinned to a floating major-version tag
samypr100/setup-dev-drive@v4 will silently pick up any v4.x.y patch release. This is consistent with how other actions in the workflow are pinned (e.g., actions/checkout@v6), so it is not a regression from base, but worth noting if the project ever adopts supply-chain SHA pinning.
4. NUGET_PACKAGES is unset on Linux and macOS — the fallback is implicit
The expression ${{ env.NUGET_PACKAGES || '~/.nuget/packages' }} relies on NUGET_PACKAGES being empty on non-Windows runners. This works today, but if any other step or the runner image ever sets NUGET_PACKAGES to a different value on Linux/macOS, the cache key and restore path would diverge silently. A more defensive approach is to set the env var explicitly at the job level as a default:
env:
NUGET_PACKAGES: ~/.nuget/packages # default for all platformsThen setup-dev-drive's env-mapping overrides it for Windows, and the cache step just uses ${{ env.NUGET_PACKAGES }} without any || fallback. This makes the intent explicit and removes the implicit dependency on the env var being absent.
Summary
The current state of the PR is cleaner and more correct than earlier iterations. The core approach is sound and the NuGet restore performance improvement on Windows is real — ReFS + Defender trust bypass can meaningfully reduce restore time on large package graphs like TUnit's.
The primary actionable items are:
- Explicitly decide whether dropping
~/.local/share/NuGetand%LocalAppData%\NuGet\v3-cachefrom the cache is intentional, and if not, restore them for the respective platforms while keepingNUGET_PACKAGESdynamic. - Consider setting
NUGET_PACKAGESas a job-level default env var to avoid the implicit||fallback and make cross-platform behaviour explicit.
Neither item is a blocking correctness issue — item 1 is a performance concern and item 2 is a maintainability concern. The PR is otherwise in good shape if the NuGet-packages-only scope is intentional.
…bles for NuGet packages and temporary files
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. |
There was a problem hiding this comment.
Follow-up Review (latest commit: 6cc8174)
This is a follow-up to my three previous reviews. The PR has continued to evolve and the latest commit addresses several outstanding concerns.
Previous Issues — Status
Fixed: NuGet cache path mismatch — The cache path: now uses ${{ env.NUGET_PACKAGES }} (resolves to the Dev Drive on Windows, ~/.nuget/packages elsewhere). This was the primary correctness concern from earlier reviews. ✅
Fixed: Implicit || fallback for NUGET_PACKAGES — A job-level env: block now sets NUGET_PACKAGES: ~/.nuget/packages as a default for all platforms. The setup-dev-drive action overrides it for Windows via env-mapping. No implicit environment-state dependency. ✅
Fixed: Cache drops ~/.local/share/NuGet and %LocalAppData%\NuGet\v3-cache — These paths are still present in the cache path: block. HTTP response cache is preserved. ✅
Remaining: Floating action tag — samypr100/setup-dev-drive@v4 is still a floating major-version tag. This is consistent with other actions in the workflow (e.g., actions/checkout@v6), so it is not a regression. Low priority.
New Observations on the Latest Commit
TEMP/TMP remapping is a meaningful addition
Remapping TEMP and TMP to the Dev Drive means intermediate files emitted during compilation (e.g., Roslyn source generators, MSBuild temporary outputs) land on the trusted volume and bypass Defender scanning. This addresses the earlier concern that only NuGet restore benefited while build intermediates remained on the standard NTFS volume.
One edge to be aware of: some tooling (e.g., older MSBuild tasks, test adapters) reads TEMP at process startup before the environment is fully inherited. If a step uses shell: cmd, it may not see the updated TMP value depending on how the runner proxies environment variables. This is unlikely to be a problem in practice with the current workflow steps, but worth knowing if unexpected temp-path failures surface.
Ordering: setup-dev-drive runs after actions/checkout
The source tree is checked out before the Dev Drive is mounted. Only subsequent writes (NuGet restore, temp files) go to the Dev Drive; the .git directory and working tree stay on NTFS. This is a known, acceptable trade-off for the current approach — moving checkout to the Dev Drive would require a more complex junction-based setup (which was explored and dropped in earlier commits).
Summary
The PR is in good shape. All correctness and behavioural concerns from earlier reviews have been addressed. The remaining items (floating tag, NTFS workspace) are either consistent with the existing workflow's conventions or are intentional scope decisions.
The Dev Drive + NuGet redirect + TEMP redirect combination is now a solid, low-risk Windows CI performance improvement. Ready to merge from a code-correctness standpoint.
No description provided.