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

Updated FCS to .NET/F# 9.0 + Prepare Fable 5 alpha #3957

Merged
merged 6 commits into from
Nov 24, 2024

Conversation

ncave
Copy link
Collaborator

@ncave ncave commented Nov 16, 2024

  • Updated FCS to .NET/F# 9.0 (commit 3dff60a)
  • Changed Analyzers build image to ubuntu-latest
  • Updated Fable TFM to .NET 8.0
  • Updated build dependencies
  • Removed rarely used build dependencies

@ncave
Copy link
Collaborator Author

ncave commented Nov 16, 2024

@MangelMaxime This is just a first pass. We may want to update the Fable CLI to .NET 8.0, now that .NET 6.0 is no longer supported. There are quite a few security warning for old dependencies that need to be updated.

Before we merge this, do you mind publishing a new Fable release with all the pending changes that have already been merged?

@ncave
Copy link
Collaborator Author

ncave commented Nov 16, 2024

Here is perhaps why the Analyzers build fails: dotnet/sdk#44868
We may have to disable it until it's resolved.

Update: Switching the analyzers build to Linux makes it work.

@ncave ncave marked this pull request as draft November 16, 2024 08:30
@ncave ncave marked this pull request as ready for review November 16, 2024 09:06
@MangelMaxime
Copy link
Member

Hello @ncave,

I made a new Fable release with all the changes available on main.

However, to do the release I had to revert Run husky with --allow-roll-forward because I had an error telling me that --allow-roll-forward is not a recognised argument.

I didn't investigate any further as I wanted to make the release and we can re-introduce --allow-roll-forward if needed.

@ncave
Copy link
Collaborator Author

ncave commented Nov 19, 2024

@MangelMaxime

I had to revert #3956

That's fine, we can add it later. Just need to figure out how to conditionally add it only for .NET 9.0.

@ncave
Copy link
Collaborator Author

ncave commented Nov 19, 2024

@MangelMaxime
Rebased, ready for review.

Let me know if you think we need to bump the Fable CLI major version for this change in TFM.

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@MangelMaxime
Copy link
Member

@ncave Thank you, I will have a look at it tomorrow.

@MangelMaxime
Copy link
Member

I think this is ready to be merge but we need to make a decision regarding the version number indeed.

I believe we should probably bump the major version because people requesting .NET 6 using global.json will not be able to use Fable 5. At least, that's what I would expect dotnet/NuGet to do.

If we do the bump, here are the "side effects" we should resolve (perhaps there are some others too):

  1. Can this cause issues with Fable compiler plugins?

    In theory, this should be fine because Compiler plugins should have defined something like override _.FableMinimumVersion = "4.0"

    Example with Feliz

    There are probably only 2 plugin really impact by that Feliz which should be ok and Oxpecker.Solid which should also be fine.

  2. What about Fable.AST ?

    I think I remember a discussion where we mentioned that Fable AST version was mirroring Fable version. So Fable AST 4.x was for Fable 4.

    Should we also bump Fable.AST to version 5 or says that Fable.AST 4 is compatible with Fable 4 & 5?

    I think the easier path is to consider Fable.AST 4 compatible with Fable 4 & 5. We can even make a matrix in Fable documentation to make an official statement.

  3. If we bump to .NET 8, it means we are now in a situation where we can make MSBuildCrackerResolver the default instead of BuildalyzerCrackerResolver.

    If we do the major bump, I think this a good opportunity to do the switch and add a --legacy-resolver options to Fable CLI as a potential workaround if something was not catch during the experimentation phase of MSBuildCrackerResolver.

  4. We need to archive current Fable REPL under /repl4 and make the /repl target Fable 5.

  5. We need to create FABLE_COMPILER_5 compiler directive. There is a risk that some libraries will need to be updated if they used #if FABLE_COMPILER_4 but I expect this impact to be minimal.

  6. I checked SAFE Stack template and they are already using .NET 8 via global.json. So at least, here we should avoid too much migration path issues if we bump to a .NET 8 tool.

With all that said I see 2 solutions:

  1. We keep release F# 9 support in Fable 4 targeting .NET 6, which will give us time to think about the issues mentioned above. We can
  2. We go with Fable 5 and decide now what happens in regards of the different impacted areas.

@ncave
Copy link
Collaborator Author

ncave commented Nov 22, 2024

@MangelMaxime All good points, I fully agree (especially # 3, I was going to say the same exact thing).
IMO, let's just go with it, the nullability change in F# 9 is reason enough to bump major version in Fable too.

@nojaf
Copy link
Member

nojaf commented Nov 22, 2024

My two cents: go with a major and publish alpha's until you get it right.

@MangelMaxime
Copy link
Member

You are right @nojaf better be safe than sorry.

@ncave Unless, you started working on the different items I listed, I can work on them and if all goes well make an alpha version to check it on a "real project".

@ncave
Copy link
Collaborator Author

ncave commented Nov 22, 2024

@MangelMaxime Please go ahead. IMO # 4 (the repl update) can wait until #3960 is merged.

@MangelMaxime MangelMaxime changed the title Updated FCS to .NET/F# 9.0 Updated FCS to .NET/F# 9.0 + Prepare Fable 5 alpha Nov 24, 2024
@MangelMaxime MangelMaxime merged commit 418fb1f into fable-compiler:main Nov 24, 2024
19 checks passed
@MangelMaxime
Copy link
Member

@ncave Fable 5.0.0-alpha.1 has been released.

I made the main branch for Fable 5 release and created a fable4 branch if we need to make a release for this version.

I did it in this order because, I don't see Fable 5 to stay in alpha for a long time and like that we can focus on a single version instead of maintaining 2 versions at the same time. If needed, we can always revisit this position.

@ncave ncave deleted the fsharp9 branch November 25, 2024 10:23
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.

3 participants