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

Microsoft.Extensions.PlatformAbstractions broken on .NET Core 3.0 preview 2 #28613

Closed
jskeet opened this issue Feb 4, 2019 · 17 comments
Closed
Assignees
Milestone

Comments

@jskeet
Copy link

jskeet commented Feb 4, 2019

I understand that Microsoft.Extensions.PlatformAbstractions is "somewhat deprecated" but I suspect there are plenty of class libraries that still depend on it. (The one I'm interested in is Google.Api.Gax; we use it to report the framework version in HTTP headers.)

The expression in question is:

Microsoft.Extensions.PlatformAbstractions.PlatformServices.Default?.Application?.RuntimeFramework?.Version

This has worked fine with .NET Core until now, but under .NET Core 3.0 preview 2, we get an exception:

Unhandled Exception: System.TypeInitializationException: The type initializer for 'Microsoft.Extensions.PlatformAbstractions.PlatformServices' threw an exception. ---> System.Reflection.TargetParameterCountException: Parameter count mismatch.
   at System.Reflection.RuntimeMethodInfo.InvokeArgumentsCheck(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at Microsoft.Extensions.PlatformAbstractions.ApplicationEnvironment.GetEntryAssembly()
   at Microsoft.Extensions.PlatformAbstractions.ApplicationEnvironment..ctor()
   at Microsoft.Extensions.PlatformAbstractions.PlatformServices..ctor()
   at Microsoft.Extensions.PlatformAbstractions.PlatformServices..cctor()
   --- End of inner exception stack trace ---
   at Microsoft.Extensions.PlatformAbstractions.PlatformServices.get_Default()

I would expect .NET Core 3.0 to be compatible with older versions of .NET Core - if it's not, that's a pretty unfortunate story for class library developers.

I've confirmed that I can use System.Reflection.Assembly.GetEntryAssembly().GetCustomAttribute<TargetFrameworkAttribute>().FrameworkName from netstandard1.3 (the .NET Standard version I'm targeting in the class library) - I'll need to do a bit of parsing, but it's all feasible. That only helps users who are able to update to a newer version of my library though - it feels to me like it shouldn't be necessary.

This looks like the code that's throwing in PlatformAbstractions:

var getEntryAssemblyMethod =
    typeof(Assembly).GetMethod("GetEntryAssembly", BindingFlags.Static | BindingFlags.NonPublic) ??
    typeof(Assembly).GetMethod("GetEntryAssembly", BindingFlags.Static | BindingFlags.Public);
return getEntryAssemblyMethod.Invoke(obj: null, parameters: Array.Empty<object>()) as Assembly;

Obviously using reflection invites compatibility issues, but is there any reason we can't preserve a parameterless GetEntryAssembly method for compatibility?

@stephentoub
Copy link
Member

stephentoub commented Feb 4, 2019

Obviously using reflection invites compatibility issues, but is there any reason we can't preserve a parameterless GetEntryAssembly method for compatibility?

There is a parameterless GetEntryAssembly, it's just public, e.g. remove the first line in your repro and make it just:

var getEntryAssemblyMethod =
    typeof(Assembly).GetMethod("GetEntryAssembly", BindingFlags.Static | BindingFlags.Public);
return getEntryAssemblyMethod.Invoke(obj: null, parameters: Array.Empty<object>()) as Assembly;

and it works fine.

I believe the actual issue here is that there's also a private method named GetEntryAssembly with additional arguments. That other overload doesn't exist in .NET Framework, so in .NET Framework your first line above just returns null, whereas in .NET Core 3.0 it's throwing because it's finding a non-public static GetEntryAssembly with a different parameter count.

I would expect .NET Core 3.0 to be compatible with older versions of .NET Core - if it's not, that's a pretty unfortunate story for class library developers.

Private reflection is never guaranteed to be compatible across versions. If that were the bar, we'd be unable to improve many things in .NET.

@jskeet
Copy link
Author

jskeet commented Feb 4, 2019

Private reflection is never guaranteed to be compatible across versions. If that were the bar, we'd be unable to improve many things in .NET.

While that's entirely reasonable on the face of it, it does seem unfortunate for Microsoft to break their own NuGet package. After all, as a user of that package (which is still listed on NuGet) I don't know and shouldn't need to be aware that it uses reflection. I expect a Microsoft package to work with later versions of .NET Core without issues.

Note that the code I quoted before was from the source code of Microsoft.Extensions.PlatformAbstractions.ApplicationEnvironment. Perhaps it would be best to create a new version of that (a patch bump would probably be most appropriate) so that users can just update their dependencies?

@davidfowl
Copy link
Member

davidfowl commented Feb 4, 2019

We don't even build this code anymore.

cc @natemcmaster @Eilon

@jskeet
Copy link
Author

jskeet commented Feb 4, 2019

Does that mean you can't update it and build a patch version though?

It's one thing for Microsoft to say "The way we used to recommend you detect the runtime version in a platform-agnostic way is no longer the recommended way of doing so." (Not that the package description says that.) It's another to say "If you used the recommended way before, you now need to change your code if you want it to keep working on .NET Core 3.0."

@stephentoub
Copy link
Member

While I don't want this to become precedent, it should be easy enough to change the private method to be named something else, assuming that's actually the problem.

@stephentoub stephentoub self-assigned this Feb 4, 2019
@jskeet
Copy link
Author

jskeet commented Feb 4, 2019

Completely agree with it not becoming precedent - and that would be wonderful, thanks :)
To be clear, I'm going to update our code to remove the dependency on Microsoft.Extensions.PlatformAbstractions anyway, but if we can avoid breaking folks unnecessarily, that would be fab.

@davidfowl
Copy link
Member

Does that mean you can't update it and build a patch version though?

It's code, anything is possible, we just don't and haven't for several majors. It would require a bunch of work which means it needs to get prioritized among other feature work and bugs etc. If we do get around to releasing a new version of this, I'd also deprecate all of the APIs. This really existed for a pre-.NET 4.6 world.

To be clear, I'm going to update our code to remove the dependency on Microsoft.Extensions.PlatformAbstractions anyway, but if we can avoid breaking folks unnecessarily, that would be fab.

Great! Even if we were to update the dependency, you'd still need to update the packages that depend on it. Nobody will automatically get the fix.

@jskeet
Copy link
Author

jskeet commented Feb 4, 2019

Great! Even if we were to update the dependency, you'd still need to update the packages that depend on it. Nobody will automatically get the fix.

Yup - but if Stephen changes the name of the method, they won't need to. Removing the dependency is largely a matter of reducing the risk of future breakage.

It might be worth unlisting the package, to reduce future usage from others.

@natemcmaster
Copy link
Contributor

For some context: aspnet/Announcements#237. We deprecated the library 2 years ago. This announcement recommends replacement APIs you can use.

Unlisting packages might help new libraries avoid adding the dependency, but doesn't do much to help existing projects. I think what I'd really like to see is a proper way to mark a package on nuget as deprecated.

@jskeet
Copy link
Author

jskeet commented Feb 4, 2019

+1 for "deprecation on NuGet" in general. There's no way that we should require general .NET Core developers (who aren't explicitly even using ASP.NET Core) to follow announcements on https://github.com/aspnet.

I think a clearer distinction between ASP.NET Core and non-ASP.NET-core packages would be welcome as well. While the package source code is in https://github.com/aspnet, there's nothing obviously ASP.NET-Core-centric about it.
I wouldn't want to speak for other developers, but for me, I'd expect a closer binding between explicitly ASP.NET-Core-centric packages and .NET Core versioning than for more general packages.

@natemcmaster
Copy link
Contributor

Cc @DamianEdwards @davidfowl

I agree with you. The libraries are only "aspnet" from a team ownership perspective. We've considered ways to reposition the Microsoft.Extensions.* packages to clarify that they are not just for AspNetCore apps. I can't promise anything specific, but it is something we are aware of.

@davidfowl
Copy link
Member

There's a plan to move Microsoft.Extensions to the dotnet org but I dunno where we are in that plan. I hope we get there soon. That'll be a big factor in positioning. Next after we get the source moved is the docs.

@am11
Copy link
Member

am11 commented Feb 6, 2019

+1 for "deprecation on NuGet" in general.

https://github.com/NuGet/Home/wiki/Deprecate-packages

@danmoseley
Copy link
Member

@loop-evgeny
Copy link

This is still broken in 3.0 Preview 3 and it prevents me from using the Google.Cloud.BigQuery.V2 package (v1.3.0 - latest available). Please fix this!

System.TypeInitializationException: The type initializer for 'Google.Cloud.BigQuery.V2.BigQueryClientImpl' threw an exception. ---> System.TypeInitializationException: The type initializer for 'Microsoft.Extensions.PlatformAbstractions.PlatformServices' threw an exception. ---> System.Reflection.TargetParameterCountException: Parameter count mismatch.
   at System.Reflection.RuntimeMethodInfo.InvokeArgumentsCheck(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at Microsoft.Extensions.PlatformAbstractions.ApplicationEnvironment.GetEntryAssembly()
   at Microsoft.Extensions.PlatformAbstractions.ApplicationEnvironment..ctor()
   at Microsoft.Extensions.PlatformAbstractions.PlatformServices..ctor()
   at Microsoft.Extensions.PlatformAbstractions.PlatformServices..cctor()
   --- End of inner exception stack trace ---
   at Microsoft.Extensions.PlatformAbstractions.PlatformServices.get_Default()
   at Google.Api.Gax.VersionHeaderBuilder.AppendDotNetEnvironment()
   at Google.Api.Gax.Rest.UserAgentHelper.CreateRequestModifier(Type type)
   at Google.Cloud.BigQuery.V2.BigQueryClientImpl..cctor()
   --- End of inner exception stack trace ---
   at Google.Cloud.BigQuery.V2.BigQueryClientImpl.get_ApplicationName()
   at Google.Cloud.BigQuery.V2.BigQueryClient.CreateImpl(String projectId, GoogleCredential scopedCredentials)
   at Google.Cloud.BigQuery.V2.BigQueryClient.Create(String projectId, GoogleCredential credential)

@jskeet
Copy link
Author

jskeet commented Mar 7, 2019

@loop-evgeny: As a workaround, if you add an explicit dependency on Google.Api.Gax.Rest version 2.6.0, that should fix the issue for you. Later releases of Google.Cloud.BigQuery.V2 will have the updated dependency themselves.

@loop-evgeny
Copy link

That works, thanks a lot!

@jkotas jkotas closed this as completed Mar 16, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 3.0 milestone Feb 1, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants