Skip to content

Commit ea9cbbd

Browse files
committed
Revised from Devin AI comments
1 parent 4ce9d95 commit ea9cbbd

5 files changed

Lines changed: 82 additions & 12 deletions

File tree

.github/instructions/build.instructions.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,11 +52,13 @@ This repo supports multiple concurrent builds across git worktrees. Prefer the s
5252
- Optional actor tagging: set `FW_BUILD_STARTED_BY=user|agent` (or pass `-StartedBy`) to record who owns the lock.
5353

5454
### MSBuild node reuse default
55-
- `build.ps1` defaults `-NodeReuse` to `false` to reduce cross-worktree lock contention from shared MSBuild worker nodes.
56-
- If you prefer faster local inner-loop builds and are not running concurrent worktrees, you can opt back in with:
55+
- `build.ps1` defaults `-NodeReuse` to `auto`.
56+
- In `auto` mode, MSBuild node reuse is enabled when the repository has a single local worktree and disabled when multiple local worktrees are detected, reducing cross-worktree lock contention from shared worker nodes.
57+
- You can still override the policy explicitly with:
5758

5859
```powershell
5960
.\build.ps1 -NodeReuse $true
61+
.\build.ps1 -NodeReuse $false
6062
```
6163

6264
## Troubleshooting (common)

.vscode/settings.json

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,9 @@
4444
// Explicitly allow read verbs (view/list/status/search) and block chained/redirection patterns.
4545
"/^(&\\s+)?gh\\s+(pr|issue|repo|run|release|workflow|label|milestone|project|discussion|gist)\\s+(view|list|status)\\b(?!.*[;&|<>])/i": true,
4646
"/^(&\\s+)?gh\\s+search\\s+(prs|issues|repos|commits|code)\\b(?!.*[;&|<>])/i": true,
47-
// gh api is approved only when no explicit mutating method is specified.
48-
"/^(&\\s+)?gh\\s+api\\b(?!.*\\s(-X|--method)\\s*(POST|PUT|PATCH|DELETE)\\b)(?!.*[;&|<>])/i": true,
47+
// gh api is approved only for non-GraphQL calls when no explicit mutating method is specified.
48+
// gh api graphql always uses POST, so require manual approval for any graphql invocation.
49+
"/^(&\\s+)?gh\\s+api\\b(?!.*\\bgraphql\\b)(?!.*\\s(-X|--method)\\s*(POST|PUT|PATCH|DELETE)\\b)(?!.*[;&|<>])/i": true,
4950

5051
// Python/scripting (consolidated)
5152
"/^(&\\s+)?python\\s+(scripts\\/|-m|\\.github\\/)(?!.*[;&|<>])/": true,

Build/Agent/FwBuildHelpers.psm1

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ function Get-WorktreeMutexName {
146146

147147
$hash = [System.BitConverter]::ToString($hashBytes).Replace('-', '')
148148
$shortHash = $hash.Substring(0, 16)
149-
return "Global\FieldWorks.Worktree.$shortHash"
149+
# Global scope is intentional to allow mutex visibility across processes regardless of session or user (e.g. VS and command line)
150150
return "Global\FieldWorks.Worktree.$shortHash"
151151
}
152152

build.ps1

Lines changed: 70 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,9 @@
4242
is written next to the built executable. Useful for crash investigation.
4343
4444
.PARAMETER NodeReuse
45-
Enables or disables MSBuild node reuse (/nr). Default is false to improve
46-
worktree isolation for concurrent local builds. Set to true to favor speed over isolation.
45+
Controls MSBuild node reuse (/nr). Accepts true, false, or auto.
46+
Default is auto: enable reuse when this repository has a single local worktree,
47+
and disable it when multiple local worktrees exist to improve isolation.
4748
4849
.PARAMETER MsBuildArgs
4950
Additional arguments to pass directly to MSBuild.
@@ -97,6 +98,10 @@
9798
Optional actor label written to the worktree lock metadata (for example: user or agent).
9899
Defaults to the FW_BUILD_STARTED_BY environment variable when set, otherwise 'unknown'.
99100
101+
.PARAMETER SkipWorktreeLock
102+
Internal switch used when build.ps1 is invoked from test.ps1 while the parent test workflow
103+
already owns the same-worktree lock. Skips acquiring/releasing that lock again.
104+
100105
.PARAMETER TailLines
101106
If specified, only displays the last N lines of output after the build completes.
102107
Useful for CI/agent scenarios where you want to see recent output without piping.
@@ -140,7 +145,8 @@ param(
140145
[switch]$BuildAdditionalApps,
141146
[string]$Project = "FieldWorks.proj",
142147
[string]$Verbosity = "minimal",
143-
[bool]$NodeReuse = $false,
148+
[ValidateSet('true', 'false', 'auto')]
149+
[string]$NodeReuse = 'auto',
144150
[string[]]$MsBuildArgs = @(),
145151
[string]$LogFile,
146152
[int]$TailLines,
@@ -158,6 +164,7 @@ param(
158164
[string]$LocalLcmPath,
159165
[ValidateSet('user', 'agent', 'unknown')]
160166
[string]$StartedBy = 'unknown',
167+
[switch]$SkipWorktreeLock,
161168
[switch]$SkipDependencyCheck
162169
)
163170

@@ -302,8 +309,64 @@ function Get-BuildStampPath {
302309
return Join-Path $outputDir "BuildStamp.json"
303310
}
304311

312+
function Resolve-NodeReuse {
313+
param(
314+
[Parameter(Mandatory = $true)][string]$Mode
315+
)
316+
317+
$normalizedMode = $Mode.ToLowerInvariant()
318+
if ($normalizedMode -eq 'true') {
319+
return [pscustomobject]@{
320+
Enabled = $true
321+
Source = 'explicit'
322+
Reason = 'requested explicitly'
323+
}
324+
}
325+
326+
if ($normalizedMode -eq 'false') {
327+
return [pscustomobject]@{
328+
Enabled = $false
329+
Source = 'explicit'
330+
Reason = 'requested explicitly'
331+
}
332+
}
333+
334+
$worktreeList = & git worktree list --porcelain
335+
if ($LASTEXITCODE -ne 0) {
336+
Write-Warning 'Failed to inspect git worktrees. Defaulting MSBuild node reuse to false for safety.'
337+
return [pscustomobject]@{
338+
Enabled = $false
339+
Source = 'auto'
340+
Reason = 'git worktree detection failed'
341+
}
342+
}
343+
344+
$worktreeCount = 0
345+
foreach ($line in (($worktreeList | Out-String) -split "`r?`n")) {
346+
if ($line.StartsWith('worktree ')) {
347+
$worktreeCount++
348+
}
349+
}
350+
351+
if ($worktreeCount -le 1) {
352+
return [pscustomobject]@{
353+
Enabled = $true
354+
Source = 'auto'
355+
Reason = 'single local worktree detected'
356+
}
357+
}
358+
359+
return [pscustomobject]@{
360+
Enabled = $false
361+
Source = 'auto'
362+
Reason = "$worktreeCount local worktrees detected"
363+
}
364+
}
365+
305366
try {
306-
$worktreeLock = Enter-WorktreeLock -RepoRoot $PSScriptRoot -Context "FieldWorks build" -StartedBy $StartedBy
367+
if (-not $SkipWorktreeLock) {
368+
$worktreeLock = Enter-WorktreeLock -RepoRoot $PSScriptRoot -Context "FieldWorks build" -StartedBy $StartedBy
369+
}
307370

308371
# Worktree-aware cleanup: only stop conflicting processes related to this repo root.
309372
Stop-ConflictingProcesses -IncludeOmniSharp -RepoRoot $PSScriptRoot
@@ -373,6 +436,7 @@ try {
373436

374437
# Construct MSBuild arguments
375438
$finalMsBuildArgs = @()
439+
$nodeReuseDecision = Resolve-NodeReuse -Mode $NodeReuse
376440

377441
# Parallelism
378442
if (-not $Serial) {
@@ -385,7 +449,7 @@ try {
385449
$finalMsBuildArgs += "/consoleloggerparameters:Summary"
386450

387451
# Node Reuse
388-
$finalMsBuildArgs += "/nr:$($NodeReuse.ToString().ToLower())"
452+
$finalMsBuildArgs += "/nr:$($nodeReuseDecision.Enabled.ToString().ToLower())"
389453

390454
# Properties
391455
$finalMsBuildArgs += "/p:Configuration=$Configuration"
@@ -425,7 +489,7 @@ try {
425489
Write-Host ""
426490
Write-Host "Building FieldWorks..." -ForegroundColor Cyan
427491
Write-Host "Project: $projectPath" -ForegroundColor Cyan
428-
Write-Host "Configuration: $Configuration | Platform: $Platform | Parallel: $(-not $Serial) | Tests: $($BuildTests -or $RunTests)" -ForegroundColor Cyan
492+
Write-Host "Configuration: $Configuration | Platform: $Platform | Parallel: $(-not $Serial) | Tests: $($BuildTests -or $RunTests) | NodeReuse: $($nodeReuseDecision.Enabled) [$($nodeReuseDecision.Source): $($nodeReuseDecision.Reason)]" -ForegroundColor Cyan
429493

430494
if ($BuildAdditionalApps) {
431495
Write-Host "Including optional FieldWorks executables" -ForegroundColor Yellow

test.ps1

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,10 @@ try {
225225
}
226226
else {
227227
Write-Host "Building before running tests..." -ForegroundColor Cyan
228-
& "$PSScriptRoot\build.ps1" -Configuration $Configuration -BuildTests
228+
# This nested call runs while test.ps1 already owns the same-worktree lock.
229+
# Pass -SkipWorktreeLock explicitly so the build path does not depend on the
230+
# current '&' invocation sharing the same thread and Windows mutex recursion.
231+
& "$PSScriptRoot\build.ps1" -Configuration $Configuration -BuildTests -SkipWorktreeLock
229232
if ($LASTEXITCODE -ne 0) {
230233
Write-Host "[ERROR] Build failed. Fix build errors before running tests." -ForegroundColor Red
231234
$script:testExitCode = $LASTEXITCODE

0 commit comments

Comments
 (0)