Modernize tooling #50
Conversation
This ensures line endings in the repo are normalized
- Remove Fake and Paket (use NuGet and normal dotnet build) - Add Directory.Build.props - Use DotNet.ReproducibleBuilds - Add solution files for easy access in VS/Rider - Use GitHub Actions for CI - Add weekly Dependabot scan
global.json
Outdated
| } | ||
| "sdk": { | ||
| "version": "6.0.404", | ||
| "rollForward": "disable" |
There was a problem hiding this comment.
unless there's a really compelling, specific reason to stay on 6.0.404, I would want this line removed.
There was a problem hiding this comment.
I detailed this in the PR description. Did you read that?
There was a problem hiding this comment.
I did - I just vehemently disagree with the position. As the .NET SDK PM I want folks on the absolute latest SDK possible at all times, unless there's a compelling reason to not be, and I didn't see one presented yet.
There was a problem hiding this comment.
I had no idea you were the .NET SDK PM 🙂
Not that it changes my argument, but it's a great learning opportunity for me. So let me be more specific so that you can enlighten me as to why I am probably wrong. (And I can of course remove this line even if we agree to disagree.)
In dotnet/fsharp#13678, the OP experienced builds suddenly breaking on CI. I experienced the same at my workplace, which caused some consternation as it broke all our CI builds and prevented me from publishing updates to our services for some hours while I debugged the issue and fixed the build.
The issue only happens if you follow certain best practices regarding repeatable builds, such as restoring packages with lock files (and maybe other stuff, e.g. added by Dotnet.ReproducibleBuilds). The core of the issue was that the FSharp.Core package shipped with the SDK and the corresponding version of FSharp.Core on NuGet had different content hashes. (As I understand it, this was an error on the F# team's part, the details of which can be read in the thread.) The result was that you could install/restore FSharp.Core locally, but not on build servers, depending on the environment on the dev machines and servers (the .NET SDK version, I guess).
One very simple workaround (that saved my day) was to lock the .NET SDK to the previous (patch) version, which did not have this issue. Hopefully, the issue would then be fixed in the next SDK update. (The issue actually occurred again just after, before they finally fixed it.)
Having been burned by this, I chose to pin the exact SDK version at work, and later also for my OSS projects. My experience was that disabling roll-forward was necessary for repeatable builds, since it broke and caused trouble for us when I didn't. You live, you learn. According to the research I did, pinning the .NET SDK version is something that most people don't do - it's a tradeoff of up-to-date SDKs vs. completely repeatable builds - but since I was actually burned by it, due to factors outside my control, I chose to do it for my projects.
I'm sure there are errors in my reasoning, and I look forward to being corrected and learning more. 🙂 For example, the issue discusses DisableImplicitLibraryPacksFolder, which (despite me having participated actively in that discussion) I still don't fully understand. Reading over it again now, it seems like enabling this would mean that FSharp.Core is always restored from NuGet, never the SDK, and therefore fix similar problems without having to pin the SDK version. Is that correct? Since DisableImplicitLibraryPacksFolder is set to true in this PR, that would mean that pinning (here and in all my other projects, both OSS and at work) is unnecessary.
By the way, why specify a version in global.json at all - or at least, why specify .NET 7, as you requested above? Why not specify the lowest version that will work in global.json, and allow major version roll forward?
There was a problem hiding this comment.
This is an excellent comment, but I will be away for the next couple hours on errands so it'll be a bit before I can give it the response it deserves 😅
There was a problem hiding this comment.
I see. I have now removed that line. However, I have several questions:
- Removing the line defaults
rollForwardtolatestPatch. If we always want to use the latest .NET SDK, should we setlatestMajor? If not, why not, and then what about otherrollForwardvalues? - If we want
latestMajor, then why have aglobal.jsonat all? As I understand it, not having it is the same asrollForwardset tolatestMajor, except that when it's present, we require at least the version specified inglobal.json. Unless we need an SDK version that is so recent that we can't expect contributors or build servers to have it, it seems we can do withoutglobal.json. - If we want
latestMajorand despite my reasoning above still wantglobal.json, why set it to a recent SDK version and not the earliest possible version that will work?
There was a problem hiding this comment.
There are I think three main use cases that global.json helps with (and that we (SDK team) should support and document):
- I want to be on latest all the time - aka delete global.json. This is our stated goal, and anything that prevents users from doing this is A Problem to me.
- I want to establish a minimum bar for contribution to this repository - aka global.json with a version and rollForward set to
latestMajor. This is the next-most ideal state IMO. - I have a very particular bug/regression that the SDK is causing - aka global.json with version and rollForward set to
disabled. This is the least ideal state, and should only be a short-term decision, until the next monthly SDK release.
Ideally there would be some way to express case 2 easily, because it does have value - it clearly declares what this repo needs in order to build.
There was a problem hiding this comment.
I see. Thank you for those clarifications. I am confused, however, since you merged this PR in its current state. I would assume that you either wanted to specify latestMajor in global.json, or else remove global.json entirely. I'm happy to make a PR with one of those changes. Would you like that?
There was a problem hiding this comment.
I didn't see either of them as a blocker - but I think I would like option 2 best for this repo, if you wanted to send that up at your leisure.
I got distracted with Ionide.ProjInfo, since it needs an FCS update before this repo can do an FCS update.
There was a problem hiding this comment.
Just thought of something: Based on the Releases and support for .NET page, there can be breaking changes between major and minor SDK versions. Is it then really a good idea to allow automatic roll-forward using latestMajor or latestMinor? Couldn't that cause problems when breaking changes occur? Also (asking purely for my own interest now), does the use of self-contained apps impact that decision, since AFAIK then the runtime .NET version is determined by the SDK it's built with?
|
RE automatic releases, this is one reason I made Ionide.KeepAChangelog - I can track everything in Changelog.md, seamlessly have that data embedded in my generated packages, and also have a format that has easy integrations with tools from other ecosystems (e.g. Github actions CI releases: https://github.com/fsharp/FsAutoComplete/blob/main/.github/workflows/release.yml#L34-L46) |
cmeeren
force-pushed
the
modernize-tooling
branch
from
88d964c to
d6f742b
Compare
If you want to modify the release steps (currently only NuGet push) and/or trigger (currently a tag that starts with |
|
Do you want to always keep SDK and tool package versions in sync? If so, I can move the version from the |
|
This looks great @cmeeren - thanks for pushing on this. |
Partially fixes #49
Important:
.fsprojfiles toDirectory.Build.props. Let me know and I'll move it.v/. In other words, when wanting to release, simply push a tag likev/1.2.3, and it'll be published. If you want, this can be changed, e.g. so that it always releases when the.fsprojversion is bumped (but that makes it easier to inadvertently publish unfinished releases, so I suggest sticking to tags, which I do for all my projects.)NUGET_API_KEY. Let me know if you are unsure how to set this, and I can guide you. (I cannot do it for you since it requires contributor/owner access to the repo as well as the NuGet package.)You may have to actually "activate" GitHub actions once (and point it to the file in the repo) before the pipeline starts working.Edit: It seems this isn't needed.RELEASE_NOTES.mdpackage consumers have everything they need. If you really want them, it's very easy to create them manually - point it to the relevant tag and copy the release notes fromRELEASE_NOTES.mdto the GH release description. There is probably a way to add them automatically, too, but I don't know anything about that.global.jsonrequires the exact version specified (no roll-forward). Note that this applies to development of the code in this repo, not on the runtime behavior of the tool (which is stillLatestMajor). The reason is to avoid builds breaking because of bugs like FSharp.Core version 6.0.5 has multiple content hashes. dotnet/fsharp#13678. We should strive to always keep theglobal.jsonversion up to date with the latest patch version of the .NET SDK, since AFAIK it's not possible (or at least not easy) to install a lower patch (but same major/minor) version of a .NET SDK than what you have already installed. In any case, that's a change that can be done simply by updating the version number inglobal.json; no other work is necessary. If CI passes (which it will if there are no upstream bugs like the one linked to above), it's fine..fsprojfiles. This breaks the build. Always update the.fsprojpackages in an IDE (or rundotnet restore --force-evaluateafter changing the files) so that the lock files are updated.Note that I had to add a reference to Microsoft.Build.Framework to the CLI project in order to get project loading to work.
Next up after merging this PR is more housekeeping, including Fantomas.