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

Update to lts 31 #17

Closed
wants to merge 9 commits into from
Closed

Update to lts 31 #17

wants to merge 9 commits into from

Conversation

tebeco
Copy link

@tebeco tebeco commented Jan 18, 2020

This PR follows the same idea as serilog/serilog-aspnetcore#165
Also, as this nuget is a dependency of Serilog.AspNetCore this would need to be released before serilog/serilog-aspnetcore#165

This PR contains :

  • update the global.json to update the SDK used
  • remove warning for powershell linter
  • Change MsBuild tag used for License and Icon (either already deprecated of being deprecated)
  • cross target netstandard2.0/netcoreapp3.1
    this is questionable because this lib only depends on netstandard2.0
    i used the cross targetting to support both 2.1 and 3.1, doing this wont fix issue if a customer want to use Microsoft.Extensions.xxx 3.1.* from another runtime than netcoreapp3.1

we could also just stop support for 2.x because 3.1 is LTS <=== This would require less complexity here, and customer using runtime/Microsoft.Exntesion.* would just need to pin this dependency to the current release
Note that the fact that a customer does not want to update it own dependency is driven to "pin down" it dependencies as they need evolve.
I have no idea What is Serilog stance on that but this is what Major is for ;)

For now i've tried to do this by using cross targeting, but as said the proper solution for this lib is to drop support for any Microsoft.Extensions.xxx 2.x as 3.1 is LTS since December 2019

@tebeco
Copy link
Author

tebeco commented Jan 18, 2020

It seems i'll nned to add the Setup.ps1 in this repo to be sure that it run dotnet-install properly

Comment on lines +46 to +48
<PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" Version="3.1.*" />
<PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="3.1.*" />
<PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="3.1.*" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these packages are targeted to netstandard2.0. No need to multitargeting <TargetFrameworks>netstandard2.0;netcoreapp3.1</TargetFrameworks>.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that's what was all about my remarks in the comment ;)
That the solution i would prefer too

but that also means that people using AspNetCore 2.x won't resolved this assembly anymore and need to pin it to the current version.

Can you validate that you are ok with it ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that also means that people using AspNetCore 2.x won't resolved this assembly anymore and need to pin it to the current version.

I don't understand.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the Metapackage used for AspNetCore 2.x :
https://www.nuget.org/packages/Microsoft.AspNetCore.App

Open the dependency list, it's built by dependending on Microsoft.Extension.xxxxx 2.x
So if this nuget stays netstandard2.0 (As it should, and as you suggest too), and now depends on Microsoft.Extension.xxxxx 3.x, then not sure what's going to happen.

Again the solution for user still using AspNetCore 2.x would just be to pin this dependency, that's all

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, in essence, we are talking about two incompatible versions of the package for the same netstandard2.0 platform? And they are incompatible because of their dependencies, which changed the major version. Right?

Then this is really some problem. But you should keep in mind that nothing prevents using the new version of this package with 3.1 dependencies in applications that do not use AspNetCore at all. New 3.1 libs are just upgraded libs but still target to netstandard2.0

I would say that AspNetCore 2.x users were initially limited by the ability to evolve into new versions of libraries due to restrictions introduced by the authors of this metapackage.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, in essence, we are talking about two incompatible versions of the package for the same netstandard2.0 platform? And they are incompatible because of their dependencies, which changed the major version. Right?

This is basically a Major version change yes, so in order to be ... up to date, people needs to update :D
I always find it funny to say but that basically it :

  • people using up to date aspnetcore would just get this lib up to date
  • people not updating their own app would just pin this lib, that's all

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, in any case, we should not retarget the package to netcoreapp3.1. Right?

Copy link
Author

@tebeco tebeco Jan 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the decision is

  • to use "SemVer" / "just update" and "consumer needing OLD version of Microsoft (i mean still using 2.x) have to pin this dependency" => then yes
  • To let both aspnetcore 2.x and 3.x consumer => there was never a solution since packaged existed ... so the targetframework here is an opportunity that would only fix it for NetCoreApp3.1 (AspNetCore 3.1 / UWP / WinForm / Wpf / Console using Netcoreapp3.1).

Remember that .Net 5 is goign to be a big game changer for that ;)
As we speak Mono is being merge in dotnet/runtime ;)

Copy link
Author

@tebeco tebeco Jan 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again i insist on the fact that consumer that keeps using aspnetcore 2.x have already pinned their own system, so to my eyes, they would also just pin their dependencies

So yes i'm in favor of target netstandard2.0, and move to PackageReference Microsoft.Extensions.xxxx 3.x
But that's not my decision to take

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's listen to @nblumhardt opinion.

@sungam3r
Copy link
Contributor

Serilog.Extensions.Hosting package has no netcore3.1 specific APIs and can use the latest 3.1 versions of other packages that themselves target netstandard2.0

@tebeco
Copy link
Author

tebeco commented Jan 18, 2020

I'm going to remove netcoreapp3.1 in a new commit that could be removed later once this is validated

@sungam3r
Do i bump the major version in this PR ? or is it part of another process in this repo ?

@sungam3r
Copy link
Contributor

I don't know. @nblumhardt may help.

@nblumhardt
Copy link
Member

Hi @tebeco - thanks for the PRs around dependency versions. I'm just catching up after the break, should be able to loop back around to these, soon. Thanks!

@tebeco
Copy link
Author

tebeco commented Apr 20, 2020

kindly pinging @nblumhardt :D

@poke
Copy link

poke commented Apr 20, 2020

Depending on 3.1.* versions doesn’t seem to be a smart decision here. This will have the effect that on every build/package, the current top version is being used and written to the nupkg. This means that the dependencies change for every patch release of other unrelated packages. I believe that would be a breaking change.

For libraries, you should pick the lowest version that works and let the consumer of the library move up as they need or can. So the versions would only be on 3.1.0 and stay there.

I would also challenge the move from to 3.1 at all. Staying as low as possible is the nice thing to do to allow users that cannot move forward as quickly to continue using the package. By staying on 2.2 you would not only avoid a breaking change with this update (which really isn’t necessary considering that there are no functional changes) but also allow users to continue using this with their 2.2 dependencies. For such a fundamental library I would even suggest staying on 2.1 for as long as you technically can, but I guess for this one that works on top of M.E.Hosting, it may not be feasible to stay that low.

Finally, two minor nits:

  • I would get rid of the global.json. Sticking to a single SDK is generally bad as it will essentially require developers to stick with outdated SDK versions. You could use a rollForward behavior but you could also just skip this completely and expect developers working on this to have a compatible version. Just ensure that a current version runs on the CI and let that have the final say.
  • If you only have a single target framework, don’t use the plural <TargetFrameworks>. That will go an alternative route in the SDK.

@tebeco
Copy link
Author

tebeco commented Apr 20, 2020

will that fix that thanks

On the other hand i really wonder if we can use <TargetFrameworks>netstandard2.0;netstandard2,1</...> then
to helps nuget to differentiate the runtime being targeted to allow 3.1.x resolution

Also, do totally understand that TargetFramework are not done for that

@tebeco
Copy link
Author

tebeco commented Apr 20, 2020

closing, going to raise way too much noise

@tebeco tebeco closed this Apr 20, 2020
@tebeco tebeco deleted the update-to-lts-31 branch April 20, 2020 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants