Target .Net 6 Framework and upgrade additionalSdks to latest version by anwarjaved · Pull Request #1300 · autofac/Autofac

anwarjaved · 2021-12-10T10:11:09Z

No description provided.

anwarjaved · 2021-12-10T10:26:44Z

It Seems .net SDK not there on appveyor which failing build

tillig · 2021-12-11T00:16:59Z

This is going to be possibly more involved than you imagined. I'll start a review for it, but if it sounds like something you're not interested in taking on, it's cool if you want to abandon the PR.

tillig

I think I caught all the stuff that needs to change. Unfortunately, it's not necessarily as straightforward as you might think. You can use Autofac.Extensions.DependencyInjection as a help/guide if you get stuck or wonder what we did elsewhere - that's the only project so far that we've updated to .NET 6.

Note there's not really a reason to update core Autofac that I'm aware of. We updated A.E.DI because there were new Microsoft.Extensions.DependencyInjection features that only exist in .NET 6 that we needed to support. There's nothing specific in .NET 6 that I'm aware of that core Autofac would need. That said, having some of the nicer new language features is helpful, so I'm for it.

tillig · 2021-12-11T00:18:14Z

bench/Autofac.BenchmarkProfiling/Autofac.BenchmarkProfiling.csproj

  <PropertyGroup>
    <OutputType>Exe</OutputType>
-    <TargetFramework>netcoreapp3.1</TargetFramework>
+    <TargetFrameworks>netcoreapp3.1;net5.0;net6.0</TargetFrameworks>


Suggested change

<TargetFrameworks>netcoreapp3.1;net5.0;net6.0</TargetFrameworks>

<TargetFramework>net6.0</TargetFramework>

We don't actually run the benchmarks in every framework - just one, enough to see if something has changed dramatically. Going with just net6 is fine.

tillig · 2021-12-11T00:19:41Z

bench/Autofac.Benchmarks/Autofac.Benchmarks.csproj


  <ItemGroup>
-    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.6.1" />
+    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.0.0" />


I can't make a comment at the top of this file, but if the Autofac.BenchmarkProfiling.csproj changes to net6, this project does, too.

tillig · 2021-12-11T00:20:58Z

global.json

 {
  "sdk": {
-    "version": "5.0.400",
+    "version": "6.0.0",


You need to reference an actual, specific version in global.json. Steal the global.json from Autofac.Extensions.DepdencyInjection, where we already target .NET 6.

tillig · 2021-12-11T00:24:42Z

src/Autofac/Autofac.csproj

+    <TargetFrameworks>netstandard2.1;netstandard2.0;net5.0;net6.0</TargetFrameworks>
    <LangVersion>latest</LangVersion>
    <Nullable>enable</Nullable>
    <NoWarn>$(NoWarn);CS1591;IDE0008</NoWarn>


Down on line 31 you'll see <Features> - I can't make a comment that far down in the PR interface. The way analyzers work in .NET 6 is different than in other frameworks. We have to turn some things off and turn some things on.

<Features> needs to be removed. That's obsolete now.

<AnalysisMode>AllEnabledByDefault</AnalysisMode> needs to be added where the <Features> line is.

This also needs to be done in the test projects that have analyzers. I don't know if they all have <Features> but that <AnalysisMode> line needs to be there.

This may cause some new warnings to show up. All warnings need to be resolved as part of the .NET upgrade - we don't allow builds with warnings. This may be more than you want to take on.

tillig · 2021-12-11T00:26:21Z

src/Autofac/Autofac.csproj

+    <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.1.1" Condition="Exists('$(MSBuildThisFileDirectory)../../.git')">
      <PrivateAssets>All</PrivateAssets>
    </PackageReference>
    <PackageReference Include="StyleCop.Analyzers" Version="1.2.0-beta.205">


Suggested change

<PackageReference Include="StyleCop.Analyzers" Version="1.2.0-beta.205">

<PackageReference Include="StyleCop.Analyzers" Version="1.2.0-beta.376">

The .NET 6 support in StyleCop is constantly changing. This also needs to be updated in the test projects that reference StyleCop.

Updated StyleCop Package

tillig · 2021-12-11T00:28:46Z

src/Autofac/Autofac.csproj


  <ItemGroup>
-    <PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.1">
+    <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="6.0.0" />


This Microsoft.Bcl.AsyncInterfaces package was originally inside a conditional include so it only applied for netstandard2.0. It needs to stay conditional. I don't think it needs to even be updated if it's in the conditional, and that'd be fine. We don't want to force folks to take package updates.

tillig · 2021-12-11T00:30:02Z

test/Autofac.Test.Scenarios.ScannedAssembly/Autofac.Test.Scenarios.ScannedAssembly.csproj

 <Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
-    <TargetFramework>netstandard2.1</TargetFramework>


This assembly is only used as a target for assembly scanning. It should only target the lowest framework required - revert to netstandard2.1.

anwarjaved · 2021-12-11T09:08:02Z

I think I caught all the stuff that needs to change. Unfortunately, it's not necessarily as straightforward as you might think. You can use Autofac.Extensions.DependencyInjection as a help/guide if you get stuck or wonder what we did elsewhere - that's the only project so far that we've updated to .NET 6.

Note there's not really a reason to update core Autofac that I'm aware of. We updated A.E.DI because there were new Microsoft.Extensions.DependencyInjection features that only exist in .NET 6 that we needed to support. There's nothing specific in .NET 6 that I'm aware of that core Autofac would need. That said, having some of the nicer new language features is helpful, so I'm for it.

I am going through Autofac.Extensions.DependencyInjection to understand what else I need to do

anwarjaved · 2021-12-12T13:08:13Z

I think its done now, Please review again @tillig

tillig · 2021-12-12T16:43:20Z

Looks like the build is failing with some of the new analyzer warnings (warnings-as-errors is on in the final build). At first glance the fixes for some of these, while totally correct and valid, may cause some breaking API changes. It may be that we need to put this on a net6 branch or something while we ramp up for a v7 major release. In particular I'm looking at some of the ones where it wants to change to EventHandler<T> - which appears to be one of the new analyzer messages. It also looks like some of our back-compat shims need to get some NET6_0 conditionals.

@alistairjevans do you have a preference on going straight to develop with this or maybe a v7 branch where we can queue up some breaking changes? Thinking v7. There's one [Obsolete] item I need to mark for another issue as part of v7 as well.

tillig · 2021-12-12T16:45:18Z

Hmmmm. Also not sure if doing the EventHandler<T> thing is worth the breaking changes. Maybe do that last. I'm thinking about suppressing that one because the breaking change on event handler types isn't really worth the "clean code" of the EventHandler<T> thing.

alistairjevans · 2021-12-12T16:50:48Z

If we think the .NET6 version will have breaking changes, then yes, a separate branch makes sense. Is the next version going to have to be major to introduce the new obsoleted feature?

From my experience moving to .NET6, there's not a huge motivation to introduce breaking change, it's just nice to get the more performant IL and runtime.

If we have any NET5_0 conditions, we should change them to NET5_0_OR_GREATER instead, to save us having to do this again.

tillig · 2021-12-12T16:56:14Z

I just didn't want to add [Obsolete] warnings to folks in a 0.1.0 update. I've had that happen and it sucks.

I think if we can get away without breaking changes, going to develop with this is fine. I can postpone the [Obsolete] thing for later.

Let's use
[SuppressMessage("CA1003", "CA1003", Justification = "Not changing to EventHandler<T> to avoid breaking changes.")]
to suppress the individual event handler warnings. I'd like to keep the check ON in case we add new events - I'd like to start heading in that direction. But the attribute (from System.Diagnostics.CodeAnalysis) will turn off the individual warnings for those messages and avoid the breaking change. We can decide when or if to change them later.

tillig · 2021-12-12T17:00:13Z

There's also a warning about SegmentedStack where it doesn't like the name. We'll need to suppress that one, too, since it's public and not interesting to break.

[SuppressMessage("CA1711", "CA1711", Justification = "Not changing Stack suffix to avoid breaking changes.")]

tillig · 2022-02-23T17:28:17Z

Thanks for the kickstart here; I've opened a new PR #1306 to take this and finish the remaining work. I'll close this one and we can iterate there as needed.

Target .Net 6 Framework and upgrade additionalSdks to latest version

da8dd9b

tillig requested changes Dec 11, 2021

View reviewed changes

anwarjaved added 2 commits December 11, 2021 14:28

Fix Code Review Fixes

5dc7e58

Code Review Fixes

58c21e9

Remove Unused Dependecies in .net 5 and 6

efda800

tillig mentioned this pull request Dec 21, 2021

Simple question: #1304

Closed

tillig mentioned this pull request Feb 23, 2022

.NET 6 Support #1306

Merged

tillig closed this Feb 23, 2022

	<TargetFrameworks>netcoreapp3.1;net5.0;net6.0</TargetFrameworks>
	<TargetFramework>net6.0</TargetFramework>

	<PackageReference Include="StyleCop.Analyzers" Version="1.2.0-beta.205">
	<PackageReference Include="StyleCop.Analyzers" Version="1.2.0-beta.376">

Uh oh!

Conversation

anwarjaved commented Dec 10, 2021

Uh oh!

anwarjaved commented Dec 10, 2021

Uh oh!

tillig commented Dec 11, 2021

Uh oh!

tillig left a comment

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!

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!

anwarjaved commented Dec 11, 2021

Uh oh!

anwarjaved commented Dec 12, 2021

Uh oh!

tillig commented Dec 12, 2021

Uh oh!

tillig commented Dec 12, 2021

Uh oh!

alistairjevans commented Dec 12, 2021

Uh oh!

tillig commented Dec 12, 2021

Uh oh!

tillig commented Dec 12, 2021

Uh oh!

tillig commented Feb 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants