Replace Inputs/Outputs with stamp existence check for incremental builds - #170
Replace Inputs/Outputs with stamp existence check for incremental builds#170MattKotsenas wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Updates Husky’s MSBuild incremental-install behavior to avoid relying on the .config/dotnet-tools.json path (which changed in .NET SDK 10.0.102), switching to a stamp-file existence check so the restore/install target can reliably skip after first run.
Changes:
- Replace MSBuild
Inputs/Outputsincremental build logic with!Exists(install.stamp)gating. - Introduce/configure a
HuskyRootMSBuild property to derive all paths (stamp, working directory) consistently. - Update attach command generation, tests, and docs to match the new stamp +
HuskyRootapproach.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/HuskyTest/Cli/AttachCommandTests.cs | Updates assertions to validate HuskyRoot + !Exists(stamp) condition and remove Inputs/Outputs expectations. |
| src/Husky/Husky.csproj | Adds HuskyRoot property and switches target gating from Inputs/Outputs to stamp existence. |
| src/Husky/Cli/AttachCommand.cs | Generates HuskyRoot PropertyGroup and a stamp existence condition; removes prior incremental Inputs/Outputs behavior. |
| docs/guide/submodules.md | Updates manual XML example to include HuskyRoot and stamp existence gating. |
| docs/guide/automate.md | Updates manual attach instructions and rationale to the new HuskyRoot + stamp approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1272f93 to
8fb4ba4
Compare
|
Ok, let me release this branch, then it'll be ready for review. Edit: Updated now. @alirezanet, do you mind taking a look whenever you have time? If you have any questions, please let me know. Thanks! |
8fb4ba4 to
f8e90d5
Compare
|
Hi @alirezanet, sorry for the bother, but I wanted to make sure you saw this is now updated and ready for review. If you have any questions, please let me know. Thanks! |
|
Hey @MattKotsenas, |
The target listed the stamp in <FileWrites>, but it lives at the repo root (.husky/_/) outside the project obj/bin dirs, so MSBuild never recorded it in FileListAbsolute.txt. dotnet clean left the stamp in place and the documented "clean re-installs" behavior never worked. Replace the dead <FileWrites> with a HuskyClean target (AfterTargets="Clean") that deletes the stamp.
LoggerEx.logger is a process-global set once at startup in production (husky runs one command per process). The unit tests mutate it per-test, and with xUnit's default cross-class parallelism InstallCommandTests' "Git hooks installed" output leaked into AttachCommandTests' console assertion, failing it ~50% of runs. Serialize the suite via xunit.runner.json, matching the integration project's wiring. No production code is affected.
|
Hi @alirezanet! I apologize for the delay; I had some family issues to attend to. I've been validating this change and can confirm no breaking changes (to the limits of "absence of evidence is not evidence of absence" :)). In testing I discovered that Clean wasn't working correctly, so I also fixed that as part of this PR and added an integration test to validate.
Lastly, I added the xunit config to serialize the tests, otherwise they pollute each other on the shared static Logger. |
| // The stamp is written from a target that runs AfterTargets="Restore", so it is | ||
| // never recorded in the build's FileListAbsolute.txt and `dotnet clean` cannot | ||
| // remove it. Delete it explicitly after Clean so the next build re-installs | ||
| // (e.g. after a tool version bump). | ||
| var cleanTarget = new XElement("Target"); | ||
| cleanTarget.SetAttributeValue("Name", "HuskyClean"); | ||
| cleanTarget.SetAttributeValue("AfterTargets", "Clean"); | ||
| var delete = new XElement("Delete"); | ||
| delete.SetAttributeValue("Files", "$(HuskyRoot).husky/_/install.stamp"); | ||
| cleanTarget.Add(delete); | ||
| doc.Add(cleanTarget); |
| </ItemGroup> | ||
| </Target> | ||
| <Target Name="HuskyClean" AfterTargets="Clean"> | ||
| <Delete Files="$(HuskyRoot).husky/_/install.stamp" /> |
| await c.BashAsync(output, "dotnet build TestProjectBase.csproj"); | ||
| var firstMtime = (await c.BashAsync(output, StampMtime)).Stdout.Trim(); | ||
| firstMtime.Should().NotBe("NONE", "the first build should create the stamp"); | ||
|
|
||
| await c.BashAsync(output, "dotnet build TestProjectBase.csproj"); | ||
| var secondMtime = (await c.BashAsync(output, StampMtime)).Stdout.Trim(); |
Description
Follow-up to #169 (should be merged first or taken together).
.NET SDK 10.0.102 intentionally changed where
dotnet new tool-manifestplaces the manifest file, from.config/dotnet-tools.jsonto the current directory (dotnet/sdk#49296). The SDK's manifestdiscovery walks up the directory tree checking both
dotnet-tools.jsonand.config/dotnet-tools.jsonat each level, so manifests can now live anywhere along the path to root.The MSBuild target's
Inputsattribute had a hardcoded path to.config/dotnet-tools.json. On repos created with SDK 10.0.102+, this file doesn't exist, so MSBuild can't do the timestamp comparison, and the target re-runs every build, defeating the incremental optimization.Rather than adding manifest discovery logic to MSBuild XML, this replaces the
Inputs/Outputsmechanism with a simpler!Exists(stamp)condition. The target runs once (creating the stamp), then skips on all subsequent builds.dotnet cleanremoves the stamp (via
FileWrites), so the next build re-installs. If you update tool versions in the manifest, rundotnet cleanordotnet tool installto pick up the change.Alternatively, we could create another ItemGroup or Target that searches for the dotnet-tools.json file before calling our Husky target. If we want the most deterministic build possible, I'm happy to make that change instead. However, in that case I'd suggest dropping the manual install instructions and pushing everyone more forcefully towards the attach command as part of the same change.
Depends on #169.
Type of change
Checklist