Converge common msbuild properties and targets #338
Conversation
ViktorHofer
force-pushed
the
CommonMSBuild
branch
from
d7e12d7 to
86e9661
Compare
trylek
left a comment
trylek
left a comment
There was a problem hiding this comment.
Looks good, thanks for cleaning this up. I'll take a look at the extra CoreCLR scripts.
|
Had to force push without changing anything (Arcade update wasn't picked up in a retry). |
Moving common msbuild properties and targets into the repo root.
Consolidating msbuild analyzers logic centrally in the repo root and adding a property switch `EnableAnalyzers` to enable analyzers. We still need to import the Analyzers.props file unconditionally as libraries currently doesn't use the PackageReference logic in ref and src projects and instead uses a depproj to create a props file dynamically.
ViktorHofer
force-pushed
the
CommonMSBuild
branch
from
e2bb94a to
8035f21
Compare
src/coreclr/src/System.Private.CoreLib/System.Private.CoreLib.csproj
Outdated
Show resolved
Hide resolved
| <AssemblyName>System.Private.CoreLib</AssemblyName> | ||
| <AssemblyVersion>5.0.0.0</AssemblyVersion> | ||
| <ExcludeAssemblyInfoPartialFile>true</ExcludeAssemblyInfoPartialFile> | ||
| <GenerateTargetFrameworkAttribute>false</GenerateTargetFrameworkAttribute> |
There was a problem hiding this comment.
Why do we need to generate the target framework atrribute in CoreLib? It is just an extra cruft that nobody cares about.
There was a problem hiding this comment.
We generate the attribute for all assemblies in libraries, even for Private ones like System.Private.Xml. Mainly I did this to follow-up on your comment here: dotnet/corefx#32680 (comment)
There was a problem hiding this comment.
Ah, ok - I forgot about that discussion. As you can tell, I still wonder why we are generating this cruft for inbox assemblies.
There was a problem hiding this comment.
I still wonder why we are generating this cruft for inbox assemblies.
It's funny because in the past you mentioned that we should enable it for all - even the inbox ones. I don't have a strong opinion here but I would like to enable this for the sake of consistency. Also I prefer not opting out of the default sdk behavior if there isn't a strong reason.
Also as a side note, I noticed partners taking a dependency on the attribute, ie vstest for test assemblies: https://github.com/microsoft/vstest/blob/7b6248203164f8ea821f6795632bd22e0b69afb0/src/Microsoft.TestPlatform.ObjectModel/Utilities/AssemblyLoadWorker.cs#L67.
There was a problem hiding this comment.
It's funny because in the past you mentioned that we should enable it for all - even the inbox ones.
I do not think that I have said that. I have said that if all inbox ones have it, CoreLib should have it too.
vstest for test assemblies:
These are not inbox assemblies. And I believe that this path is .NET Framework specific. I do not think this method will ever execute on .NET Core.
There was a problem hiding this comment.
I have said that if all inbox ones have it, CoreLib should have it too.
Thanks for clarifying.
And I believe that this path is .NET Framework specific.
I know there was another code path somewhere for .NET Core but I can't find it right now...
There was a problem hiding this comment.
If you think we should remove the attribute from all inbox assemblies we can do that in a follow-up PR.
ghost
locked as resolved and limited conversation to collaborators
5 participants
Move common msbuild properties and targets into the repo root.
@trylek I don't really understand the difference between dir.common.props/targets and Directory.Build.props/targets. I think we should clean this up so that only the latter exists and is imported everywhere.