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

[WIP] Compile fsx #1728

Closed
wants to merge 3 commits into from
Closed

[WIP] Compile fsx #1728

wants to merge 3 commits into from

Conversation

nojaf
Copy link
Member

@nojaf nojaf commented Feb 11, 2019

Using Dotnet.ProjInfo.Workspace.FCS we can compile fsx files as well.

Possible flow

Users use paket for dependencies.
paket load can create a script to reference everything needed.

#load @".paket\load\netstandard2.0\main.group.fsx"

Current limitations

  • the *.fsx script requires a reference to netstandard2.0.

script.fsx:

#r @"C:\Users\nojaf\.nuget\packages\netstandard.library\2.0.3\build\netstandard2.0\ref\netstandard.dll"
  • webpack-dev-server keeps compiling in watch mode, might be a loader issue. Not sure atm.

Todo

  • Fix limitations.
  • Add something to Fable repo for CI, I'm currently using a test project on my machine.
  • Update documentation.

@MangelMaxime
Copy link
Member

About the limitation to references netstandard2.0 would something like used in FAKE solves the issue:

#if !FAKE
#r "Facades/netstandard"
#r "netstandard"
#endif

@nojaf
Copy link
Member Author

nojaf commented Feb 11, 2019

@MangelMaxime yeah tried that, it doesn't load all the necessary references that way.
#r @"C:\Users\nojaf\.nuget\packages\netstandard.library\2.0.3\build\netstandard2.0\ref\netstandard.dll" adds more references than #netstandard. //cc @enricosada

@alfonsogarciacaro
Copy link
Member

In fable-compiler-js we're injecting the necessary references by ourselves, so maybe we could do the same here. The main need for Msbuild is to load other files properly instead of just using regex (for example, if #load is surrounded by #if !FABLE_COMPILER).

My main concern about scripts is it's currently more cumbersome to add references to packages than generating an SDK-style fsproj.

@nojaf
Copy link
Member Author

nojaf commented Feb 11, 2019

Hi @alfonsogarciacaro, could you elaborate a bit more, please?

  • inject references ourselves: you mean adding references_core by default?
  • more cumbersome, what do you mean by this? Are you talking about the experience for the end-user?

@alfonsogarciacaro
Copy link
Member

Yes 😸

  • more cumbersome, what do you mean by this? Are you talking about the experience for the end-user?

Also yep. Anything that requires paths to external folders in your local machine becomes problematic when shared in a repo. And the only way I know to use packages with an .fsx is to use Paket with the generate_fsx_scripts options, which requires some setup and kind-of defeats the purpose of using an .fsx to test things quickly.

@enricosada
Copy link
Contributor

  • the *.fsx script requires a reference to netstandard2.0.

@nojaf what's the limitation you see in this issue and fsxBinder.GetProjectOptionsFromScriptBy there?

@nojaf
Copy link
Member Author

nojaf commented Feb 11, 2019

@enricosada I think the fsxBinder.GetProjectOptionsFromScriptBy always assumes full framework based on what netFwInfo.LatestVersion () returns.

@nojaf nojaf closed this Feb 11, 2019
@nojaf nojaf reopened this Feb 11, 2019
@enricosada
Copy link
Contributor

You can choose the version, but only .NET full is considered.

There isn’t an FSI for .net core yet, that’s why i didn’t support that

@nojaf
Copy link
Member Author

nojaf commented Feb 11, 2019

The main ux problem here is probably when you want to open Fable.Core. I don't think it is currently present in the node_modules.
So you might want to add something like #r @"node_modules\fable-compiler\bin\Fable.Core.dll".

For all other deps, paket would be the only usable option atm. Maybe that can improve in #5850.

@nojaf
Copy link
Member Author

nojaf commented Feb 12, 2019

Discovered fable-metadata today!
That contains all dlls that might be missing. This seems good enough for now.

It also solves the problem of getting intellisense when using vanilla Fable.
You can do something like:

#r "./node_modules/fable-metadata/lib/Fable.Core.dll"

open Fable.Core

@ncave
Copy link
Collaborator

ncave commented Feb 12, 2019

@nojaf Just keep in mind those binaries in fable-metadata don't contain code, just metadata.

@nojaf
Copy link
Member Author

nojaf commented Feb 15, 2019

@ncave I believe that is all I need, do you see any harm in this approach @alfonsogarciacaro?

@alfonsogarciacaro
Copy link
Member

@nojaf Well, the main issue is fable-compiler package doesn't have a dependency to fable-metadata so it's not safe to assume the dlls will actually be there. Maybe you can use some basic types to detect the location of the assemblies:

Path.GetDirectoryName(typeof<Int32>.GetTypeInfo().Assembly.Location) // Directory to basic assemblies
typeof<Option<obj>>.GetTypeInfo().Assembly.Location // Location of FSharp.Core

@nojaf
Copy link
Member Author

nojaf commented Mar 3, 2019

@alfonsogarciacaro @enricosada I stole like a lot of code from Dotnet.ProjInfo.Workspace.FCS to make this work again. How could I use the Workspace.FCS nuget in combination with the custom fcs dll?

@alfonsogarciacaro
Copy link
Member

Sorry @nojaf, I missed the notification. What's the difference between Dotnet.ProjInfo and Dotnet.ProjInfo.Workspace.FCS?

@nojaf
Copy link
Member Author

nojaf commented Mar 6, 2019

FsxBinder is used to get the projectoptions from the script.
That part of the code lives in Dotnet.ProjInfo.Workspace.FCS, and that has a dep on the nuget FCS.

<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>netcoreapp2.1</TargetFramework>
<RuntimeFrameworkVersion>2.1.0</RuntimeFrameworkVersion>
</PropertyGroup>
<ItemGroup>
<Compile Include="../fable-standalone/src/Metadata.fs" />
Copy link
Member

Choose a reason for hiding this comment

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

We have a similar list to Metadata.references_core in Fable.Cli/Util.fs (SYSTEM_CORE_REFERENCES), maybe we could unify them somehow.

// TEMP NOJAF
module Utils =

let runProcess (log: string -> unit) (workingDir: string) (exePath: string) (args: string) =
Copy link
Member

Choose a reason for hiding this comment

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

There's a similar helper to this in Fable.Cli/Util.fs (Process.start).

if not (String.IsNullOrEmpty ev) then [ev]
else EnvUtils.tryFindPath (sideBySidePaths @ legacyPaths) "MsBuild.exe"

type MSBuildLocator () =
Copy link
Member

Choose a reason for hiding this comment

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

We should only call MBuild through dotnet SDK (dotnet msbuild). If we want to allow non-global dotnet locations we should parameterize it, because it's also used in other locations (when calling dotnet restore, when the dotnet process is spawned from node...).

static member FromPATH (msbuildLocator: MSBuildLocator) =
{ NetFWInfoConfig.MSBuildHost = msbuildLocator.MSBuildFromPATH }

module internal NETFrameworkInfoProvider =
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't worry about different NET Framework versions, we can just assume netstandard2.0 which is what Fable supports.

yield "System.Web.Services"
yield "System.Windows.Forms"
yield "System.Numerics"
]
Copy link
Member

Choose a reason for hiding this comment

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

We have this list but also a reference to Metadata.references_core. Why are both needed?

// END TEMP

let getProjectOptionsFromScript scriptFile =
let (checker: FSharpChecker) = FSharpChecker.Create(keepAssemblyContents=true)
Copy link
Member

Choose a reason for hiding this comment

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

This checker won't be used to read the AST so we shouldn't need keepAssemblyContents. Also, there seems to be a high bootstrap time to initialize a checker, I hope it's not the case here.

let tfm = netFwInfo.LatestVersion () // or specify a .NET version like `"4.6.1"`
let fsxBinder = FsxBinder(netFwInfo, checker)

let input = System.IO.File.ReadAllText(scriptFile)
Copy link
Member

Choose a reason for hiding this comment

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

Files should be read in non-blocking fashion to prevent issues. See

/// File.ReadAllText fails with locked files. See https://stackoverflow.com/a/1389172
let readAllText path =
use fileStream = new IO.FileStream(path, IO.FileMode.Open, IO.FileAccess.Read, IO.FileShare.ReadWrite)
use textReader = new IO.StreamReader(fileStream)
textReader.ReadToEnd()

dllPath::dllRs
else
dllRs
) dllRx Fable.Standalone.Metadata.references_core
Copy link
Member

Choose a reason for hiding this comment

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

If you're adding here the basic System assemblies necessary to compile Fable projects, what's the rest of the code doing, just getting custom #r references and file #loads?

Also, Metadata.references_core only includes assembly names not their location in the machine. How is this solved.

@alfonsogarciacaro
Copy link
Member

TBH, I'm a bit reluctant to add a dependency without knowing well what it does. The README in enricosada/dotnet-proj-info doesn't mention the Dotnet.ProjInfo.Workspace packages and the name seems to suggest they are related to the Workspace functionality in VS Code which doesn't have anything to do with Fable 🤔

@enricosada
Copy link
Contributor

enricosada commented Mar 7, 2019

Sorry for late reply.

@alfonsogarciacaro the Dotnet.ProjInfo.Workspace and Dotnet.ProjInfo.Workspace.FCS are the helper libraries to parse and fsproj and load all info, to get finally the FSharpProjectOptions ready for initialize Fsharp.Compiler.Service.

I have a PR open in FSAC ( ionide/FsAutoComplete#347 ) to replace all the code to navigate project/get fsc arguments/etc, using that library instead.
It will do all project parsing, for old sdk and fsx too, removing the old ProjectCracker
The starting code is pretty much the same of FSAC for .net sdk project loading (i refactored out FSAC code), but refactored as library plus fixes and features.
It will allow to iterate faster and fix bugs, in FSAC was too entangled.
In that repo i can test all scenario, in isolation (like https://github.com/enricosada/dotnet-proj-info/blob/master/test/Dotnet.ProjInfo.Workspace.FCS.Tests/Tests.fs#L199-L412 )

In fable will eventually replace this code https://github.com/fable-compiler/Fable/blob/e965930ec30937ccc8d9613684a3fc98ad491196/src/Fable.Cli/ProjectCoreCracker.fs with one or two lines (same feature, but maintained in a library with fixes, and some more like choose dotnet path etc).

NOTE i am dogfooding the library in FSAC atm, it's not ready yet that's why is not publicly announced yet. i have one or two bugs more to fix.
When done i'll send a PR to fable to replace that code, so will use the same FSAC/vscode will use.

@alfonsogarciacaro alfonsogarciacaro mentioned this pull request Mar 8, 2019
@alfonsogarciacaro
Copy link
Member

Thanks for explanations @enricosada. That's very good to hear, I'd be very happy to remove the ProjectCoreCracker file because right now I wouldn't know how to edit it in case of need. A couple of notes about the current Fable situation if you want to send a PR :)

  • As said above, right now we're not using FCS from Nuget but a custom build from ncave's fork. Adding a dependency which depends on Nuget FCS is going to be problematic.
  • Dealing with Nuget, MSBuild, frameworks, etc. is the most frustrating part of Fable development. Because of this, I'm not interested in supporting other scenarios than dotnet SDK format projects. The only thing we need is parsing "modern" .fsproj files (and maybe .fsx) as we do now. Ideally also being able to pass a custom "dotnet" location.
  • Fable is not responsible for watching the files (clients like Webpack or fable-splitter do that) so we cannot subscribe to a notifier for file changes.

@enricosada
Copy link
Contributor

enricosada commented Mar 8, 2019

  • As said above, right now we're not using FCS from Nuget but a custom build from ncave's fork. Adding a dependency which depends on Nuget FCS is going to be problematic.

Final will be like this, that's why already exists Dotnet.ProjInfo.Workspace nupkg (without FCS reference, unlike of Dotnet.ProjInfo.Workspace.FCS built on top of that).
Fable will use that. I ❤️ fable 😄 , happy to help as needed.

  • Dealing with Nuget, MSBuild, frameworks, etc. is the most frustrating part of Fable development. Because of this, I'm not interested in supporting other scenarios than dotnet SDK format projects. The only thing we need is parsing "modern" .fsproj files (and maybe .fsx) as we do now. Ideally also being able to pass a custom "dotnet" location.

Np, is transparent for the library. if you want to use modern fsproj (best obv), is ok.
Custom dotnet is already supported.

  • Fable is not responsible for watching the files (clients like Webpack or fable-splitter do that) so we cannot subscribe to a notifier for file changes.

There is no file changes watcher. When will be added in a later version, will be optional and built on top of base functions.
Atm the notification you see are just .NET event to see progress, completly optional.

Thank for the info @alfonsogarciacaro better be sure will be ok to integrate, when ready!

@nojaf
Copy link
Member Author

nojaf commented Mar 15, 2019

Closing in favor of #1780

@nojaf nojaf closed this Mar 15, 2019
@nojaf nojaf deleted the fsx branch March 15, 2019 09:50
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.

None yet

5 participants