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

Compiling as library #3561

Merged
merged 31 commits into from
Dec 12, 2023
Merged

Compiling as library #3561

merged 31 commits into from
Dec 12, 2023

Conversation

nojaf
Copy link
Member

@nojaf nojaf commented Oct 21, 2023

This is a very rough proof of concept for #3552.
It worked on my machine, I think it is kinda cool.
I duplicated a lot of code from Fable.Cli, you should imagine Fable.Cli takes a dependency on Fable.Compiler.Service and the right shared core components live there.

This is just an attempt to illustrate what I think could be valuable.
Checkout tests/FCSTest/Program.fs to see what a third-party tool could do with this.

image

I believe the MSBuild experiment in #3549 could benefit from this as well.

@ncave
Copy link
Collaborator

ncave commented Oct 28, 2023

@nojaf I agree with the sentiment expressed in other threads that it makes more sense to rename Fable.Compiler.Service project to just Fable.Compiler, otherwise it's visually very confusing.

@nojaf
Copy link
Member Author

nojaf commented Nov 27, 2023

Hi all,

A bit of an update, I'm still working on this from time to time.

In its current form, I can load an fsproj with a NuGet dependency from my Vite plugin.
Originally, I tried to crack the projects myself using an out-of-process design time build with the new dotnet 8 functionality. While I still think this is the coolest thing ever, it became apparent that I ProjectCracker does a lot more custom logic I also would have to deal with and so I decided to move ProjectCracker as is to Fable.Compiler. I'm using the same cracking as the cli tool does and that does work out to resolve the NuGet dependencies. This is good enough for now.

One day, I would like to revisit the way we compose FSharpProjectOptions for Fable, see #3613

The next step would be to polish this PR and try integrating an existing project.
I'll probably try out the online tool of Telplin, as that is a small project in scope.

@nojaf
Copy link
Member Author

nojaf commented Dec 1, 2023

Hi @ncave , I would like to polish this PR and wrap it up so we can ship the first alpha version of Fable.Compiler.

Two open questions:

  • Could you sync your F# fork one more time to include ncave/fsharp@3a553a7. The PR was merged but is not synced with the Fable repo yet.
  • How do we wish to package the dependencies of Fable.Compiler? Do we just want to include the FSharp.Core and FCS lib as part of the same NuGet package? This is maybe a little less clean but would be more convenient I think.

@nojaf
Copy link
Member Author

nojaf commented Dec 1, 2023

Oh also, Fable.Standalone.fsproj appear to be using fable-fcs.fsproj instead of the binary.
Do I need to add InteractiveChecker.GetDependentFiles there as well? Or how is that synced with your fork?

@ncave
Copy link
Collaborator

ncave commented Dec 1, 2023

@nojaf

Could you sync your F# fork one more time?

Sure, if you don't have any more changes for it.

Do we just want to include the FSharp.Core and FCS lib as part of the same NuGet package?

Probably yes, same dependencies as the Fable.Cli tool.

Fable.Standalone.fsproj appear to be using fable-fcs.fsproj instead of the (FCS) binary.

Yes, Fable.Standalone is using a different branch of the FCS fork, which is more heavily modified so it can be compiled with Fable itself, mostly for the REPL and pure JS version of fable-compiler-js running on Node.js.

@ncave
Copy link
Collaborator

ncave commented Dec 1, 2023

@nojaf
To solve the Fable.Standalone issue, is there a need to push the FCS type checker up the stack all the way to Fable.Transforms, just to get the dependencies? Can it stay at Fable.Cli or Fable.Compiler level, and just pass in the graph lookup results?

If not, there are other reverse dependencies on lower stacks implementations in the interface here, perhaps the graph dependency lookup can be added there in the interface and implemented in Fable.Compiler.

@nojaf
Copy link
Member Author

nojaf commented Dec 1, 2023

Sure, if you don't have any more changes for it.

Yes, that should be all for now.

Thanks for the feedback, I'll look into this over the weekend probably.

@jwosty
Copy link
Contributor

jwosty commented Dec 1, 2023

In the case of FSharp.Compiler, since it's a library package, shouldn't FSharp.Core be a proper dependency? Especially because the application depending on us might not choose exactly the same FSharp.Core version. I think bundling it only makes sense for the tool. FCS of course is an exception since it's a custom fork.

@ncave
Copy link
Collaborator

ncave commented Dec 1, 2023

@jwosty

shouldn't FSharp.Core be a proper dependency

Yes, we can, but we'll have to build FCS with the same version.

@nojaf nojaf force-pushed the carve-out branch 2 times, most recently from 855e4f0 to 97938ce Compare December 6, 2023 15:37
@nojaf nojaf marked this pull request as ready for review December 6, 2023 15:51
@nojaf
Copy link
Member Author

nojaf commented Dec 6, 2023

This is ready for review.

It still lacks:

  • Newer FCS binaries. @ncave would you have time for that sync? I don't want to include that nojaf folder.
  • Needs a changelog file. Would like to introduce KAC first.
  • Needs some more though on creating the nupkg.

@ncave
Copy link
Collaborator

ncave commented Dec 8, 2023

@nojaf Rebased FCS in #3646, let me know if there is anything else you need.

@nojaf
Copy link
Member Author

nojaf commented Dec 11, 2023

Thanks @ncave! No need to do an extra sync for this, but could you include the .pdb files in the future?

@nojaf
Copy link
Member Author

nojaf commented Dec 11, 2023

@MangelMaxime even though this still has some todos, would it be ok to have this PR merged?
I'd like to follow up with future PRs instead of keeping this PR in sync any longer.

@ncave
Copy link
Collaborator

ncave commented Dec 11, 2023

@nojaf

could you include the .pdb files in the future?

The PDBs are already embedded in the assemblies, no?

@nojaf
Copy link
Member Author

nojaf commented Dec 11, 2023

The PDBs are already embedded in the assemblies, no?

Ah, yes indeed, I believe you are right. Didn't think of that.

@ncave
Copy link
Collaborator

ncave commented Dec 11, 2023

@nojaf

even though this still has some todos ...

I assume one of the todos is to eventually support the full set of languages that Fable supports.

@MangelMaxime
Copy link
Member

@MangelMaxime even though this still has some todos, would it be ok to have this PR merged? I'd like to follow up with future PRs instead of keeping this PR in sync any longer.

Looking at the discussion history it seems like this PR is not yet ready for a release.

You said above:

Needs a changelog file. Would like to introduce KAC first.

We can have the KAC integration done later, current system is working fine so Fable.Compiler could just use the same publishing system as the others.

Needs some more though on creating the nupkg.

What more though are needed?

These questions needs to be answered because we need to be able release Fable.CLI, otherwise we will not be able to releases fixes for the different bug or improvements made to Fable.

@ncave
Copy link
Collaborator

ncave commented Dec 11, 2023

@nojaf A good question for the Fable.Compiler as a library is what is the best way for the library user to drive some of the different Fable behaviors/options that are currently passed in as command line arguments.

@nojaf
Copy link
Member Author

nojaf commented Dec 11, 2023

Hi everyone,

I'm keen on getting this PR merged to avoid further rebasing, which is becoming quite tedious. Just to be clear, I'm not suggesting an immediate release on NuGet. Using it locally works for me. What I'm really aiming for is to contribute more regularly but in smaller chunks.

I assume one of the todos is to eventually support the full set of languages that Fable supports.

Eventually yes, but right now, there is no use case for anything different than JS.

What more though are needed?

I want to run this by Chet in terms of the NuGet creation. Whether I have all the right props in place and I am adhering to the best practices. And also hook this up in the Fable.Build project to publish the NuGet. All things that can be done later I think.

@MangelMaxime
Copy link
Member

Just to be clear, I'm not suggesting an immediate release on NuGet.

I understand that you are not suggesting an immediate release.

Any fixes we want to do for Fable, require a new release of Fable.CLI. Which with this PR requires Fable.Compiler to be released meaning that it needs to have a Changelog and be hooked to the actual build process.

That's why I suggested to not use KAC for now and just use the way things are for now. KAC integration can be done later if/when needed.

Personally, I am ok with releasing a version of Fable.CLI that use the current Fable.Compiler. Even if Fable.Compiler, is not stable yet. As long as the surfacing Fable.CLI args and requirements in term of SDKs/Tools installed on the user machine doesn't change compared to what we have now, I am fine with it.

@nojaf
Copy link
Member Author

nojaf commented Dec 12, 2023

Personally, I am ok with releasing a version of Fable.CLI that use the current Fable.Compiler. Even if Fable.Compiler, is not stable yet. As long as the surfacing Fable.CLI args and requirements in term of SDKs/Tools installed on the user machine doesn't change compared to what we have now, I am fine with it.

Yup, that would be the short-term plan for sure. If anything were to change somewhere in Fable.Compiler that affects Fable.Cli, it should be listed in the Fable.Cli changelog.
For example, a fix happens in Glob.fs.

<Reference Include="../../lib/fcs/FSharp.Core.dll" />
<Reference Include="../../lib/fcs/FSharp.Compiler.Service.dll" />
<Reference Include="../../lib/fcs/FSharp.Compiler.Service.dll" />
<Reference Include="../../lib/fcs/FSharp.DependencyManager.Nuget.dll" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it normal that we have a new "dependency" here?

I asking in case this is something that has been forgotten.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't be used in Transforms, so I will remove it.
If we were using vanilla FCS that ref will be there:
image

In this case, we are not using it, so I will remove it.

@MangelMaxime MangelMaxime merged commit bd08800 into fable-compiler:main Dec 12, 2023
15 checks passed
@MangelMaxime
Copy link
Member

@nojaf I had to revert the merge of this PR because I could not pack Fable.Compiler.

Unhandled exception. System.AggregateException: One or more errors occurred. (The command exited with code 1.

Standard output (stdout):

MSBuild version 17.8.3+195e7f5a3 for .NET
  Determining projects to restore...
  Restored /workspaces/Fable/src/Fable.Compiler/Fable.Compiler.fsproj (in 213 ms).
  3 of 4 projects are up-to-date for restore.
  Rust.AST -> /workspaces/Fable/src/Fable.Transforms/Rust/AST/bin/Release/netstandard2.0/Rust.AST.dll
  Fable.AST -> /workspaces/Fable/src/Fable.AST/bin/Release/netstandard2.0/Fable.AST.dll
  Fable.Transforms -> /workspaces/Fable/src/Fable.Transforms/bin/Release/netstandard2.0/Fable.Transforms.dll
  Fable.Compiler -> /workspaces/Fable/src/Fable.Compiler/bin/Release/net6.0/Fable.Compiler.dll
/home/vscode/.dotnet/sdk/8.0.100/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets(221,5): error NU5019: File not found: '/workspaces/Fable/src/Fable.Compiler/bin/Release/net6.0/Fable.Transforms.pdb'. [/workspaces/Fable/src/Fable.Compiler/Fable.Compiler.fsproj]


Standard error (stderr):

@nojaf nojaf mentioned this pull request Dec 13, 2023
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