Multi-target more projects used by unit tests#43836
Multi-target more projects used by unit tests#43836RikkiGibson merged 6 commits intodotnet:masterfrom
Conversation
src/Compilers/Test/Resources/Core/Microsoft.CodeAnalysis.Compiler.Test.Resources.csproj
Show resolved
Hide resolved
6330001 to
4ef49c3
Compare
eng/targets/Settings.props
Outdated
| <DisableImplicitNuGetFallbackFolder>true</DisableImplicitNuGetFallbackFolder> | ||
| <ToolsetPackagesDir>$(RepoRoot)build\ToolsetPackages\</ToolsetPackagesDir> | ||
| <RoslynPortableTargetFrameworks>netcoreapp3.1;net472</RoslynPortableTargetFrameworks> | ||
| <RoslynLibraryTargetFrameworks>netcoreapp3.1;netstandard2.0</RoslynLibraryTargetFrameworks> |
There was a problem hiding this comment.
I prefer that we not use MSBuild properties for our TargetFramework groups. It makes the projects themselves harder to read. I do understand I am the one who created the original groups but ended up deleting them later because I didn't really think it was worth the cost here.
| <OutputType>Library</OutputType> | ||
| <RootNamespace>Roslyn.Test.PdbUtilities</RootNamespace> | ||
| <TargetFramework>netstandard2.0</TargetFramework> | ||
| <TargetFrameworks>$(RoslynLibraryTargetFrameworks)</TargetFrameworks> |
There was a problem hiding this comment.
This change means that when you build for netcoreapp3.1 then yes you get the single TF build experience. But when you build for net472 you still build all TF. Why did we not change this to multi-target between net472 and netcoreapp3.1 instead?
There was a problem hiding this comment.
MS.CA.CSharp.Test.Utilities and MS.CA.VisualBasic.Test.Utilities depend on this. We could make those target 'netcoreapp3.1;net472' as well. Let me know if I had the right idea in 56f5b10.
There was a problem hiding this comment.
I would go ahead and move those to netcoreapp3.1;net472. If we're going to do this lets commit to it and do it right.
jaredpar
left a comment
There was a problem hiding this comment.
I don't think we should add new MSBuild property groups here. I rejected this approach with reason when we initially flipped to multi-targeting large parts of our code. It ends up making the consuming code harder to read.
|
Once you get this merged I'll remove the existing MSBuild property here that is leading people down the wrong path here. |
jaredpar
left a comment
There was a problem hiding this comment.
Comment to make GitHub happy
|
@jaredpar do you want me to change new usages of |
Definitely okay with you doing the work 😄 |
|
@jaredpar please let me know if further changes are required to this PR. You still have this one marked 'changes requested'. |
Related to #43805
This change makes it so when you edit sources in Microsoft.CodeAnalysis.CSharp and incrementally build a test project, you only need to build Microsoft.CodeAnalysis.CSharp and everything in between once in netcoreapp3.1 instead of needing to build both netcoreapp3.1 and netstandard2.0 flavors.
When I edit a file in Microsoft.CodeAnalysis.CSharp and incrementally build a test project on my machine, this PR reduces the build time from ~14 seconds to ~8 seconds. Here's the command I used:
.\.dotnet\dotnet.exe msbuild .\src\Compilers\CSharp\Test\Symbol\ -p:TargetFramework=netcoreapp3.1 -p:UseRoslynAnalyzers=false