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

Build with .NET 8.0 #2818

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Build with .NET 8.0 #2818

wants to merge 15 commits into from

Conversation

Thorium
Copy link
Member

@Thorium Thorium commented Sep 3, 2024

Changes with this PR:

  • 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.

Possible issue:
If tests restore still previous fake-cli 6.1.1 and not this version, then they might fail.

paket.lock Outdated Show resolved Hide resolved
@Thorium
Copy link
Member Author

Thorium commented Sep 3, 2024

The real issue with the build.fsx is that it's recursive: In the beginning it says "use paket source release/dotnetcore" which is a local folder, that is having the latest build result. If there is nothing yet, then it uses latest release from the NuGet.

But how can you build .NET 8 local package, when there is no .NET 8 compatible packages in NuGet?

@Numpsy
Copy link
Contributor

Numpsy commented Sep 3, 2024

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?

@Thorium
Copy link
Member Author

Thorium commented Sep 4, 2024

If I build this locally with dotnet build or dotnet pack it works on both my Mac and Windows.
But if I try to run it with dotnet fake run it fails on "Fake is not defined" which doesn't make sense to me. Maybe I'm missing something trivial. At least it does read the dll so that is already further than the current Fake 6.1.1 which just fails for #2314 (comment)

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.

@Numpsy
Copy link
Contributor

Numpsy commented Sep 4, 2024

But if I try to run it with dotnet fake run it fails on "Fake is not defined" which doesn't make sense to me

Not sure if this is an issue or not, but in case it is -

When testing a local build I noticed that build.fsx.lock has a RESTRICTION: || (== net6.0) (== netstandard2.0) at the top, and wondered if that could be an issue if dealing with the latest versions of libraries like Microsoft.Build which don't have either of those TFMs (just .NET 8.0 and FW 4.7.2)?

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

"framework: netstandard2.0,net6.0" + "\n" + p
should be different in a .NET 8.0 build?

@Thorium
Copy link
Member Author

Thorium commented Sep 5, 2024

if you run locally dotnet fake --version then you can observe that it has picked FakePath: .../net6.0/... for whatever reason even there should be both net6.0 and net8.0 in the tool package, and the global.json tells to use net8.0

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.

@Thorium
Copy link
Member Author

Thorium commented Sep 6, 2024

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:
image

The reason is CompileRunner.fs calling FSharpChecker.Create()
and I expect this is related: dotnet/fsharp#14243

This seems to be the case in both current master and this branch.

@Thorium
Copy link
Member Author

Thorium commented Sep 6, 2024

When testing a local build I noticed that build.fsx.lock has a RESTRICTION: || (== net6.0) (== netstandard2.0) at the top,

Oh, but netstandard 2.0 is not compatible with .net8. Shouldn't it be 2.1?

@Numpsy
Copy link
Contributor

Numpsy commented Sep 6, 2024

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

build.fsx Show resolved Hide resolved
@Numpsy
Copy link
Contributor

Numpsy commented Sep 17, 2024

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

let assembly = AssemblyLoadContext.Default.LoadFromAssemblyName(name)
manages to load the assembly -
image

However, the isFramework check a few lines down then discards the loaded dll. This is different from the current .NET 6.0 build, which keeps it.

Looking at the difference, it appears that the .NET 6.0 version of System.Globalization has the .NETFrameworkAssembly attribute, and the .NET 8 version doesn't.

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?

@Numpsy
Copy link
Contributor

Numpsy commented Sep 19, 2024

I guess the latest failure is becuse

FAKE/build.fsx

Line 807 in 3a3e00c

DotNet.publish
needs to specify whether to publish as .NET 6.0 or 8.0? (assuing it still needs to build as 6.0 rather than just using 8.0)

@Thorium
Copy link
Member Author

Thorium commented Sep 22, 2024

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?

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.

@xperiandri
Copy link
Collaborator

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.
@Thorium
Copy link
Member Author

Thorium commented Sep 24, 2024

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"
Copy link
Contributor

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?

@Numpsy
Copy link
Contributor

Numpsy commented Sep 25, 2024

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.

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

@Thorium
Copy link
Member Author

Thorium commented Sep 26, 2024

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.
@Thorium
Copy link
Member Author

Thorium commented Sep 26, 2024

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.

@Numpsy
Copy link
Contributor

Numpsy commented Sep 29, 2024

I like the idea, but maybe separately in PR to master-branch and I'll merge it here then?

I'll have a look at what it would involve

@Numpsy
Copy link
Contributor

Numpsy commented Sep 30, 2024

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.

Ok, some test results are getting published now - e.g. https://github.com/fsprojects/FAKE/actions/runs/11105226220/job/30852070808

Copy link
Contributor

Test Results

  4 files   -   5    4 suites   - 5   52m 6s ⏱️ + 4m 59s
437 tests  -   2  435 ✅  -   4  0 💤 ±0  2 ❌ +2 
479 runs   - 772  476 ✅  - 775  0 💤 ±0  3 ❌ +3 

For more details on these failures, see this check.

Results for commit 6b18ea8. ± Comparison against base commit 6f2fc43.

This pull request removes 2 tests.
[Fake.DotNet.FxCop.Tests; Test failure on non-Windows platforms]
[Fake.DotNet.ILMerge.Tests; Test failure on non-Windows platforms]

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