-
Notifications
You must be signed in to change notification settings - Fork 587
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
Build with .NET 8.0 #2818
base: master
Are you sure you want to change the base?
Build with .NET 8.0 #2818
Conversation
The real issue with the build.fsx is that it's recursive: In the beginning it says "use paket But how can you build .NET 8 local package, when there is no .NET 8 compatible packages in NuGet? |
How was it done when .NET 6.0 support was first added? Do the Microsoft.Build.* package references in build.fsx need unpinning from 17.3.2 to support .NET 8 buiids? |
If I build this locally with I have a bad hunch that version update could have been done by releasing some -alpha package without Fake, and then moved to build.fsx when it's uploaded to NuGet. But hopefully there is a way to avoid that, as it sounds slow and error-prone. |
Not sure if this is an issue or not, but in case it is - When testing a local build I noticed that I tried adding .NET 8.0 in the fsx file at 2852870 as a test and it failed to load some v8.0.0 libraries - https://github.com/Numpsy/FAKE/actions/runs/10702782852/job/29671766410#step:10:950 - so not sure if that counts as an improvement. Anyway - is it possible that the default frameworks at FAKE/src/app/Fake.Runtime/FakeHeader.fs Line 182 in fac7295
|
if you run locally I think this is some kind of issue of "dotnet tool restore" picking the framework from latest version instead of instructed 1.0.0-local version. There is --framework parameter on dotnet tool install but that doesn't work on local tools like this. |
lf I open .fake\ dll with IlSpy, it seems to be compiled to .NET core 3.1, which could explain why it can't handle the libraries referenced: The reason is CompileRunner.fs calling FSharpChecker.Create() This seems to be the case in both current master and this branch. |
Oh, but netstandard 2.0 is not compatible with .net8. Shouldn't it be 2.1? |
I'm not entirely sure if those restrictions are a 'can be consumed by' or a 'same family of TFMs as' ? The last build does seem to have picked the .NET Standard 2.1 version of FSharp.Core though - https://github.com/fsprojects/FAKE/actions/runs/10729495009/job/29756204428#step:10:404 |
I made an attempt at debugging the cli tool locally to see if I could see what's going on with the failure to load System.Globalization.dll seen in the CI builds, and made an observation: When debugging it, the call to LoadFromAssemblyName at FAKE/src/app/Fake.Runtime/CoreCache.fs Line 331 in 3a3e00c
However, the Looking at the difference, it appears that the .NET 6.0 version of System.Globalization has the Searching about a bit for that attribute value I spotted dotnet/runtime#89490, where said property was removed from all in-box assembles in .NET 8.0. So - I wonder if the logic there is just wrong in .NET 8 and newer? |
I guess the latest failure is becuse Line 807 in 3a3e00c
|
We might need to go with a route where there is "fake-cli 8.0.0-alpha -package" in NuGet to not break any existing fake implementations but still be able to call the global tool. |
it needs rebase again |
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.
# Conflicts: # RELEASE_NOTES.md # src/app/Fake.Runtime/SdkAssemblyResolver.fs
When trying to reason the build-log: There are 3 parallel threads all writing to same output, and many of the commands are just starting a process and then monitoring exit-code. |
build.fsx
Outdated
@@ -658,15 +655,15 @@ Target.create "DotNetCoreIntegrationTests" (fun _ -> | |||
|
|||
runExpecto | |||
root | |||
"src/test/Fake.Core.IntegrationTests/bin/Release/net6.0/Fake.Core.IntegrationTests.dll" | |||
"src" </> "test" </> "Fake.Core.IntegrationTests" </> "bin" </> "Release" </> "net6.0" </> "Fake.Core.IntegrationTests.dll" |
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.
Do these need ()
round them?
Would it be worthwhlie trying to get something like https://github.com/EnricoMi/publish-unit-test-result-action plugged into the build and publish the test result xml files? Might be easier to see the results vs. trawling through all the text |
I like the idea, but maybe separately in PR to master-branch and I'll merge it here then? |
…ll the versions: they'll be filtered later with exist.
…ll the versions: they'll be filtered later with exist.
It turns out the latest FAKE 6.1.3 builds .NET 8 projects just fine (after this PR #2835), so I think the priority of this struggle just dropped. |
I'll have a look at what it would involve |
Ok, some test results are getting published now - e.g. https://github.com/fsprojects/FAKE/actions/runs/11105226220/job/30852070808 |
Test Results 4 files - 5 4 suites - 5 52m 6s ⏱️ + 4m 59s For more details on these failures, see this check. Results for commit 6b18ea8. ± Comparison against base commit 6f2fc43. This pull request removes 2 tests.
|
Related to this: I had a try at updating to Paket 9 rather than the alpha version it's currently using, and
I'm currently wondering if (2) might be because the dll was built with .NET 9 and has compressed metadata enabled so the .NET 6 compiler can't consume it, but I haven't looked into it any more than that so far |
Changes with this PR:
Possible issue:
If tests restore still previous fake-cli 6.1.1 and not this version, then they might fail.