Conversation
3b08fdb to
cb0dfbb
Compare
| <Target Name="_GetFilesToPackage" DependsOnTargets="$(_DependsOn)"> | ||
| <ItemGroup> | ||
| <_File Include="@(DesktopCompilerArtifact)" TargetDir="tasks/net472"/> | ||
| <_File Include="@(DesktopCompilerResourceArtifact)" TargetDir="tasks/net472"/> | ||
| <_File Include="@(CoreClrCompilerBuildArtifact)" TargetDir="tasks/net6.0"/> | ||
| <_File Include="@(CoreClrCompilerToolsArtifact)" TargetDir="tasks/net6.0"/> | ||
| <_File Include="@(CoreClrCompilerBinArtifact)" TargetDir="tasks/net6.0/bincore"/> | ||
| <_File Include="@(CoreClrCompilerBinRuntimesArtifact)" TargetDir="tasks/net6.0/bincore/runtimes"/> | ||
| <_File Include="@(CoreClrCompilerBuildArtifact)" TargetDir="tasks/net7.0"/> | ||
| <_File Include="@(CoreClrCompilerToolsArtifact)" TargetDir="tasks/net7.0"/> | ||
| <_File Include="@(CoreClrCompilerBinArtifact)" TargetDir="tasks/net7.0/bincore"/> | ||
| <_File Include="@(CoreClrCompilerBinRuntimesArtifact)" TargetDir="tasks/net7.0/bincore/runtimes"/> |
There was a problem hiding this comment.
I think I have found ways to condition most all of these TFM changes except these Microsoft.Net.Compilers.Toolset.Package projects.
We could define a duplicate set of ItemGroups and PropertyGroups in this file for source-build and for not, but it feels wrong to repeat these. Would it work to conditionally define a property like NetTFM at the top of the project and let it be interpreted in these paths?
There was a problem hiding this comment.
I'm not sure we can do this yet. I need to think about it for a bit.
|
@333fred see my comments on the associated issue |
I'm really not a fan of this approach. The conditional |
|
I don't understand why source build must target latest. That means source build is not shipping what we actually ship. It's shipping a distinctly different product. |
Because the 6.0 reference packages contain analyzers, it is not feasible to source-build them. You can no longer treat them as a simple reference only package and build them via source-build-reference-packages. |
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
| <TargetFramework>net6.0</TargetFramework> | ||
| <TargetFramework Condition="'$(DotNetBuildFromSource)' == 'true'">net7.0</TargetFramework> |
There was a problem hiding this comment.
After some discussion with others, we'd prefer net7.0 be added into the existing <TargetFramework> vs. having a new one which is conditioned on $(DotNetBuildFromSource). We still have our concerns about the testing burden increase this creates but we will deal with that in a follow up PR.
Adding the comment here but applies to all instances of this change.
There was a problem hiding this comment.
If we target net6.0 at all, then we will need 6.0 targeting packs which are not available in source build. Would something like this be acceptable?
<TargetFramework>net6.0;net7.0</TargetFramework>
<TargetFramework Condition="'$(DotNetBuildFromSource)' == 'true'">net7.0</TargetFramework>
There was a problem hiding this comment.
No. We've historically found that Condition paired with TargetFramework to be unmaintainable.
There was a problem hiding this comment.
I'm sorry but net6.0 is a reality that we cannot escape. We've also found that conditioned TargetFramweork is a maintainenance nightmare for us.
I'm still perplexed at why we're being forced to ship net7.0 in our TF now. It's never been required before and analyzers / generators have been around since 1.0
|
|
||
| <PropertyGroup> | ||
| <_RoslynTargetDirectoryName Condition="'$(MSBuildRuntimeType)' == 'Core'">net6.0</_RoslynTargetDirectoryName> | ||
| <_RoslynTargetDirectoryName Condition="'$(MSBuildRuntimeType)' == 'Core' AND '$(DotNetBuildFromSource)' != 'true'">net7.0</_RoslynTargetDirectoryName> |
There was a problem hiding this comment.
This does not seem correct. This is the props file that will exist in our NuPkg. That means it's evaluated on the customer machine, not on the build machine. On the customer machine $(DotNetBuildFromSource) won't be a variable.
|
|
||
| <PropertyGroup> | ||
| <_RoslynTargetDirectoryName Condition="'$(MSBuildRuntimeType)' == 'Core'">net6.0</_RoslynTargetDirectoryName> | ||
| <_RoslynTargetDirectoryName Condition="'$(MSBuildRuntimeType)' == 'Core' AND '$(DotNetBuildFromSource)' != 'true'">net7.0</_RoslynTargetDirectoryName> |
There was a problem hiding this comment.
This does not seem correct. This is the props file that will exist in our NuPkg. That means it's evaluated on the customer machine, not on the build machine. On the customer machine $(DotNetBuildFromSource) won't be a variable.
| <CoreClrArtifactsDir>net6.0</CoreClrArtifactsDir> | ||
| <CoreClrArtifactsDir Condition="'$(DotNetBuildFromSource)' == 'true'">net7.0</CoreClrArtifactsDir> |
There was a problem hiding this comment.
I prefer this approach of having a single variable which we set via Condition="'$(DotNetBuildFromSource)' as done in this file vs. what was done in src/CodeStyle/VisualBasic/CodeFixes/Microsoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes.vbproj where an entire statement was duplicated on a Condition.
Overall I think we should do the following. Add variable <RoslynCoreClrTargetFramework> whchi is conditioned between net6.0 and net7.0 based on source build. Then use that variable where we are hard coding net6.0 today. That should be applied here as well as the two other places where we're doing similar work.
|
@lbussell - can you capture the agreed upon direction that was taken instead of this PR and then close this? |
|
Instead of moving Roslyn to net7.0 and significantly increasing the test burden, we decided to strip the analyzers from the Microsoft.NETCore.App.Ref 6.0.0 package and add it to SBRP. This only works because Roslyn doesn't use any analyzers during its build. See dotnet/source-build-reference-packages#438. |
Fixes #62510.
Currently WIP, want this so I have a clean reference build and we can get Roslyn folks to comment on testability concerns.