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

Should M.E.DependencyModel only target netstandard2.0+ going forward? #3425

Closed
ahsonkhan opened this issue Jan 25, 2019 · 10 comments · Fixed by #34296
Closed

Should M.E.DependencyModel only target netstandard2.0+ going forward? #3425

ahsonkhan opened this issue Jan 25, 2019 · 10 comments · Fixed by #34296
Assignees
Milestone

Comments

@ahsonkhan
Copy link
Member

M.E.DependencyModel currently supports several TFMs which means we have to maintain the use of two JSON APIs (System.Text.Json for netstandard2.0+ and Newtonsoft.Json for < netstandard2.0).

https://github.com/dotnet/core-setup/blob/63a01b08e5d1d1a6b8544f598b3d3bda76e6e424/src/managed/Microsoft.Extensions.DependencyModel/Microsoft.Extensions.DependencyModel.csproj#L6

We should consider only targeting netstandard2.0 for the upcoming major release and simplify our dependencies.

VS requires 4.7.2 which is compatible with netstandard2.0.

This effort goes in conjunction with dotnet/announcements#90 and helps remove the dependency on Json.NET more cleanly: https://github.com/dotnet/core-setup/issues/3311

Related PRs:

cc @joshfree, @glennc, @eerhardt, @terrajobst, @jaredpar, @ericstj, @davidfowl, @livarcocc

@terrajobst
Copy link
Member

terrajobst commented Jan 25, 2019

Dropping a platform is a breaking change for your consumers.

For BCL-level packages we generally don't do that. However, we often only have the current source be on latest and harvest the old binaries from the previously uploaded package. This reduces the maintenance burden. We could talk to the NuGet team about generalizing this during pack.

I don't have opinions on whether or not it's acceptable for M.E.DependencyModel. I don't consider DI to be a BCL-level component, so maybe dropping a platform is acceptable here.

@jaredpar
Copy link
Member

In case it's relevant Roslyn decided to move to netstandard20 only dropping our netstandard1.3 platforms in the NuPkg. This was a result of VS moving to require net472 hence making netstandard20 an option. The benefits of targeting it were high enough we felt it justified the dropping support for netstandard1.3.

@terrajobst
Copy link
Member

The benefits of targeting it were high enough we felt it justified the dropping support for netstandard1.3.

That sounds a like a non-sequitur to me. You can offer .NET Standard 2.0 without dropping 1.x. Presumably your unstated position is that you don't want the support burden of doing both, so if you had to pick only one, you felt justified with choosing 2.0?

@jaredpar
Copy link
Member

That sounds a like a non-sequitur to me. You can offer .NET Standard 2.0 without dropping 1.x

Yes / No. Consider that many of our dependencies were moving to netstandard20 exclusively. That forced our hand a bit. But if we had all collectively decided that "hey we want to keep netstandard1.3 around" then as a group we could have all decided to keep it. But we didn't because ...

Presumably your unstated position is that you don't want the support burden of doing both, so if you had to pick only one, you felt justified with choosing 2.0?

Correct. The burden of maintaining netstandard1.1 is quite high in our setup.

@terrajobst
Copy link
Member

Correct. The burden of maintaining netstandard1.1 is quite high in our setup.

That's fair.

@eerhardt
Copy link
Member

I believe DependencyModel is in a similar position as Roslyn here - the burden of maintaining netstandard1.x and net451 implementations is not worth the value. The main consumers of DependencyModel are ASP.NET and the .NET Core SDK. Neither of those need a netstandard1.x or net451 implementation, AFAIK. (@davidfowl @natemcmaster @nguerrera @dsplaisted - please correct me if I'm wrong). There are other consumers of DependencyModel, one of which is explicitly asking for dropping Newtonsoft as a dependency.

Consider that many of our dependencies were moving to netstandard20 exclusively.

Again, we have the same situation here - we are making DependencyModel depend on the new JSON parsing/writing source package, which is netstandard2.0 exclusive to my knowledge.


Unless we get customer feedback that DependencyModel 3.0 must keep targeting net451, and netstandard1.x, I think we are justified in dropping those TFMs and only providing a netstandard2.0 implementation.

@natemcmaster
Copy link
Contributor

Confirming what @eerhardt said - aspnet doesn't need netstandard1.x. I'm in favor of dropping it unless we get real feedback that lower TFMs are important for 3.0.

@nguerrera
Copy link
Contributor

For 3.0+, sdk requires something that is net472 and netcoreapp3.0 compatible. So netstandard2.0 is fine for us too.

@eerhardt
Copy link
Member

eerhardt commented Apr 3, 2019

Setting the milestone to Future as this isn't required for 3.0. However, if someone wanted to do the necessary work for 3.0, we can pull it in to 3.0.

@msftgits msftgits transferred this issue from dotnet/core-setup Jan 30, 2020
@msftgits msftgits added this to the Future milestone Jan 30, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@eerhardt eerhardt added help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Feb 25, 2020
@eerhardt eerhardt self-assigned this Mar 30, 2020
@eerhardt eerhardt removed the help wanted [up-for-grabs] Good issue for external contributors label Mar 30, 2020
@eerhardt eerhardt modified the milestones: Future, 5.0 Mar 30, 2020
@eerhardt
Copy link
Member

I plan on doing this as part of moving Microsoft.Extensions.DependencyModel from src\installer to src\libraries.

We can add back the netstandard1.x targets in the future, if deemed necessary. But please speak up early if you know that only supporting netstandard2.0 is going to be a problem.

eerhardt added a commit to eerhardt/runtime that referenced this issue Apr 1, 2020
Moving DependencyModel to the same folder and infrastructure as the rest of our libraries, and out of the installer folder.

I also dropped support for anything below netstandard2.0 at this time.

Contributes to dotnet#3470
Fix dotnet#3425
eerhardt added a commit that referenced this issue Apr 4, 2020
* Move DependencyModel to libraries

Moving DependencyModel to the same folder and infrastructure as the rest of our libraries, and out of the installer folder.

I also dropped support for anything below netstandard2.0 at this time.

Contributes to #3470
Fix #3425

* Exclude DependencyModel in shims and package checks since it has a duplicated type with System.Collections.

* Harvest previous TFM assets from the previously shipped package.

This also means we start building DependencyModel for net461 to ensure full framework support works correctly, and doesn't pick up the old net451 asset.

* Fix unit tests to consistently pass on core and netfx.

* Add back HashCodeCombiner as obsolete.

* Fix DependencyModel pkg build to reference the correct version of Newtonsoft.Json for the harvested package assets.

* Adding IgnoredTypes for Serialization.Primitives in netcoreapp1.0 and netcoreapp1.1

Co-authored-by: Jose Perez Rodriguez <joperezr@microsoft.com>
@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

Successfully merging a pull request may close this issue.

8 participants