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

Pin .NET Core SDK to 3.1.* feature band and patch level #24

Merged
merged 1 commit into from
May 27, 2020

Conversation

ralphhendriks
Copy link

Update the pinned .NET Core SDK to the 3.1.3?? feature band. Do not allow prereleases.

@ralphhendriks
Copy link
Author

ralphhendriks commented May 26, 2020

Ok, this does not compile on AppVeyor until AppVeyor updates the Visual Studio 2019 image (see https://www.appveyor.com/updates/).
My local .NET SDK got updated (I am using Chocolatey to update), hence the PR.
Maybe we should be less restrictive and pin the .NET SDK using the LatestMinor option, pinning to 3.1.* (see https://docs.microsoft.com/en-us/dotnet/core/tools/global-json)?
Any thoughts?

@andrewlock
Copy link
Collaborator

Yeah, it's an annoying tradeoff - it depends if you prioritise user (contributor) experience, in which case you should pin to the minimal version possible and allow floating higher; or build repeatability, in which case you should lock it down hard to a minor version.

Personally, I'm very much in favour of the former, but I know some people are passionate about the latter 🙂

@ralphhendriks
Copy link
Author

@andrewlock I agree with your preference for the former (i.e. relaxing the SDK version constraints). The Microsoft documentation page that I referenced above even states:

In general, you want to use the latest version of the SDK tools, so no global.json file is needed. In some advanced scenarios, you might want to control the version of the SDK tools

I couldn't figure out from the repo's history why the global.json has been added in the first place, probably there was a valid reason related to AppVeyor builds or past issues at the time .NET Core was updated from 2.* to 3.*. If we don't have a clear advantage/use case for having global.json in the project right now, I would argue to delete the file completely. What'd you think?

@adamchester
Copy link
Member

I’d go with latestFeature https://docs.microsoft.com/en-us/dotnet/core/tools/global-json?tabs=netcore3x#rollforward

We did the same over in serilog/serilog

Pin the .NET Core SDK version to 3.1.*, not allowing prerelease
versions.
@ralphhendriks
Copy link
Author

@adamchester I have changed the PR accordingly. Sounds like a reasonable compromise for now. It would still be interesting to hear the reason for having global.json at all. Maybe the reason why it has been added is no longer relevant.

@adamchester
Copy link
Member

I can’t say why it was added here.

However, I often add it (global json and pinning), and my reasons are to help inform myself and others which versions are/were required, and to help keep CI builds more (but not necessarily 100%) repeatable.

It’s a trade off for sure. Reasonable people can have different opinions. I personally don’t always fall into the camp of pinning, but I usually do.

@ralphhendriks ralphhendriks changed the title Pin .NET Core SDK to 3.1.3?? feature band Pin .NET Core SDK to 3.1.* feature band and patch level May 26, 2020
@nblumhardt
Copy link
Member

Behavior's changed a bit as .NET SDKs have evolved in the past few years. global.json has been a bit of a defensive move - but we've only applied it to the most critical/core chunks of Serilog. The fear without it is that changes to the CI infra (AppVeyor images) might at some point subtly change the package structure or dependency graph in an unexpected way. Maybe we can relax and remove it at some future point. The PR looks like a good compromise, for now, though!

@nblumhardt nblumhardt merged commit 7ba1560 into serilog:dev May 27, 2020
@nblumhardt nblumhardt mentioned this pull request May 27, 2020
@ralphhendriks ralphhendriks deleted the update-sdk-version branch May 27, 2020 06:40
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