Use net7.0 in source-build #62916

crummel · 2022-07-25T19:46:44Z

Currently WIP, want this so I have a clean reference build and we can get Roslyn folks to comment on testability concerns.

lbussell · 2022-08-26T23:44:32Z

src/NuGet/Microsoft.Net.Compilers.Toolset/AnyCpu/Microsoft.Net.Compilers.Toolset.Package.csproj

  <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"/>


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?

I'm not sure we can do this yet. I need to think about it for a bit.

333fred

For many (maybe all?) of these things, I suspect we could just upgrade them to .NET 7 and be done with it. @sharwell and @jaredpar for thoughts.

jaredpar · 2022-08-29T15:49:32Z

@333fred see my comments on the associated issue

#62510 (comment)

lbussell · 2022-09-08T20:02:37Z

@jaredpar, @333fred, could we get a review on this soon?

jaredpar · 2022-09-08T21:00:39Z

<TargetFrameworks Condition="'$(DotNetBuildFromSource)' == 'true'">net7.0;net472</TargetFrameworks>

I'm really not a fan of this approach. The conditional <TargetFrameworks> here make our build more complicated and subvert tooling we have to keep our build clean.

jaredpar · 2022-09-08T21:01:15Z

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.

MichaelSimons · 2022-09-09T15:45:03Z

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.

jaredpar · 2022-09-12T16:11:53Z

src/CodeStyle/Tools/CodeStyleConfigFileGenerator.csproj

@@ -2,6 +2,7 @@
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
+    <TargetFramework Condition="'$(DotNetBuildFromSource)' == 'true'">net7.0</TargetFramework>


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.

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>

No. We've historically found that Condition paired with TargetFramework to be unmaintainable.

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

jaredpar · 2022-09-12T16:18:52Z

src/NuGet/Microsoft.Net.Compilers.Toolset/AnyCpu/build/Microsoft.Net.Compilers.Toolset.props

@@ -3,6 +3,7 @@

  <PropertyGroup>
    <_RoslynTargetDirectoryName Condition="'$(MSBuildRuntimeType)' == 'Core'">net6.0</_RoslynTargetDirectoryName>   
+    <_RoslynTargetDirectoryName Condition="'$(MSBuildRuntimeType)' == 'Core' AND '$(DotNetBuildFromSource)' != 'true'">net7.0</_RoslynTargetDirectoryName>   


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.

jaredpar · 2022-09-12T16:19:13Z

...uGet/Microsoft.Net.Compilers.Toolset/arm64/build/Microsoft.Net.Compilers.Toolset.Arm64.props

@@ -3,6 +3,7 @@

  <PropertyGroup>
    <_RoslynTargetDirectoryName Condition="'$(MSBuildRuntimeType)' == 'Core'">net6.0</_RoslynTargetDirectoryName>   
+    <_RoslynTargetDirectoryName Condition="'$(MSBuildRuntimeType)' == 'Core' AND '$(DotNetBuildFromSource)' != 'true'">net7.0</_RoslynTargetDirectoryName>   


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.

jaredpar · 2022-09-12T16:26:15Z

src/NuGet/Microsoft.Net.Compilers.Toolset/AnyCpu/Microsoft.Net.Compilers.Toolset.Package.csproj

+    <CoreClrArtifactsDir>net6.0</CoreClrArtifactsDir>
+    <CoreClrArtifactsDir Condition="'$(DotNetBuildFromSource)' == 'true'">net7.0</CoreClrArtifactsDir>


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.

MichaelSimons · 2022-09-29T18:00:06Z

@lbussell - can you capture the agreed upon direction that was taken instead of this PR and then close this?

lbussell · 2022-09-30T23:46:01Z

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.

ghost added the Area-Infrastructure label Jul 25, 2022

MichaelSimons mentioned this pull request Aug 9, 2022

Upgrade all repos targeting net6.0 to net7.0 dotnet/source-build#2966

Closed

lbussell force-pushed the upgradeNet7 branch 2 times, most recently from 3b08fdb to cb0dfbb Compare August 10, 2022 16:25

lbussell added 2 commits August 26, 2022 12:00

Target net7.0 for source-build

479469d

Minor fixups to keep non-source-build working

7828758

lbussell force-pushed the upgradeNet7 branch from cb0dfbb to 7828758 Compare August 26, 2022 19:11

lbussell reviewed Aug 26, 2022

View reviewed changes

lbussell marked this pull request as ready for review August 26, 2022 23:44

lbussell requested review from a team as code owners August 26, 2022 23:44

333fred reviewed Aug 26, 2022

View reviewed changes

333fred added the Source-Build Tracking dotnet/source-build related tasks. label Aug 29, 2022

lbussell added 4 commits August 31, 2022 15:41

Merge remote-tracking branch 'upstream/main' into upgradeNet7

083cdbb

Don't include System.Configuration on net6.0

863b535

Output artifacts to different directories depending on TFM

c873ba2

Merge remote-tracking branch 'upstream/main' into upgradeNet7

55a0f60

lbussell requested review from 333fred and a team September 6, 2022 23:04

jaredpar reviewed Sep 12, 2022

View reviewed changes

lbussell closed this Sep 30, 2022

		<CoreClrArtifactsDir>net6.0</CoreClrArtifactsDir>
		<CoreClrArtifactsDir Condition="'$(DotNetBuildFromSource)' == 'true'">net7.0</CoreClrArtifactsDir>

Use net7.0 in source-build #62916

Use net7.0 in source-build #62916

Uh oh!

Conversation

crummel commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Aug 29, 2022

Uh oh!

lbussell commented Sep 8, 2022

Uh oh!

jaredpar commented Sep 8, 2022

Uh oh!

jaredpar commented Sep 8, 2022

Uh oh!

MichaelSimons commented Sep 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelSimons commented Sep 29, 2022

Uh oh!

lbussell commented Sep 30, 2022

Uh oh!

Uh oh!

crummel commented Jul 25, 2022 •

edited

Loading