Reduce DG Spec Write Frequency #6259
Conversation
microsoft-github-policy-service
bot
added
the
Status:No recent activity
nkolev92
force-pushed
the
dev-nkolev92-reduceDgSpecWriteFrequency
branch
from
0c548cb to
4e3bfac
Compare
microsoft-github-policy-service
bot
removed
the
Status:No recent activity
| @@ -412,11 +415,13 @@ private void InitializeTelemetry(TelemetryActivity telemetry, int httpSourcesCou | |||
| cacheFile, | |||
| _request.Project.RestoreMetadata.CacheFilePath, | |||
| _request.ProjectStyle, | |||
| restoreTime.Elapsed), cacheFile); | |||
| restoreTime.Elapsed), noOpCacheFileEvaluation, cacheFile); | |||
There was a problem hiding this comment.
nit: newlines for these parameters
| public async Task RestoreResult_Commit_WithExistingDGSpecAndDidDGHashChange_WritesDependencyGraphSpec(bool didDGHashChange) | ||
| { | ||
| // Arrange | ||
| using (var td = TestDirectory.Create()) |
There was a problem hiding this comment.
Can the simple using statement be used? https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0063
| [Theory] | ||
| [InlineData(true)] | ||
| [InlineData(false)] | ||
| public async Task RestoreResult_Commit_WithExistingDGSpecAndDidDGHashChange_WritesDependencyGraphSpec(bool didDGHashChange) |
There was a problem hiding this comment.
nit: It'd be good to get away from repeating test class names in test method names.
non-nit: The criteria isn't clear here. I read the assertions as it should only write when the DGHash changed, because it already existed. Can the test name be clearer?
| public async Task RestoreResult_Commit_WithExistingDGSpecAndDidDGHashChange_WritesDependencyGraphSpec(bool didDGHashChange) | |
| public async Task CommitAsync_WithExistingDGSpec_OnlyWritesWhen_DidDGHashChangeIsTrue(bool didDGHashChange) |
| var path = Path.Combine(td, "project.assets.json"); | ||
| var cachePath = Path.Combine(td, "project.csproj.nuget.cache"); | ||
| var dgSpecPath = Path.Combine(td, "project1.nuget.g.dgspec.json"); | ||
| File.WriteAllText(dgSpecPath, "{}"); |
There was a problem hiding this comment.
I'm guessing we don't have much choice in implementing this test without writing to disk? I quickly checked ProjectTestHelpers in Test.Utility and didn't see a utility to avoid this write.
Bug
Fixes: NuGet/Home#14135
Description
When a restore is completed successfully, it has dirty checks for the assets file and the nuget.g.props and nuget.g.targets.
This is important because these 3 trigger a DTB.
However, dgspec.json does not have a dirty check and that leads to a dgspec write when someone runs something as simple as rebuild in VS.
The "dgspec.json" content is what's used for the no-op hash, so using that value to determine whether to write should be good enough of heuristic.
PR Checklist
Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.