This repository has been archived by the owner. It is now read-only.

Consider removing this component completely #50

Closed
natemcmaster opened this Issue Dec 13, 2016 · 25 comments

Comments

Projects
None yet
8 participants
@natemcmaster
Copy link
Member

natemcmaster commented Dec 13, 2016

The API provided in this package has been whittled down so much that it's no longer really useful as a standalone assembly. We can simplify our dependency graph by using .NET APIs such as AppContext.BaseDirectory and System.Runtime.InteropServices.RuntimeInformation, or making this a .Sources package instead.

@natemcmaster natemcmaster changed the title Consdier removing this component completely Consider removing this component completely Dec 22, 2016

@natemcmaster

This comment has been minimized.

Copy link
Member Author

natemcmaster commented Feb 16, 2017

@Eilon @BrennanConroy I think this worth considering for 2.0.

@Eilon

This comment has been minimized.

Copy link
Member

Eilon commented Feb 20, 2017

It seems to have a ton of downloads in even just the last 6 weeks - over 1.2 million: http://www.nuget.org/stats/packages/Microsoft.Extensions.PlatformAbstractions?groupby=Version

Is it just because we have dependencies on it from core components? If so, what are those components?

@davidfowl

This comment has been minimized.

Copy link
Member

davidfowl commented Feb 20, 2017

Lets just never ship an update. As @natemcmaster says, it's been replaced by the BCL APIs which are also in netstandard.

@natemcmaster

This comment has been minimized.

Copy link
Member Author

natemcmaster commented Feb 21, 2017

Is it just because we have dependencies on it from core components? If so, what are those components?

Yes, I think this is more likely the case. It is used in Scaffolding, Hosting, WebSockets, MvcPrecompilation, SignalR, Logging, Localization, and probably a few others I missed in my quick GitHub search.

@hishamco

This comment has been minimized.

Copy link

hishamco commented Mar 27, 2017

Is that mean, there's no code change needed in this repo?!!

@muratg muratg added the 1 - Ready label Mar 29, 2017

@muratg muratg added this to the 2.0.0 milestone Mar 29, 2017

@muratg

This comment has been minimized.

Copy link
Member

muratg commented Mar 29, 2017

@anurse Let's sync up later on this one.

@anurse

This comment has been minimized.

Copy link
Member

anurse commented Mar 31, 2017

Tracking for removal:

Branches and changes made:

  • Caching
  • Common
  • Entropy
  • Hosting
  • Localization
  • Logging
  • MusicStore
  • Mvc
  • Razor
  • Scaffolding
  • ServerTests
  • SignalR
  • WebSockets

Issues Identified

  • Some components require netstandard1.5 in order to use Assembly.GetEntryAssembly: Hosting
@anurse

This comment has been minimized.

Copy link
Member

anurse commented Mar 31, 2017

First major problem is going to be Assembly.GetEntryAssembly which is only publicly exposed on .NET Standard 1.5. We cheat in PlatformAbstractions and call it with reflection since .NET Core does have this method, but if we want to get back on the path of righteousness we'll have to set some repos to a minimum of .NET Standard 1.5 (Hosting is the first I've found)

/cc @davidfowl @Eilon @muratg

@hishamco

This comment has been minimized.

Copy link

hishamco commented Mar 31, 2017

So that's why I asked before, Assembly.GetEntryAssembly & AppContext.TargetFrameworkName needs NET Standard 1.5

@natemcmaster

This comment has been minimized.

Copy link
Member Author

natemcmaster commented Mar 31, 2017

IMO we should take this opportunity to get back on the "path of righteousness". netstandard1.3 does not accurately reflect runtime requirements for this API.

Background: reflecting out Assembly.GetEntryAssembly was introduced as a workaround during the 1.0.0 RC1 release. The API became available in 1.0.0-rc2 but we never updated our code. (see dotnet/corefx#4146)

@Eilon

This comment has been minimized.

Copy link
Member

Eilon commented Mar 31, 2017

@natemcmaster are you suggesting we bump up certain projects from netstandard1.3 to 1.5? If so, which ones, and what down-stream ones would also need to get bumped up?

We're not opposed to raising the min-reqs, but it's also not as simple as just flipping some bits, because there's obvious customer impact on this.

@natemcmaster

This comment has been minimized.

Copy link
Member Author

natemcmaster commented Mar 31, 2017

are you suggesting we bump up certain projects from netstandard1.3 to 1.5?

Yes.

If so, which ones, and what down-stream ones would also need to get bumped up?

Not sure. It all depends on which libraries use GetEntryAssembly.

I expect impact would be minimal. The netstandard1.3 binaries are used on .NET Core like 99.9% of the time. Netstandard1.3 binaries are technically API compatible with other runtimes -- like Mono, Xamarin, .NET Framework 4.6, UWP, and WP -- but very few customers do that. .NET Framework consumers end up with the net451 binary, and very few people are running web servers on UWP, Xamarin, etc.

@Eilon

This comment has been minimized.

Copy link
Member

Eilon commented Mar 31, 2017

netstandard1.5 is .NET 4.6.2, but interestingly, netstandard2.0 is .NET 4.6.1. So if we drop netstandard1.3 we could technically get away with dropping support for only 4.6.0. But, either way, that's a reduction in supported platforms, so I think we would need @DamianEdwards to be on board with that.

@anurse - can you produce a list of packages that would need to bump up so that we can consider what to do with their supported netstandard's?

@anurse

This comment has been minimized.

Copy link
Member

anurse commented Apr 3, 2017

Yeah, I can work on it.

@anurse

This comment has been minimized.

Copy link
Member

anurse commented Apr 14, 2017

Packages we'd need to update:

  • Microsoft.AspNetCore.Hosting, currently targets netstandard1.3;netstandard1.5; delete netstandard1.5
  • Microsoft.AspNetCore.Server.HttpSys, currently targets netstandard1.3;net46; change to netstandard1.5;net46
  • Microsoft.AspNetCore.Server.Kestrel, currently targets netstandard1.3;net46; change to netstandard1.5;net46
  • Microsoft.AspNetCore targets netstandard1.3, it would need to target netstandard1.5
@anurse

This comment has been minimized.

Copy link
Member

anurse commented Apr 14, 2017

Our current plan (according to @davidfowl and I):

  • Remove PlatformAbstractions
  • Change Hosting to use the name of the assembly containing startup (likely a breaking change)
  • Fix other issues as they arise (investigating that now)

Cross-repo changes

  • Remove from Universe (anurse/remove-platform-abstractions)
  • Caching (anurse/remove-platform-abstractions)
  • Common (anurse/remove-platform-abstractions)
  • Entropy (anurse/remove-platform-abstractions)
  • Hosting (anurse/remove-platform-abstractions; aspnet/Hosting#1023): Microsoft.AspNetCore.Hosting.csproj,Microsoft.AspNetCore.Server.IntegrationTesting.csproj,Microsoft.AspNetCore.Hosting.Tests.csproj
  • Localization (anurse/remove-platform-abstractions): Microsoft.AspNetCore.Localization.FunctionalTests.csproj
  • Logging (anurse/remove-platform-abstractions): SampleApp.csproj
  • MetaPackages (anurse/remove-platform-abstractions): Microsoft.AspNetCore.All.csproj
  • MusicStore (anurse/remove-platform-abstractions): MusicStore.E2ETests.csproj
  • Mvc (anurse/remove-platform-abstractions): Microsoft.AspNetCore.Mvc.Core.csproj
  • Razor (anurse/remove-platform-abstractions): Microsoft.AspNetCore.Mvc.Razor.Extensions.Test.csproj
  • Scaffolding (anurse/remove-platform-abstractions): Microsoft.VisualStudio.Web.CodeGeneration.Tools.csproj,Microsoft.VisualStudio.Web.CodeGeneration.Utils.csproj
  • ServerTests (anurse/remove-platform-abstractions): ServerComparison.FunctionalTests.csproj
  • SignalR (anurse/remove-platform-abstractions): Microsoft.AspNetCore.WebSockets.Internal.ConformanceTest.csproj
  • WebSockets (anurse/remove-platform-abstractions): Microsoft.AspNetCore.WebSockets.ConformanceTest.csproj
  • MvcPrecompilation (anurse/remove-platform-abstractions):
  • Build Tools: Microsoft.AspNetCore.BuildTools.ApiCheck? (It doesn't reference it directly and doesn't use it)
  • Remove from Universe
  • Remove from packages.csv
@MichaCo

This comment has been minimized.

Copy link

MichaCo commented Apr 21, 2017

@anurse hey,
Maybe stupid question, but there is another package Microsoft.DotNet.PlatformAbstractions which does kind of similar things, getting the ApplicationBasePath for example.

Is this package also supposed to be removed in 2.0? Or still as dependency in e.g. MVC?
It also has a bunch of code handling the OS Version, maybe this should move to BCL, too? Looks kinda similar to System.Runtime.InteropServices.RuntimeInformation.* to me

@anurse

This comment has been minimized.

Copy link
Member

anurse commented Apr 21, 2017

We're removing all references to both packages in our code. ASP.NET doesn't own the Microsoft.DotNet.PlatformAbstractions package, it lives in https://github.com/dotnet/core-setup/ so I don't know what the plans are for removing it in 2.0

@anurse

This comment has been minimized.

Copy link
Member

anurse commented Apr 21, 2017

In build 24566, there are now no references to Microsoft.Extensions.PlatformAbstractions. There is still one reference to Microsoft.DotNet.PlatformAbstractions through Microsoft.Extensions.DependencyModel

@Eilon

This comment has been minimized.

Copy link
Member

Eilon commented Apr 25, 2017

@anurse safe to close this now? Or anything else pending?

@anurse

This comment has been minimized.

Copy link
Member

anurse commented Apr 26, 2017

Goodbye, PlatformAbstractions...

https://media.giphy.com/media/cjKY0wWlPzLdC/giphy.gif

@anurse anurse closed this Apr 26, 2017

@muratg

This comment has been minimized.

Copy link
Member

muratg commented Apr 26, 2017

YAYY!

@natemcmaster

This comment has been minimized.

Copy link
Member Author

natemcmaster commented Apr 26, 2017

Should we delete files on dev and mark the repo as obsolete?

@hishamco

This comment has been minimized.

Copy link

hishamco commented Apr 26, 2017

It would be nice to make this repo as obsolete

@CoskunSunali

This comment has been minimized.

Copy link

CoskunSunali commented Jun 5, 2017

System.AppDomain.CurrentDomain.SetupInformation.ApplicationName property is said to be the alternative to the ApplicationEnvironment.ApplicationName property.

However, I am unable to find the SetupInformation property on the CurrentDomain property.

I solved the issue by using System.Reflection.Assembly.GetEntryAssembly().GetName().Name though.

Is it expected that I cannot access the SetupInformation property when using the latest preview packages?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.