Move to 8.0 #71507

jaredpar · 2024-01-05T15:21:13Z

This updates our TFMS to recognize that .NET 8 has shipped and .NET 6 is no longer supported.

At the same time I am trying to update our code base to be more driven off of $(NetCurrent) and $(NetPrevious). This is important for source build and VMR work as these properties are often overriden to drive the larger build efforts. Hard coded .NET Core based TFMs interfere with that and cause friction for those teams.

Note: this PR is being discussed in the following teams thread

Please begin the review by reading the changes to the Target Framework Strategy.md file as that largely outlines the change here.

VS Validation Build
VS Test Insertion

docs/contributing/Target Framework Strategy.md

eng/targets/Imports.BeforeArcade.targets

jasonmalinowski · 2024-01-05T19:55:37Z

docs/contributing/Target Framework Strategy.md

+# Picking the right TargetFramework
+Projects in our repository should include the following values in `<TargetFramework(s)>` based on the rules below:


This documentation is great, thanks!

Glad you appreciate it. Really want to try and do this more as I do infra changes so people can better understand the rationale a few months from now.

eng/targets/Settings.props

...Compilers/CSharp/Test/CommandLine/Microsoft.CodeAnalysis.CSharp.CommandLine.UnitTests.csproj

jasonmalinowski · 2024-01-05T20:03:32Z

src/Dependencies/Collections/Microsoft.CodeAnalysis.Collections.Package.csproj

@@ -2,16 +2,13 @@
 <!-- Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. See the LICENSE file in the project root for more information. -->
 <Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
-    <TargetFrameworks>net6.0;netcoreapp3.1;netstandard2.0</TargetFrameworks>
+    <TargetFrameworks>$(NetPrevious);netstandard2.0</TargetFrameworks>


I'm not sure why this had explicit references for net6.0 and netcoreapp3.1 since it appears this is effectively a source package?

Agree. I'm not sure what the point of the multi-target here was other than maybe to get nullable warnings. But that didn't explain why two .NET TFMs.

src/EditorFeatures/Core/Microsoft.CodeAnalysis.EditorFeatures.csproj

...xternalAccess/OmniSharp.CSharp/Microsoft.CodeAnalysis.ExternalAccess.OmniSharp.CSharp.csproj

src/Workspaces/Core/MSBuild/Rpc/RpcClient.cs

jaredpar · 2024-01-09T22:25:26Z

eng/build.ps1

@@ -399,12 +399,12 @@ function TestUsingRunTests() {
  $args += " --configuration $configuration"

  if ($testCoreClr) {
-    $args += " --tfm net6.0 --tfm net7.0 --tfm net8.0"
+    $args += " --tfm net7.0 --tfm net8.0"


This is so so fragile. Save for the integration tests failing I would've missed that I needed to update the TFMS here.

I think this is a complication we don't really need anymore. The only real distinction we need in RunTests is framework or core. Probably going to simplify RunTests to only consider that.

Thanks for tackling that too!

jaredpar · 2024-01-09T22:26:53Z

@jasonmalinowski, @dibarbet, @RikkiGibson PTAL

dibarbet · 2024-01-09T22:38:41Z

eng/targets/TargetFrameworks.props

+    <Otherwise>
+      <PropertyGroup>
+        <NetRoslynToolset>net8.0</NetRoslynToolset>
+        <NetRoslynSourceBuild>net7.0;net8.0</NetRoslynSourceBuild>


Should this point to $(NetVSShared), or is it possible for those to be different?

It's possible for them to be different. Consider ~6 months into the future where

DevKit has moved to net8.0

Roslyn has a .NET 9 SDK preview

At that point $(NetVSShared) will simply be net8.0 but $(NetRoslynSourceBuild) will be net8.0;net9.0

...xternalAccess/OmniSharp.CSharp/Microsoft.CodeAnalysis.ExternalAccess.OmniSharp.CSharp.csproj

jasonmalinowski · 2024-01-10T23:49:51Z

docs/contributing/Target Framework Strategy.md

- Source build: requires us to ship `$(NetCurrent)` and `$(NetPrevious)` in workspaces and below (presently `net8.0` and `net7.0` respectively)
- Visual Studio: requires us to ship `net472` for base IDE components and `net6.0` for private runtime components.
+- .NET SDK: requires us to ship compilers on current servicing target framework (presently `net8.0`)
+- Source build: requires us to ship `$(NetCurrent)` and `$(NetPrevious)` in workspaces and below (presently `net9.0` and `net8.0` respectively)


Suggested change

- Source build: requires us to ship `$(NetCurrent)` and `$(NetPrevious)` in workspaces and below (presently `net9.0` and `net8.0` respectively)

- Source Build: requires us to ship workspaces and below on `$(NetCurrent)` and `$(NetPrevious)` (presently `net9.0` and `net8.0` respectively)

Since the "workspaces and below" might have been what the parenthetical applied to.

jasonmalinowski · 2024-01-10T23:51:12Z

docs/contributing/Target Framework Strategy.md

+
+1. `$(NetRoslynSourceBuild)`: code that needs to be part of source build. This property will change based on whether the code is building in a source build context or official builds. In official builds this will include the TFMs for `$(NetVSShared)`
+2. `$(NetVS)`: code that needs to execute on the private runtime of Visual Studio.
+3. `$(NetVSCode)`: code that needs to execute in DevKit host


Suggested change

3. `$(NetVSCode)`: code that needs to execute in DevKit host

3. `$(NetVSCode)`: code that needs to execute in the DevKit host

jasonmalinowski · 2024-01-10T23:52:07Z

docs/contributing/Target Framework Strategy.md

+4. `$(NetVSShared)`: code that needs to execute in both Visual Studio and VS Code but does not need to be source built.
+5. `$(NetRoslynToolset)`: packages that ship the Roslyn toolset. The compiler often builds against multiple target frameworks. This property controls which of those frameworks are shipped in the toolset packages. This value will potentially change in source builds.
+6. `$(NetRoslynAll)`: code, generally test utilities, that need to build for all .NET runtimes that we support.
+7. `$(NetRoslyn)`:code that needs to execute on .NET but does not have any specific product deployment requirements. For example utilities that are used by our infra, compiler unit tests, etc ...


Suggested change

7. `$(NetRoslyn)`:code that needs to execute on .NET but does not have any specific product deployment requirements. For example utilities that are used by our infra, compiler unit tests, etc ...

7. `$(NetRoslyn)`: code that needs to execute on .NET but does not have any specific product deployment requirements. For example utilities that are used by our infra, compiler unit tests, etc ...

jasonmalinowski · 2024-01-10T23:54:15Z

eng/build.ps1

@@ -399,12 +399,12 @@ function TestUsingRunTests() {
  $args += " --configuration $configuration"

  if ($testCoreClr) {
-    $args += " --tfm net6.0 --tfm net7.0 --tfm net8.0"
+    $args += " --tfm net7.0 --tfm net8.0"


Thanks for tackling that too!

jasonmalinowski · 2024-01-11T00:03:52Z

src/Compilers/Test/Core/Platform/Custom/MetadataSignatureHelper.cs

+// The compiler must emit binary serialization metadata entries even though it doesn't 
+// actually do binary serialization.
+#pragma warning disable SYSLIB0050 


Yeah it's an interesting problem. The technology is being deprecated but compiler still has to emit and read it cause it will live forever in net472

I'm also surprised the compiler needs to do special stuff for this.

jasonmalinowski · 2024-01-11T00:15:13Z

src/Features/Lsif/Generator/Microsoft.CodeAnalysis.LanguageServerIndexFormat.Generator.csproj

@@ -4,7 +4,7 @@
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <RootNamespace>Microsoft.CodeAnalysis.LanguageServerIndexFormat.Generator</RootNamespace>
-    <TargetFrameworks>net7.0;net472</TargetFrameworks>
+    <TargetFrameworks>$(NetRoslyn);net472</TargetFrameworks>


This may be a place where we want a different version property here -- this is a tool that's consumed by the Code Index team, so any change to this requires a change on their side. I've verified with them they're good to take the update to 8.0 (they had already been deploying that as the newer runtime version in the last few weeks), but a change here at least means they should be notified about that.

jasonmalinowski · 2024-01-11T00:19:24Z

src/Tools/IdeCoreBenchmarks/IdeCoreBenchmarks.csproj

    <IsShipping>false</IsShipping>
    <LangVersion>11</LangVersion>
    <ProduceReferenceAssembly>False</ProduceReferenceAssembly>
+    <NoWarn>$(NoWarn);NETSDK1206</NoWarn>


Consider adding a comment here.

jasonmalinowski · 2024-01-11T00:24:40Z

src/Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj

@@ -87,7 +87,7 @@
  </Target>

  <!--
-    Deploy a net472 and net6.0 version of the BuildHost process which will be used depending on the type of project. We will use the deployed version even if
+    Deploy a net472 and $(NetVS) version of the BuildHost process which will be used depending on the type of project. We will use the deployed version even if


Is $(NetVS) guaranteed to be in the set of $(NetRoslynSourceBuild)?

src/Workspaces/Core/MSBuild/Rpc/RpcClient.cs

jasonmalinowski · 2024-01-11T00:37:16Z

src/Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj

    <ItemGroup Condition="'$(DotNetBuildFromSource)' != 'true'">
      <_NetFrameworkBuildHostProjectReference Include="..\..\..\Workspaces\Core\MSBuild.BuildHost\Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj">
        <TargetFramework>net472</TargetFramework>
        <ContentFolderName>BuildHost-net472</ContentFolderName>
      </_NetFrameworkBuildHostProjectReference>
      <_NetFrameworkBuildHostProjectReference Include="..\..\..\Workspaces\Core\MSBuild.BuildHost\Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj">
-        <TargetFramework>net6.0</TargetFramework>
+        <TargetFramework>$(NetVS)</TargetFramework>


Is $(NetVS) guaranteed to be in the set of $(NetRoslynSourceBuild)? And in this case we'd need the lowest version of all of them, or otherwise we could end up with say a 7.0 targeting app that consumes this as a 7.0 library, but the underlying build host is targeting 8.0 and things are broken if the user doesn't have 8.0 on their machine too.

(Longer term, this is probably a very special case where we could leave this as hard-coded since there's little code in this and it's almost entirely just shelling out to MSBuild, but there's a bit more work that needs to happen first.)

jaredpar · 2024-01-11T18:48:31Z

@jasonmalinowski really appreciate your feedback on this PR. The remaining items seem non-blocking by my reading. This is an unwieldy PR and is blocking @tmat. At this point my plan is to merge this PR as is then grab a follow up PR to clean up a few of your remaining comments. Please let me know if there is something you think is critical to get in initially.

jasonmalinowski · 2024-01-11T20:20:36Z

@jaredpar Go for it! My bad -- I meant to send you a message affirming nothing here was blocking and you can go for a merge...and I forgot.

This updates our TFMS to recognize that .NET 8 has shipped and .NET 6 is no longer supported. At the same time I am trying to update our code base to be more driven off of `$(NetCurrent)` and `$(NetPrevious)`. This is important for source build and VMR work as these properties are often overriden to drive the larger build efforts. Hard coded .NET Core based TFMs interfere with that and cause friction for those teams.

Co-authored-by: Jason Malinowski <[email protected]>

jaredpar · 2024-01-12T21:15:03Z

The integration failures appear to all be known / part of the existing catastrophic failures we're seeing in other PRs. Everything else is green so merging.

dotnet-issue-labeler bot added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 5, 2024

jaredpar mentioned this pull request Jan 5, 2024

StandardAssemblyLoader.LoadAssembly does not respect codeBasePath on .NET Core microsoft/vs-mef#439

Closed

jasonmalinowski reviewed Jan 5, 2024

View reviewed changes

jaredpar force-pushed the tf branch from 9997c7d to a265d05 Compare January 9, 2024 04:57

jaredpar commented Jan 9, 2024

View reviewed changes

src/Workspaces/Core/MSBuild/Rpc/RpcClient.cs Outdated Show resolved Hide resolved

jaredpar commented Jan 9, 2024

View reviewed changes

jaredpar marked this pull request as ready for review January 9, 2024 22:26

jaredpar requested review from 333fred, a team and dibarbet as code owners January 9, 2024 22:26

dibarbet reviewed Jan 9, 2024

View reviewed changes

tmat approved these changes Jan 9, 2024

View reviewed changes

dibarbet approved these changes Jan 10, 2024

View reviewed changes

jasonmalinowski approved these changes Jan 11, 2024

View reviewed changes

jaredpar force-pushed the tf branch from cddbeae to 5e4140a Compare January 11, 2024 17:04

build-analysis bot mentioned this pull request Jan 11, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

jaredpar and others added 7 commits January 12, 2024 08:27

Apply suggestions from code review

8f1da7a

Co-authored-by: Jason Malinowski <[email protected]>

Clean up NETSDK1206 warnings

66d5612

NETSDK1206 warnings

2d0edf7

Include a check for duplicate TFMs

429984d

fix

634d6dc

more

63f15f7

jaredpar added 9 commits January 12, 2024 08:28

more

77c6140

more

bd60ea5

more

94870cb

more

d8e5452

Fix BuildBoss checks

56bf3e9

More

e7840ab

more

15e1ca8

more

e60a8a0

more

cc5ade6

jaredpar force-pushed the tf branch from 0aecab7 to cc5ade6 Compare January 12, 2024 16:28

more

7d2a811

jaredpar merged commit 5fad265 into dotnet:main Jan 12, 2024
25 of 31 checks passed

jaredpar deleted the tf branch January 12, 2024 21:15

ghost added this to the Next milestone Jan 12, 2024

This was referenced Jan 18, 2024

[main] Update dependencies from dotnet/sdk dotnet/installer#18238

Closed

Use NetCurrent instead of net8.0 #71229

Closed

Set NetRoslyn value for VMR build #71704

Closed

Cosifne modified the milestones: Next, 17.10 P1 Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to 8.0 #71507

Move to 8.0 #71507

jaredpar commented Jan 5, 2024 •

edited

Loading

jasonmalinowski Jan 5, 2024

jaredpar Jan 9, 2024

jasonmalinowski Jan 5, 2024

jaredpar Jan 8, 2024

jaredpar Jan 9, 2024

jaredpar Jan 9, 2024

jasonmalinowski Jan 10, 2024

jaredpar commented Jan 9, 2024

dibarbet Jan 9, 2024

jaredpar Jan 9, 2024

jasonmalinowski Jan 10, 2024

jasonmalinowski Jan 10, 2024

jasonmalinowski Jan 10, 2024

jasonmalinowski Jan 10, 2024

jasonmalinowski Jan 11, 2024

jaredpar Jan 11, 2024

jasonmalinowski Jan 11, 2024

jasonmalinowski Jan 11, 2024

jasonmalinowski Jan 11, 2024

jasonmalinowski Jan 11, 2024

jasonmalinowski Jan 11, 2024

jaredpar commented Jan 11, 2024

jasonmalinowski commented Jan 11, 2024

jaredpar commented Jan 12, 2024

		# Picking the right TargetFramework
		Projects in our repository should include the following values in `<TargetFramework(s)>` based on the rules below:

	- Source build: requires us to ship `$(NetCurrent)` and `$(NetPrevious)` in workspaces and below (presently `net9.0` and `net8.0` respectively)
	- Source Build: requires us to ship workspaces and below on `$(NetCurrent)` and `$(NetPrevious)` (presently `net9.0` and `net8.0` respectively)

	3. `$(NetVSCode)`: code that needs to execute in DevKit host
	3. `$(NetVSCode)`: code that needs to execute in the DevKit host

	7. `$(NetRoslyn)`:code that needs to execute on .NET but does not have any specific product deployment requirements. For example utilities that are used by our infra, compiler unit tests, etc ...
	7. `$(NetRoslyn)`: code that needs to execute on .NET but does not have any specific product deployment requirements. For example utilities that are used by our infra, compiler unit tests, etc ...

Move to 8.0 #71507

Move to 8.0 #71507

Conversation

jaredpar commented Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Jan 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Jan 11, 2024

jasonmalinowski commented Jan 11, 2024

jaredpar commented Jan 12, 2024

jaredpar commented Jan 5, 2024 •

edited

Loading