Restore removed API from Extensions #85846

ericstj · 2023-05-05T23:00:01Z

Fixes #87480
I spent some time seeing what it would look like to bring back API that was present on extensions packages from 2.1-3.1 so that libraries built against these old packages would at least bind against the latest packages.

I stubbed out most of the API that was added back, unless it seemed "easy" to make work or important.

Have a look below and comment about the cases and if you think we should do more (or less).

This covers most of the cases. There's one case I cannot fix:

CP0006: Cannot add interface member 'Microsoft.Extensions.Hosting.IHostBuilder.UseServiceProviderFactory<TContainerBuilder>(System.Func<Microsoft.Extensions.Hosting.HostBuilderContext, Microsoft.Extensions.DependencyInjection.IServiceProviderFactory<TContainerBuilder>>)' to e7\bin\Debug\net4.8\Microsoft.Extensions.Hosting.Abstractions.dll because it does not exist on e2\bin\Debug\net4.8\Microsoft.Extensions.Hosting.Abstractions.dll

This is an added method to an interface. One way to fix this would be to go back and modify any type that implemented that interface to add the method as "public virtual" (luckily the signature doesn't require any new types). I was able to test this and it is sufficient for the runtime to accept that method as satisfying the interface -- even if the implementer didn't see that method as part of the interface at compile time. Chances are there are very few implementations of IHostBuilder out there - so it might be tractable to patch them (or even just ignore them).

dotnet-issue-labeler · 2023-05-05T23:00:09Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-05-05T23:00:23Z

Tagging subscribers to this area: @dotnet/area-extensions-logging
See info in area-owners.md if you want to be subscribed.

Issue Details

I spent some time seeing what it would look like to bring back API that was present on extensions packages from 2.1-3.1 so that libraries built against these old packages would at least bind against the latest packages.

I stubbed out most of the API that was added back, unless it seemed "easy" to make work or important.

Have a look below and comment about the cases and if you think we should do more (or less).

This covers most of the cases. There's one case I cannot fix:

CP0006: Cannot add interface member 'Microsoft.Extensions.Hosting.IHostBuilder.UseServiceProviderFactory<TContainerBuilder>(System.Func<Microsoft.Extensions.Hosting.HostBuilderContext, Microsoft.Extensions.DependencyInjection.IServiceProviderFactory<TContainerBuilder>>)' to e7\bin\Debug\net4.8\Microsoft.Extensions.Hosting.Abstractions.dll because it does not exist on e2\bin\Debug\net4.8\Microsoft.Extensions.Hosting.Abstractions.dll

This is an added method to an interface. One way to fix this would be to go back and modify any type that implemented that interface to add the method as "public virtual" (luckily the signature doesn't require any new types). I was able to test this and it is sufficient for the runtime to accept that method as satisfying the interface -- even if the implementer didn't see that method as part of the interface at compile time. Chances are there are very few implementations of IHostBuilder out there - so it might be tractable to patch them (or even just ignore them).

Author:	ericstj
Assignees:	-
Labels:	`new-api-needs-documentation`, `area-Extensions-Logging`
Milestone:	-

joperezr · 2023-05-05T23:05:49Z

FYI @geeknoid

src/libraries/Microsoft.Extensions.Caching.Memory/ref/Microsoft.Extensions.Caching.Memory.cs

src/libraries/Microsoft.Extensions.Hosting/ref/Microsoft.Extensions.Hosting.cs

src/libraries/Microsoft.Extensions.Logging.Console/ref/Microsoft.Extensions.Logging.Console.cs

tarekgh · 2023-05-05T23:28:19Z

Is it possible to have all re-added obsolete APIs be in one source file inside each component instead of scattering it across the code? using partial classes can do it I guess.

ericstj · 2023-05-06T14:15:11Z

Is it possible to have all re-added obsolete APIs be in one source file inside each component instead of scattering it across the code? using partial classes can do it I guess.

I did demonstrate this method in some of the assemblies, but I'm not sold on that being preferable. Also if you noticed - most of the stuff we brought back was entire new types, very few members removed or renamed.

Rather than look at the content / actual mechanism I obsoleted code, have a look at the entire mess of types we're bringing back and weigh in on which we think would actually be meaningful to bring back.

IMO all the logging stuff looks pretty busted. There is a lot we had to bring back and none of it "works". Even in the current state there are a ton of breaking changes for the types that exist today. I have a hard time believing that folks which depended on this would be unblocked by adding back throwing API. Perhaps I could 1) invest more time in understanding if we could make it functional, 2) make it noop instead of throw and don't use any existing types, 3) cut bringing back all the logging. I'm leaning towards option 2 or 3 here or some version of them with a mix of API. It'd be good to understand usage of all this API. If there are some that were used more than others we can focus on those. I suspect that's the case.

Everything but logging looks pretty "easy". We can bring it back in a way that seems likely to unblock folks that used it.

src/libraries/Microsoft.Extensions.Logging.Console/src/ConsoleLogger.cs

src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCacheOptions.cs

src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationSection.cs

src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/IApplicationLifetime.cs

geeknoid · 2023-05-10T13:52:41Z

@RussKie has been working to get R9 samples working and encountering compat issues related to the fact old R9 had graduated dependencies (where the R9 bits for net462 depended on MS extensions 2.2, but the R9 bits for net7.0 depended on MS extensions 7.0, etc.), whereas the newer R9 now depends exclusively on MS extensions 8.0.

So these R9 samples could be used as a good basic vetting of whether the compat fixes actually make stuff work....

ericstj · 2023-05-11T17:15:03Z

So these R9 samples could be used as a good basic vetting of whether the compat fixes actually make stuff work....

I looked at this failure and its ASP.NETCore 2.x on .NET Framework, I suspect all the blockers will originate from this scenario and less R9's usage of extensions. I was able to unblock it with the bits from this PR, but not certain about the amount of coverage that sample has. Also not sure if we want to focus on this scenario for .NET consumption - it might be better to show usage in classic ASP.NET since that's where more customers will be on that framework.

ericstj · 2023-05-15T17:02:15Z

src/libraries/Microsoft.Extensions.Hosting/src/Internal/ConsoleLifetime.obsolete.cs

+    public partial class ConsoleLifetime : IHostLifetime
+    {
+        [Obsolete("IHostingEnvironment and IApplicationLifetime have been deprecated. Use Microsoft.Extensions.Hosting.IHostEnvironment and IHostApplicationLifetime instead.")]
+        public ConsoleLifetime(IOptions<ConsoleLifetimeOptions> options, IHostingEnvironment environment, IApplicationLifetime applicationLifetime)


This addition is causing many tests to fail with:

Void .ctor(Microsoft.Extensions.Options.IOptions`1[Microsoft.Extensions.Hosting.ConsoleLifetimeOptions], Microsoft.Extensions.Hosting.IHostEnvironment, Microsoft.Extensions.Hosting.IHostApplicationLifetime, Microsoft.Extensions.Options.IOptions`1[Microsoft.Extensions.Hosting.HostOptions], Microsoft.Extensions.Logging.ILoggerFactory) Void .ctor(Microsoft.Extensions.Options.IOptions`1[Microsoft.Extensions.Hosting.ConsoleLifetimeOptions], Microsoft.Extensions.Hosting.IHostingEnvironment, Microsoft.Extensions.Hosting.IApplicationLifetime)

Given this type is internal, and API usage is literally non-existent I'm going to pull this one out:
https://apisof.net/catalog/74559930-0e33-2f5b-82ad-66e031022b84

ericstj · 2023-05-17T22:27:29Z

Some information about using these packages on different versions of .NET.

Framework	In support?	Works with 8.0 packages?	Works with this PR?	Notes
NETFramework (net462+)	yes	yes	yes	No dependencies on extensions by default
ASP.NETCore on NETFramework (net462+)	yes*	no	yes	While this does enable upgrade of extensions packages here, we document that we don't support that upgrade https://dotnet.microsoft.com/en-us/platform/support/policy/aspnet/2.1-packages
netcoreapp2.1	no	no	yes	Avoids problem with 3.x due to calling WebHost, which doesn't register eventlog, instead of generic host
netcoreapp3.1	no	no	somewhat	These packages allow the app to get further, but it fails to start on Windows due to EventLog dropping support for netcoreapp3.1
net5.0	no	somewhat	somewhat	Fails to start on Windows for same reason as 3.1
net6.0	yes	yes	yes
net7.0	yes	yes	yes

So really the only place that this PR is making a difference is on unsupported frameworks or for unsupported upgrades.

geeknoid · 2023-05-17T22:51:49Z

But the point of these changes is not to show that net462 code can work with .NET 8 packages, it's about making it possible for an application developed using 2.2 extensions to build and run when binding to the 8.0 extensions. If there are breaking changes between 2.2 and 8.0, then this is not possible.

So can a net462 app compiled using the 2.2 version of MS Extensions work with the 8.0 version of MS Extensions?

ericstj · 2023-05-18T03:10:25Z

So can a net462 app compiled using the 2.2 version of MS Extensions work with the 8.0 version of MS Extensions?

It depends on what they used. If they didn't use any of the obsoleted API it'd be fine. We need to see what "typical" usage of extensions looks like on net462 that is not ASP.NET Core on NETFramework (which is the only reason for folks to stay on 2.1 -- in which case we are not interested in updating them per above). Also - 2.2 packages have been out of support for many years now. There's really no reason for folks to stay on those versions. There is no "platform affinity" between NETFramework versions and any specific release of Microsoft.Extensions nuget packages, so we shouldn't be thinking about pulling folks forward on NETFramework - those are just the latest packages with the latest features and fixes - just like any other package.

These don't have much usage and their addition could create problems for DI Providers.

ericstj · 2023-06-13T14:36:53Z

Created an API issue to bring this to review today. I don't expect detailed review, but want to make FxDC aware.

Work left for this PR:

Add EditorBrowsable.Never to all obsoleted API
Remove the 2.1 API compat infra. That was only meant to illustrate the compat gaps and will not be something we check in.

This overload has a source compatible replacement, and is only used by one package in all of nuget.org - which already has assets targeting new TargetFrameworks which don't depend on this API.

These were only available as non-obsolete in 2.0 and there is zero usage on nuget.org.

ericstj · 2023-06-14T19:41:36Z

This is now ready for review. The final decision on in/out API and justification can be found here:
MSFT internal xls / public gist
You can also see the differences with what we didn't bring back in this commit from before I removed that compat infra.

joperezr · 2023-06-14T22:06:38Z

This is now ready for review. The final decision on in/out API and justification can be found here: MSFT internal xls / public gist You can also see the differences with what we didn't bring back in this commit from before I removed that compat infra.

This is awesome, thanks a bunch for driving this @ericstj.

@geeknoid @tekian can we do a quick skim on that xls/gist to make sure all of the decisions there make sense?

@RussKie I know that you were previously doing a porting exercise of a sample that required some of these APIs to be brought back, and you used a private build at the time that Eric shared with you. Can we try that exercise again with the latest version of the packages from this PR to ensure everything still works as expected?

ericstj · 2023-06-15T00:23:18Z

You can find a private build of this PR from https://devdiv.visualstudio.com/DevDiv/_artifacts/feed/ericstj-share with the version 8.0.0-preview.6.extCompat.1

geeknoid · 2023-06-15T16:13:15Z

Looks good :-)

ericstj · 2023-06-15T23:21:19Z

@tarekgh can you have a look at the final shape the Source changes are in and let me know if you’d like anything to change?

tarekgh · 2023-06-16T18:33:16Z

src/libraries/Microsoft.Extensions.Logging.Console/src/ConsoleLoggerExtensions.Obsolete.cs

+            }
+        }
+
+        private sealed class NullChangeToken : IChangeToken, IDisposable


NullChangeToken

do we need to mark this as obsolete too? I am seeign we did that with ConsoleLoggerSettingsAdapter

No, it's private. I marked ConsoleLoggerSettingsAdapter obsolete because it needed to use other obsolete API. This type does not, so it doesn't need obsolete.

tarekgh · 2023-06-16T18:36:55Z

src/libraries/Microsoft.Extensions.Logging.Debug/src/DebugLogger.cs

@@ -31,8 +31,7 @@ public DebugLogger(string name)
        /// <inheritdoc />
        public bool IsEnabled(LogLevel logLevel)
        {
-            // If the filter is null, everything is enabled
-            // unless the debugger is not attached
+            // Everything is enabled unless the debugger is not attached


Everything

nit: I agree the old comment is unclear but it was providing extra info that LogLevel.Debug is the highest level before None and that what refer to filter in the old comment.

I don't think so. This type used to have a member named filter. That was removed but the comment was not updated: https://github.com/dotnet/extensions/blob/ab493da045ef1e645ff52675adb177ba7a503ce0/src/Logging/Logging.Debug/src/DebugLogger.cs#L46-L50

src/libraries/Microsoft.Extensions.Options/src/OptionsWrapper.cs

src/libraries/Microsoft.Extensions.Primitives/src/InplaceStringBuilder.cs

tarekgh

LGTM. nice to see returning the compatibility back.

Is there anything else left we decided not to bring?

ericstj · 2023-06-16T20:21:21Z

Is there anything else left we decided not to bring?

The XLS and gist linked above show what we didn't bring back.

ericstj requested review from eerhardt, tarekgh and joperezr May 5, 2023 23:00

dotnet-issue-labeler bot added area-Extensions-Logging new-api-needs-documentation labels May 5, 2023

ghost assigned ericstj May 5, 2023

joperezr reviewed May 5, 2023

View reviewed changes

src/libraries/Microsoft.Extensions.Caching.Memory/ref/Microsoft.Extensions.Caching.Memory.cs Outdated Show resolved Hide resolved

joperezr reviewed May 5, 2023

View reviewed changes

src/libraries/Microsoft.Extensions.Hosting/ref/Microsoft.Extensions.Hosting.cs Outdated Show resolved Hide resolved

joperezr reviewed May 5, 2023

View reviewed changes

src/libraries/Microsoft.Extensions.Logging.Console/ref/Microsoft.Extensions.Logging.Console.cs Outdated Show resolved Hide resolved

ericstj force-pushed the extensionsCompat branch from d871f8f to a34d781 Compare May 6, 2023 15:13

ericstj commented May 6, 2023

View reviewed changes

src/libraries/Microsoft.Extensions.Logging.Console/src/ConsoleLogger.cs Outdated Show resolved Hide resolved

geeknoid reviewed May 7, 2023

View reviewed changes

src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCacheOptions.cs Outdated Show resolved Hide resolved

geeknoid reviewed May 7, 2023

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationSection.cs Outdated Show resolved Hide resolved

geeknoid reviewed May 7, 2023

View reviewed changes

src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/IApplicationLifetime.cs Outdated Show resolved Hide resolved

ericstj force-pushed the extensionsCompat branch 2 times, most recently from 1902ab1 to b691ef1 Compare May 8, 2023 20:04

build-analysis bot mentioned this pull request May 9, 2023

Could not find chrome snapshot folder for Linux_x64 #85949

Closed

ericstj commented May 15, 2023

View reviewed changes

ericstj changed the title ~~Restore missing API from Extensions~~ Restore removed API from Extensions May 19, 2023

Restore missing API from Extensions

01cfe3e

ericstj added 3 commits June 8, 2023 11:03

Update console->config ref dependency

6271dc1

Update Obsolete messages and make error

f2738b3

Suppress compat around Obsolete(..., error: true)

dc7f9e1

ericstj force-pushed the extensionsCompat branch from e1abc09 to dc7f9e1 Compare June 9, 2023 20:21

Fix more obsolete messages

5f9b5dd

build-analysis bot mentioned this pull request Jun 10, 2023

Tracking issue for CI build timeouts #76454

Closed

Remove ConsoleLoggerProvider ctor overloads

dc99729

These don't have much usage and their addition could create problems for DI Providers.

ericstj added 4 commits June 13, 2023 09:01

Omit ConfigurationSection .ctor overload

c29ad96

This overload has a source compatible replacement, and is only used by one package in all of nuget.org - which already has assets targeting new TargetFrameworks which don't depend on this API.

Remove OptionsWrapper members

58a8cf4

These were only available as non-obsolete in 2.0 and there is zero usage on nuget.org.

Mark restored obsoleted API as EditorBrowsable=never

63c233d

Remove extensions 2.1 compat infrastructure

5bdf838

ericstj marked this pull request as ready for review June 14, 2023 19:39

build-analysis bot mentioned this pull request Jun 14, 2023

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

tarekgh reviewed Jun 16, 2023

View reviewed changes

src/libraries/Microsoft.Extensions.Options/src/OptionsWrapper.cs Outdated Show resolved Hide resolved

tarekgh reviewed Jun 16, 2023

View reviewed changes

src/libraries/Microsoft.Extensions.Primitives/src/InplaceStringBuilder.cs Show resolved Hide resolved

tarekgh reviewed Jun 16, 2023

View reviewed changes

src/libraries/Microsoft.Extensions.Primitives/src/InplaceStringBuilder.cs Show resolved Hide resolved

tarekgh approved these changes Jun 16, 2023

View reviewed changes

Remove unused usings

e488389

ericstj merged commit 07def40 into dotnet:main Jun 16, 2023

build-analysis bot mentioned this pull request Jun 16, 2023

NuGet failing with Response status code does not indicate success: 503 (Service Unavailable) dotnet/arcade#11723

Open

5 tasks

ghost locked as resolved and limited conversation to collaborators Jul 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore removed API from Extensions #85846

Restore removed API from Extensions #85846

ericstj commented May 5, 2023 •

edited

Loading

dotnet-issue-labeler bot commented May 5, 2023

ghost commented May 5, 2023

joperezr commented May 5, 2023

tarekgh commented May 5, 2023

ericstj commented May 6, 2023 •

edited

Loading

geeknoid commented May 10, 2023

ericstj commented May 11, 2023

ericstj May 15, 2023

ericstj May 15, 2023

ericstj commented May 17, 2023 •

edited

Loading

geeknoid commented May 17, 2023 •

edited

Loading

ericstj commented May 18, 2023

ericstj commented Jun 13, 2023

ericstj commented Jun 14, 2023

joperezr commented Jun 14, 2023 •

edited

Loading

ericstj commented Jun 15, 2023

geeknoid commented Jun 15, 2023

ericstj commented Jun 15, 2023

tarekgh Jun 16, 2023

ericstj Jun 16, 2023

tarekgh Jun 16, 2023

ericstj Jun 16, 2023

tarekgh left a comment

ericstj commented Jun 16, 2023

Restore removed API from Extensions #85846

Restore removed API from Extensions #85846

Conversation

ericstj commented May 5, 2023 • edited Loading

dotnet-issue-labeler bot commented May 5, 2023

ghost commented May 5, 2023

joperezr commented May 5, 2023

tarekgh commented May 5, 2023

ericstj commented May 6, 2023 • edited Loading

geeknoid commented May 10, 2023

ericstj commented May 11, 2023

ericstj May 15, 2023

Choose a reason for hiding this comment

ericstj May 15, 2023

Choose a reason for hiding this comment

ericstj commented May 17, 2023 • edited Loading

geeknoid commented May 17, 2023 • edited Loading

ericstj commented May 18, 2023

ericstj commented Jun 13, 2023

ericstj commented Jun 14, 2023

joperezr commented Jun 14, 2023 • edited Loading

ericstj commented Jun 15, 2023

geeknoid commented Jun 15, 2023

ericstj commented Jun 15, 2023

tarekgh Jun 16, 2023

Choose a reason for hiding this comment

ericstj Jun 16, 2023

Choose a reason for hiding this comment

tarekgh Jun 16, 2023

Choose a reason for hiding this comment

ericstj Jun 16, 2023

Choose a reason for hiding this comment

tarekgh left a comment

Choose a reason for hiding this comment

ericstj commented Jun 16, 2023

ericstj commented May 5, 2023 •

edited

Loading

ericstj commented May 6, 2023 •

edited

Loading

ericstj commented May 17, 2023 •

edited

Loading

geeknoid commented May 17, 2023 •

edited

Loading

joperezr commented Jun 14, 2023 •

edited

Loading