Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update net7.0 TFM references to net8.0 #66930

Closed
MichaelSimons opened this issue Feb 16, 2023 · 7 comments · Fixed by #67333
Closed

Update net7.0 TFM references to net8.0 #66930

MichaelSimons opened this issue Feb 16, 2023 · 7 comments · Fixed by #67333

Comments

@MichaelSimons
Copy link
Member

In order to be source-build compliant for the .NET 8.0 release. All projects targeting a net7.0 TFM must be upgraded to net8.0. It is preferred that a new NetCurrent Arcade property be utilized as it eliminates this type of maintenance work for every release.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Feb 16, 2023
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@jaredpar
Copy link
Member

In order to be source-build compliant for the .NET 8.0 release. All projects targeting a net7.0 TFM must be upgraded to net8.0.

It is not that simple though. Specifically when doing a normal build of our product, we cannot include net8.0 binaries in our standard packages because that is not compatible with our deployment requirements. When building for source build though the requirements are to only include net8.0 binaries in the package. It will produce a very different NuPkg file.

What all NuPkg files is source build expecting to consume from our repository? That is the starting point for what all needs to change.

Also how are we testing source build? Up until this point we've relied on source build essentially being us building the same product so only minimal smoke testing is really necessary. This new requirement though means that we're building a significantly different product (the NuPkg is considerably different). How is testing occurring now?

It is preferred that a new dotnet/arcade#12161 be utilized as it eliminates this type of maintenance work for every release.

I do not see how this will reduce maintainability for us. These properties are only useful in MSBuild files while we commonly find the need to specify TFM in powershell scripts and C# tool source. That means we end up hard coding what these values mean anyways.

Further it's unclear at what cadence these properties will update in arcade and if those updates our deployment requirements. For example it's uncertain when say NetMinimum would move to net7.0 and whether or not our corresponding VS assets would have moved to net7.0 by that time. Today that is a very explicit decision made with full knowledge of our dependencies in mind, with this proposed model it would come silently as a part of an arcade update.

@jaredpar
Copy link
Member

jaredpar commented Feb 23, 2023

Ran some experiments locally where I did a source build and grepped through the output. These are the DLLs we're producing today:

CodeStyleConfigFileGenerator.dll
csc.dll
CSharpSyntaxGenerator.dll
csi.dll
Microsoft.Build.Tasks.CodeAnalysis.resources.dll
Microsoft.Build.Tasks.CodeAnalysis.dll
Microsoft.CodeAnalysis.resources.dll
Microsoft.CodeAnalysis.dll
Microsoft.CodeAnalysis.CodeStyle.resources.dll
Microsoft.CodeAnalysis.CodeStyle.dll
Microsoft.CodeAnalysis.CodeStyle.Fixes.resources.dll
Microsoft.CodeAnalysis.CodeStyle.Fixes.dll
Microsoft.CodeAnalysis.CSharp.resources.dll
Microsoft.CodeAnalysis.CSharp.dll
Microsoft.CodeAnalysis.CSharp.CodeStyle.resources.dll
Microsoft.CodeAnalysis.CSharp.CodeStyle.dll
Microsoft.CodeAnalysis.CSharp.CodeStyle.Fixes.resources.dll
Microsoft.CodeAnalysis.CSharp.CodeStyle.Fixes.dll
Microsoft.CodeAnalysis.CSharp.Features.resources.dll
Microsoft.CodeAnalysis.CSharp.Features.dll
Microsoft.CodeAnalysis.CSharp.Scripting.resources.dll
Microsoft.CodeAnalysis.TestAnalyzerReference.dll
Microsoft.CodeAnalysis.VisualBasic.resources.dll
Microsoft.CodeAnalysis.VisualBasic.dll
Microsoft.CodeAnalysis.VisualBasic.CodeStyle.resources.dll
Microsoft.CodeAnalysis.VisualBasic.CodeStyle.dll
Microsoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes.resources.dll
Microsoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes.dll
Microsoft.CodeAnalysis.VisualBasic.Features.resources.dll
Microsoft.CodeAnalysis.VisualBasic.Features.dll
Microsoft.CodeAnalysis.VisualBasic.Scripting.dll
Microsoft.CodeAnalysis.VisualBasic.Workspaces.resources.dll
Microsoft.CodeAnalysis.VisualBasic.Workspaces.dll
Microsoft.CodeAnalysis.Workspaces.resources.dll
Microsoft.CodeAnalysis.Workspaces.dll
Microsoft.CodeAnalysis.Workspaces.MSBuild.resources.dll
Microsoft.CodeAnalysis.Workspaces.MSBuild.dll
vbc.dll
VBCSCompiler.dll
vbi.dll

That list feels mostly correct. The inclusion of the .NET Framework ARM64 binaries is wrong as they have no place in source build. Should trim those out.

These are the NuPkg files which are not simply dotnet pack of the DLL above.

Microsoft.CodeAnalysis.Common.4.6.0.nupkg
Microsoft.Net.Compilers.Toolset.4.6.0.nupkg
Microsoft.Net.Compilers.Toolset.Arm64.4.6.0.nupkg
Microsoft.CodeAnalysis.ExternalAccess.AspNetCore.4.6.0-dev.nupkg

Again ARM64 is wrong. Other than that though it's feels right.

@jasonmalinowski jasonmalinowski added Area-Infrastructure and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 24, 2023
@jaredpar jaredpar added the Bug label Feb 27, 2023
@jaredpar jaredpar added this to the C# 12.0 milestone Feb 27, 2023
@jaredpar
Copy link
Member

@MichaelSimons curious about this question

Also how are we testing source build? Up until this point we've relied on source build essentially being us building the same product so only minimal smoke testing is really necessary. This new requirement though means that we're building a significantly different product (the NuPkg is considerably different). How is testing occurring now?

@MichaelSimons
Copy link
Member Author

Source-build is in a way no different than the Microsoft build. We rely on the individual repo's CI legs to do the appropriate testing. As part of the Unified Build work, we hope to improve upon this by having a set of integration tests that can run against the final product build as well as being able to run the repo tests within the VMR.

FYI @mmitche

@mmitche
Copy link
Member

mmitche commented Mar 7, 2023

Interestingly, I trimmed out net472 outputs in source-build and suddenly the Microsoft.Net.Compilers.Toolset package stopped working. Turns out that two (static) files added to the package:

https://github.com/dotnet/roslyn/blob/main/src/NuGet/Microsoft.Net.Compilers.Toolset/AnyCpu/Microsoft.Net.Compilers.Toolset.Package.csproj#L54-L55

      <_File Include="$(MSBuildProjectDirectory)\build\**\*.*" Condition="'$(TargetFramework)' == 'net472'" TargetDir="build" />
      <_File Include="$(MSBuildProjectDirectory)\buildMultiTargeting\**\*.*" Condition="'$(TargetFramework)' == 'net472'" TargetDir="buildMultiTargeting" />

were only added when building for net472. They aren't net472 specific. It's just a "do this once" clause.

@jaredpar
Copy link
Member

As a part of doing this we'll need to move the .NET SDK version used in global.json forward. Right now it's pinned to the 7.0.X SDK set

https://github.com/dotnet/roslyn/blob/main/global.json#L3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants