Skip to content

Conversation

@dsplaisted
Copy link
Member

Fixes #305

@dsplaisted
Copy link
Member Author

@nguerrera @srivatsn

@rainersigwald, can you check to see if it looks like this is integrating with the Clean target correctly? Particularly, is BeforeTargets="_CheckForCompileOutputs" the right way to make sure that the FileWrites items get updated before IncrementalClean runs?


<Target Name="GenerateBuildDependencyFile"
DependsOnTargets="_DefaultMicrosoftNETPlatformLibrary"
BeforeTargets="_CheckForCompileOutputs"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My instinct would be to go before _CleanGetCurrentAndPriorFileWrites instead, since this isn't really compile-specific. But _CheckForCompileOutputs is before that so this is ok. Did you prefer this for a reason?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this cause GenerateBuildDependencyFile to run on Clean? That's not right. We want it to only run on build and log the write then.

Copy link
Member Author

@dsplaisted dsplaisted Nov 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this cause GenerateBuildDependencyFile to run on Clean?

The IncrementalClean target runs during a normal build (CoreBuild depends on it). IncrementalClean depends on _CleanGetCurrentAndPriorFileWrites, which depends on _CheckForCompileOutputs. So it's confusing (which is why I asked for Rainier's review), but I think it's doing the right thing and not causing this to run during Clean.

Did you prefer this for a reason?

I think it was because GenerateBuildDependencyFile also writes files to the output folder, so I thought it might make sense to be before the CopyFilesToOutputDirectory target, which depends on _CheckForCompileOutputs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ that meshes with my reasoning on @nguerrera's question. 👍


<Target Name="GenerateBuildDependencyFile"
DependsOnTargets="_DefaultMicrosoftNETPlatformLibrary"
BeforeTargets="_CheckForCompileOutputs"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this cause GenerateBuildDependencyFile to run on Clean? That's not right. We want it to only run on build and log the write then.

@dsplaisted
Copy link
Member Author

@MattGertz for approval

Scenario

Cleaning a project should delete all files that building it put in the output folder

Bug

#305

Workarounds

Delete files manually, or just ignore that clean doesn't clean everything

Risk

Low - Tests should cover any impacted scenarios

Performance Impact

Low - the change is only writing 3 additional lines to a file that is already written during build, and deleting 3 files during clean

Regression Analysis

Not a regression

@MattGertz
Copy link

Approved.

@dsplaisted dsplaisted merged commit 34aa24b into dotnet:master Nov 11, 2016
@dsplaisted dsplaisted deleted the clean-json-files-305 branch November 11, 2016 23:57
mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants