-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Remove Publish Output Duplicate Writes #12085
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
Conversation
|
@dsplaisted I've identified and added fixes for the cases I've found where we're seeing double writes to publish output. Do you have any feedback or know of a better approach? |
|
@sfoslund Can you link this with the other related PRs / issues? |
|
@dsplaisted sure, the main one is #10850 and one of the test cases comes from #3904 |
|
@dsplaisted do you mind taking a look at this when you have a chance? |
src/Tasks/Microsoft.NET.Build.Tasks/CheckForDuplicateItemMetadata.cs
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/CheckForDuplicateItemMetadata.cs
Outdated
Show resolved
Hide resolved
|
|
||
| <!-- | ||
| _FilterDuplicateCopyLocalPublishAssets | ||
| Remove CopyLocalPublishAssets items that could overwrite resolved files to publish. |
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.
So ResolvedFileToPublish has precedence over _ResolvedCopyLocalPublishAssets? Why is that? What do each of these items mean?
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.
That's the main reason this PR is a draft, I wanted to check with you which should have precedence. _ResolvedCopyLocalPublishAssets consists of assets from packages that need to be copied for publishing and ResolvedFileToPublish is eventually everything else, so at this point in execution it contains everything calculated here which includes child project items, content, resources, compile items, etc.
For example, in the second test System.Runtime.CompilerServices.Unsafe.dll is coming from both the microsoft.netcore.app.runtime.win-x64 package (which is in the _ResolvedCopyLocalPublishAssets itemgroup) and it is copied as content in the microsoft.testplatform.cli package (which is originally in the None itemgroup and added to the ResolvedFileToPublish itemgroup in GetCopyToPublishDirectoryItems).
Here I'm choosing ResolvedFileToPublish to have precedence but I'm not confident that this is the right choice.
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.
@dsplaisted circling back around to this, do you have any thoughts as to the right behavior here?
As it's getting late in the release cycle and it's possible that there are other cases in which we have duplicte writes that would now produce an error with this change, I'm wondering if it makes more sense to hold off on this and include it in an early preview of 6 so we can get feedback?
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ConflictResolution.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ConflictResolution.targets
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishWithoutConflicts.cs
Outdated
Show resolved
Hide resolved
src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishWithoutConflicts.cs
Show resolved
Hide resolved
@dsplaisted Yes, I found 3 common cases, the two covered by tests added in this PR and one that's covered by this test. |
|
Closing for now, will reopen when we can get back to this. |
No description provided.