-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix handling of ProjectName.deps.json file in incremental builds #424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix handling of ProjectName.deps.json file in incremental builds #424
Conversation
|
@nguerrera @srivatsn for review @AndyGerlicher @rainersigwald, Does this look like the right way to correctly handle IncrementalClean? |
| BeforeTargets="_CheckForCompileOutputs" | ||
| Condition=" '$(GenerateDependencyFile)' == 'true'"> | ||
| <ItemGroup> | ||
| <FileWrites Include="$(ProjectDepsFilePath)" Condition="Exists('$(ProjectDepsFilePath)')"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't put this in a separate target, add it to GenerateBuildDependencyFile.
MSBuild will manipulate items that it can statically understand even when it skips a target for being up to date. It wasn't doing that before because it can't predict what the task would have output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rainersigwald Thanks for clarifying, I've updated the PR accordingly.
2bdf2c9 to
c2b8803
Compare
|
@dsplaisted can you fill out the escrow template and tag Matt. |
|
I feel like I've seen this output-to-file-writes pattern all over the place (or was it really just here or just impactful here?). Do we need to scrub? |
I originally added to the |
|
@MattGertz for approval ScenarioIncremental build (building twice in a row) shouldn't delete files from output directory. BugWorkaroundsRebuild instead of build. The unit testing team is saying this blocks their scenarios too. RiskLow - Tests should cover any impacted scenarios Performance ImpactLow - the change is only writing 3 additional lines to a file that is already written during build, and deleting 3 files during clean Regression AnalysisThis was a regression introduced in #381 |
Fixes #408
The issue was that if the ProjectName.deps.json file was up to date, then the
GenerateDepsFiletarget would be skipped and the deps.json file would not be included in theFileWritesitem group, causing it to get deleted byIncrementalClean. This change adds a separate target which always runs which adds the deps.json file to theFileWritesgroup even ifGenerateDepsFileisn't run.