-
Notifications
You must be signed in to change notification settings - Fork 583
Further attempts at building with .NET 8 #2852
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
base: master
Are you sure you want to change the base?
Conversation
| let scenarioPath = resolvePath scenario "" | ||
| // dotnet tool install --version 5.19.0-alpha.local.1 fake-cli --add-source /e/Projects/FAKE/release/dotnetcore/ | ||
|
|
||
| // Work around https://github.com/dotnet/sdk/issues/40655 by specifying the tool manifest explicitly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of tests in previous runs seemed to be failing with dotnet/sdk#40655, whereby installing the dotnet tool in a subdirectory that contains a tools config file will ignore that file and use the one in the repo root, but then trying to use that tool will use the local config file and then fail.
That issue is marked as resolved, but it still seemed to be occuring in the SDK version used by the tests
| |> Async.RunSynchronously | ||
| |> List.ofSeq | ||
| |> List.find (fun product -> product.ProductVersion.Equals("6.0")) | ||
| |> List.find (fun product -> product.ProductVersion.Equals("8.0")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the logic for this (or cachedDotnetSdkReleases.json) should be when updating .NET versions.
The current cachedDotnetSdkReleases.json only contains the .NET 6 releases and that seemed to cause test failures when built as .NET 8 and looking for the v8 SDK versions.
Changing this to 8.0 fixed that, but that might then not be right if fake-cli is multi-targetted at .NET 6 and .NET 8 as the .NET 6 version might still need the list of v6 releases?
Test Results 12 files ±0 12 suites ±0 31m 3s ⏱️ - 2m 12s Results for commit afe43a2. ± Comparison against base commit d95a3ad. This pull request removes 3 and adds 3 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
The remaining failures are in the TemplateIntegrationTests, with errors like and I'm not sure if this is really a problem with the new stuff, or if it's because it's trying to use the 6.1.3 release to test the templates, and failing when running as .NET 8? (I suppose it shou;dn't be looking at the 6.0 assemblies when running as 8.0) |
|
I expect that there is a chicken-and-egg problem, and the integration tests are actually restoring the .NET6 fake to try compiling .NET8, instead of referring to the just built .NET 8 version. I tried this branch locally (Numpsy/new8-build-bump-roll-tests-2) on my machine, and my observations are here:
I run on my Windows machine dotnet build, and then from the actions: It turned out that this didn't work properly. So I removed the Clean-task and called fake manually:
And behold, all the integration tests went through, after painfully long half-an-hour later:
Where it did finally fail is: So basically, dotnet deb tool has to be updated from 0.1.220 to version 0.1.232 FAKE/.config/dotnet-tools.json Line 6 in 73fc961
...and that's it, I think we could consider merging this and releasing a beta or new major version. |
@Thorium yes it can. |
Whether there's any benefit to supporting .NET 6 in fake-cli any more is another question, and given the amount of pain in testing it all, just doing 8 seems simpler for a new major release? .NET 10 is on the horizon, so making sure it works for that going forward seems more important than keeping 6 working in new versions. (My personal opinion anyway). I'll see about rebasing it and fixing the conflicts anyway |
|
Many projects use a separate non-FAKE fsproj as a build-project and reference the FAKE libraries, and there I see value in having .NET6 support kept. But the fake-cli tool, I think, could target only the NET8 (or even 10 later). |
|
Then it will be impossible to run it unless you have the latest SDK installed |
The 'DNX' stuff that MS are talking about with .NET 10 (https://github.com/dotnet/core/blob/main/release-notes/10.0/preview/preview6/sdk.md#platform-specific-net-tools) could maybe help if the tool could be self contained, but that's a work in progress so it remains to be seen |
|
On the subject of targetting multiple .NET versions in fake-cli, re-reading the old comments reminded me of the situation with cachedDotnetSdkReleases.json - i.e., if the tool i smulti-targetted, you might need multiple versions of that file for different .NET versions? |
0364d81 to
233081b
Compare
|
@Thorium I didn't see your other PR until after i'd rebased the changes to fix the merge conflicts, which caused conflicts in that, so I've cherry picked your dotnet-deb update straight into this PR |
|
I think this file https://github.com/fsprojects/FAKE/blob/master/src/template/fake-template/Content/.config/dotnet-tools.json But I'm not sure if there is any workaround possible. |
62084b1 to
7b0784d
Compare
7b0784d to
41a04b5
Compare
|
General question about all the FAKE modules - should they all be both .NET 6 and .NET 8, or just NET 8? (or maybe only 6 if they don't have any dependencies where it matters?) |
|
Otherwise the basic question is whether this should be turned into 8.0.0 alpha 1 and then the templates and tests fixed on top of that, or if it's worth trying something more minimal with just a .NET 8 / v8 build of fake-cli and then doing the modules afterwards. |
41a04b5 to
ba8e1ed
Compare
It appears to work (all integration tests pass) if the .NET 6 runtime files are present in the local .NET install created by the tests, but not otherwise. So If i run the tests from this branch on a clean setup, if fails as the CI does. If I run the build from the master branch, it installs the .NET 6 libs into If I then run the tests from this branch again, it installs the .NET 8 libs into Having .NET 6 installed in the default system location doesn't effect this as the tests are looking in the custom install location. |
|
As net6.0 is past end of support, as is net7.0, after any necessary bridging release to get to net8.0, it would make sense to drop all mention of net6.0 - and work to prepare for net10.0 so we don't have an end of support crisis this time next year as both net8.0 and net9.0 are superseded on 10-Nov-26. Let us hope that that process that time can be made as easy as "for a bridging release add mention of net10.0 at these points in the build system, the project targets and the code, then enable this bridging flag; for real net10.0 release, remove net8.0 and clear the flag. Lather, rinse repeat for net12.0, net14.0 ..." |
fe22252 to
21017b8
Compare
| <packageSources> | ||
| <clear /> | ||
| <add key="api.nuget.org" value="https://api.nuget.org/v3/index.json" /> | ||
| <add key="localBuild" value="{nugetPackageFolder}" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows the tests to find the locally built v8-alpha fake-cli package instead of always using the 6.1.4 version off nuget.org, which lets the test pass by using the .NET 8 version instead of the .NET 6 version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although RuntimeAssemblyVersions now defaults to a 2-element list, this is actually pointless as everything derived from the value starts by goes through |> Seq.head - lines 65-68 of this file. Retaining the sequence nature may help with the net10.0 (and future) update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 283, 295 still refer to a method net60releases; this could do with renaming when it refers to net8.0 SDKs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it should be both 6/8 now, or just 8 (will something running on .NET 8 ever want the .NET 6 libs?)
@Thorium ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe line 51 should be ReleaseVersion "8.0.0" as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 283, 295 still refer to a method
net60releases; this could do with renaming when it refers to net8.0 SDKs.
renamed to netReleases
21017b8 to
439ba10
Compare
|
Should we skip net8 and go straight to net10.0 ? |
|
Personally I'd say It's a bit early to not support .NET 8.0, but as said earlier it could do with having some .NET 10 testing now rather than later. On a related note, the preview version of the paket cli that adds .NET 10 support requires .NET 8 so this change is a blocker for updating that as well. |
|
Also on the subject of Paket, I tried updating to Paket.Core v10 to get ,NET 10 support and the build failed with https://github.com/Numpsy/FAKE/actions/runs/19647052005/job/56264684996#step:6:729 which I guess is down to the compressed metadata changes in FSharp.Core 9 not5 working with the .NET 6.0 SDK any more? |
|
So, is it ready for merge? |
|
It all builds and the tests all pass at least. Someone maybe needs to confirm if all of the libraries need to target both .NET 6.0 and .NET 8.0 or could just be 8.0 for simplicity (if the tests are only run as 8 then the .NET 6 versions might not get any testing), but that could be tuned after the base updates are done) |
Change global.json .NET 6 to 8 Search and replace targetframeworks from fsproj files to add net8.0 Add net8.0 to paket.dependencies dotnet paket install to find .NET8 compatible dependencies Expecto had to be hardcoded for now, because some tests are running on netstandard2.0 library (hopefully we can update this separately later) MSBuild.StructuredLogger problem: DisableInternalBinLog = true had to be added to build.fsx NuGet commands (hopefully we can update this separately later) A few places of code had new overrides so had to explicitly type to strings SdkAssemblyResolver to default .NET 8 as well Readme update GitHub pipeline configs: Add .NET 8 install.
…l has to be upgraded from 16 to 20, to make GenerateDocs task pass.
…the 8.0.0 alpha build Instead of pulling the 6.1.X build off NuGet.org. This might bootstrap the tests enough to get them all passing
…n 8.0.0 instead of 6.0.0
Because it might be .NET 8 releases now
… up the fix for GHSA-w3q9-fxm7-j8fq Also updates transitive dependencies on System.Text.Json to
ae027fd to
afe43a2
Compare
|
#2880 does not pass. But I would merge if it does. |
The build failure there looks like it needs Fantomas running on it |



Description
As #2818, but updated on top of the latest changes and with a couple of extra commits to try to get more tests passing - see further comments for details