Fix build failures when building with the .NET 10 SDK #779
Conversation
| iIndex + child < nodeArray.Length && | ||
| jIndex + child < nodeArray.Length && | ||
| nodeArray.[iIndex + child].Hashcode = nodeArray.[jIndex + child].Hashcode) { 0..numChildren } | ||
| nodeArray.[iIndex + child].Hashcode = nodeArray.[jIndex + child].Hashcode) (seq { 0..numChildren }) |
There was a problem hiding this comment.
This is to fix a deprecation warning that happens when building the current code with .NET 10
/home/runner/work/FSharpLint/FSharpLint/src/FSharpLint.Core/Rules/Hints/HintsHelper.fs(34,88): error FS3873: This construct is deprecated. Sequence expressions should be of the form 'seq { ... }' [/home/runner/work/FSharpLint/FSharpLint/src/FSharpLint.Core/FSharpLint.Core.fsproj::TargetFramework=net9.0]
| for pair in jsonObj.AsObject() do | ||
| if pair.Value.GetValueKind() = Text.Json.JsonValueKind.Object then | ||
| match pair.Value.AsObject().TryGetPropertyValue("enabled") with | ||
| let result, isRule = pair.Value.AsObject().TryGetPropertyValue("enabled") |
There was a problem hiding this comment.
System.Text.Json v10 has two different overloads of TryGetPropertyValue, and the existing code here can't work out which one to call
|
@Numpsy thanks, do you know if this generates docs properly? I could merge this tomorrow and make a release. |
(But for that, the PR needs to be non-draft.) |
|
The docs built ok. I'll have a look at my local build to see what they look like. Actually, as it stands, just removing the install-dotnet step from the documentation CI and letting it use the already installed .NET 9 seems to work - Though that might change when 10 is installed on the build agents by default. |
Then how about a PR that removes the install-dotnet step from all jobs?
Let's cross that bridge when we arrive to it? |
Or we should just update our global.json to the same version .NET9 that is installed by default? This way maybe all jobs work without changes. |
I tested this, and it worked! So I think I'll merge this change, so long as you don't have any objections (I know it might make CI easily fail in the future, but I think we can revisit it when that happens). And in case you agree and I merge that, you could still maintain this PR for the fs/fsx changes (rebasing it), which still seem to be desirable to be prepared for .NET10 moving forward. |
|
That's fine with me. |
|
Small tweak to my comment above: removing all setup-dotnet steps works, so long as I also upgrade global.json to the last 9.x version (9.0.306). |
|
Was there a reason for global.json to disable roll forward rather than using the latest 9.0.x ? |
|
Don't ask me that haha! You're the author of that line according to git-blame hehe: b173d9f4a76fcb6f6e95f3d4ef9466abb99492e4 And seems you forgot to mention why in the commit msg? |
|
Oops, I just found the PR that this b173 commit belongs to, and it was a squash of many changes, some which might have not been pushed by you (that's why I'm not a fan of squashing PRs that have more than 1 commit). |
|
Reading the comments in #736 reminded me - It might have been hitting a bug in the 9.0.300 SDK and was pinned to the old version beacuse of that. So, maybe that's not an issue any more with the latest v9 SDK? |
Ok this is ready: #780
Let's see, CI running: https://github.com/knocte/FSharpLint/commits/wip/fixDocsCiBuildErrorByReenablingRollforward/ |
Numpsy
changed the title
|
Ok, I've updated this one to just contain the two code changes I needed to get it to build with the .NET 10 SDK |
|
Ok thanks, will merge after a new release is pushed. BTW, this morning scheduled CI was broken again T_T but FYI thankfully the rollforward change fixed it this time. |
System.Text.Json v10 has two different overloads of TryGetPropertyValue and the existing code can't work out which one to use, so tweak it so that it builds. The changed code still works when build with .NET 9
The .NET 10 compiler wamts sequence literals to exlpicitly use seq{}
|
Merged manually (but attributing proper authorship of commits), thanks @Numpsy! |
2 participants
Just an attempt at seeing if it works, still not sure what's causing the current CI failures.
This did require a couple of other changes to get it to build though - see other comments