-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Unify more build and publish logic #3348
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
… Publish="false" from PrivateAssets="all"
aacf9ca to
d253daf
Compare
3659d26 to
5c959e9
Compare
peterhuene
left a comment
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.
Looks great over all. One step closer to removing a bunch of stuff out of the publish targets.
Just a nit and question or two.
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Outdated
Show resolved
Hide resolved
| packageExclusions.UnionWith(_runtimeTarget.GetPlatformExclusionList(platformLibrary, libraryLookup)); | ||
| copyLocalPackageExclusions.UnionWith(_runtimeTarget.GetPlatformExclusionList(platformLibrary, libraryLookup)); | ||
|
|
||
| // If the platform library is not Microsoft.NETCore.App, treat it as an implicit dependency. |
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.
Why don't I see this being removed from:
| // If the platform library is not Microsoft.NETCore.App, treat it as an implicit dependency. |
Do we need both, and why? I was expecting to see things moved in to here from elsewhere. Is that the right way to think about this change?
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.
I think the duplicate logic is still used by ResolveCopyLocalAssets (the old publish asset resolving code that still gets invoked when there are runtime stores being used after these changes).
We might be able to consolidate this bit, but might also not be worth the effort if the intent is to remove ResolveCopyLocalAssets and the AssetsFileResolver (and then by extension, ProjectContext.GetRuntimeLibraries as referenced) in the future.
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.
Yes, the code in ProjectContext is still used by ResolveCopyLocalAssets.
My hope is that we can remove ResolveCopyLocalAssets with further refactoring. This would require supporting the equivalent of PreserveStoreLayout and RuntimeStorePackages in ResolvePackageAssets.
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
Outdated
Show resolved
Hide resolved
3a5f47f to
213e3b0
Compare
…028.2 (dotnet#3348) - Microsoft.NET.Sdk.Web - 5.0.100-alpha.1.19528.2
Implement some of the unification in #3311, but without changing the product behavior.
Determines which assets should be copied local for both build and publish in the
ResolvePackageAssetstask.