.Net Core Support - Githubissues

rkoeninger commented 6 years ago

@jaccarmac I saw your working branch and wanted to go over the solution approach.

It seems most of the projects have been duplicated with Framework versions and Core versions. The new Core projects use the new VS2017 proj file format, of which I approve and think they should all be migrated to that proj format.

There's no code under Kl.Make.Net45.

But there should just be one project. You can specify multiple target frameworks in the proj file and then include different libraries and even different lines of code (using #if) that are specific to a framework.

Unsure: should the libraries targe .Net Standard and the executables target both Core and Framework or should it all target Core and Framework?

As for the Core F# compiler, if it targets .Net Standard, then we could use just the one compiler for both Framework and Core and avoid having two different Compiler.fs files.

Don't want the scope of this to expand, just want to get Core support without changing too much else, but a couple of long-term goals to keep in mind:

Want to be able to include Compiler.fs in the Kl.dll library so we can do on-the-fly compilation. I'm imagining Shen projects that have a project.shen or build.shen file that loads up the source, does whatever magic and then calls shen-sharp.save-executable which compiles the code and spits out a .exe.
Want to see if the 2-solution approach can be abandoned. If Kl.Make can be made to run faster, the generation of Shen.Language.dll can be a pre or post build action in one of the proj files.

I wouldn't want you to put a lot of work into your contribution and then it's rejected or needs to be redone because of something that could have been clarified before starting. It really discourages future contributions and I hate it when it happens to me.

jaccarmac commented 6 years ago

Thanks for the update!

The double-project approach is definitely a result of the working branch being a working branch. I was able to get the existing code running on .NET Core on Windows fairly easily by nuking every project and creating new ones with the old source code. But I was unsure about compatibility with .NET 4.x, so on this branch those projects are enabling the CI builds for now.

Pretty sure standard practice is to target Standard now for libraries, but multi-targeting is really easy in the new format so either way should work about as easily.

Meanwhile I've run into a couple issues in this working branch that I didn't encounter doing naive Core stuff on Windows.

Compiling doesn't quite work in Kl.Make. Even on Windows I had to add a reference to FSharp.Core.dll and on Linux I need to tweak the dependency list even more and pass --noframework. There's also some weirdness around the resulting assembly's relationship to System.Runtime. Certain issues (fsharp/FSharp.Compiler.Service#857 + fsharp/FSharp.Compiler.Service#863) lead me to believe I need to add a --targetprofile flag but I haven't figured out how to do that for the AST version of Compile.
Updating FSharp.Compiler.Service broke the AppVeyor build in a bizarre way. I backburnered it since Travis builds still work and hopefully when the *.Net45 projects get deleted the problem will go with them.

Re: Future directions:

On-the-fly compilation is a really cool idea. I need to figure out what exactly the FSharp.Compiler.Service failure modes I'm seeing are, but it was definitely something on my mind as well.

I haven't written pre- or post-build actions before but if they can do the same thing a build script does I'm all for it! And in that same vein dependency hell has half-convinced me to try integrating Paket and FAKE into the Core update. That might help solve some of those problems as well.

rkoeninger commented 6 years ago

How can this task be broken down into individual steps that each result in working code?

Migrate the proj files to the newer format.

This can be done while still targeting only Framework and shouldn't change anything else. It might be easier to edit the proj files directly. Example here.

???

Haven't figured out how to break the rest of it broken down yet.

Just cut a new branch and do step 1 in that, open a PR and we'll merge that first.

If tooling like paket or fake need to be introduced, I'd want each of those to be done first and merged in independently.

You can use your current working branch for experimenting, and then do the work cleanly on individual branches.

jaccarmac commented 6 years ago

Sounds like a plan. I avoided it at first since without Visual Studio the process is somewhat fraught, but I'm having a try now. I see you've already done one conversion, and following your AppVeyor config seems to almost work. Still fails, but in a way I haven't seen yet.

jaccarmac commented 6 years ago

Yeah looks like AppVeyor is respecting all my package versions except for FSharp.Core where it's pulling 4.5.2 instead of 4.0.0.1. 4.5.2 will eventually get installed, just not yet :).

jaccarmac commented 6 years ago

AppVeyor does not want to play nice with the new project format. Originally I thought this was due to some weird resolution logic, but pinning FSharp.Core with Paket didn't help. My only insight into the Windows build at the moment is AppVeyor, so I can't see what's trying to resolve the 4.3.0.0 version of FSharp.Core.

Failing AppVeyor build without Paket. Successful Travis build without Paket. Failing AppVeyor build with Paket. Successful Travis build with Paket.

jaccarmac commented 6 years ago

Problems were due to binding redirects and the task copying FSharp.Core.sigdata/FSharp.Core.optdata. I solved both with the help of Paket. In theory each of those problems can be solved without Paket but I've not been successful on that front yet.

Binding redirects: Using App.config without Paket doesn't solve the issue of AppVeyor restoring the wrong version of FSharp.Core, and that causes a build failure later.

Copying files: The new MSBuild restore task doesn't restore to packages any more. The Paket website claims that "csproj/fsproj files can references[sic] files in the global cache" but I haven't been able to find out how to do this.

So, I can keep working on a PR for step 1 when I find solutions, or we can get a working 4.5 build by adding Paket. There's an example of the latter option on my paket branch.

rkoeninger commented 6 years ago

I tried out your paket branch. I'm running the most recent update of Visual Studio 2017. I've worked on ShenSharp on this machine before.

Visual Studio told me I need to restore NuGet in order to generate project.assets.json, which then appeared in each project's obj/ folder. I've never heard of that file before. Not sure why that was necessary.

Trying to run Kl.Get and Kl.Make fails with a message like:

'Could not find a part of the path 'C:\Users\rkoeninger\Workspace\Me\ShenSharp\Kl.Make\packages\ShenOSKernel-21.0\klambda\toplevel.kl'

This just requires adding another .. to the relative paths. Working dirs are nested under another level of folder because of the file new format.

jaccarmac commented 6 years ago

Yeah, I need to update the paths. I did on my experiment branch but doctored the AppVeyor and Travis builds on this one instead.

Is the Paket branch what you want to move forward with?

(The JSON in the obj folder is the last relic of the JSON-based .NET Core flow, AFAIK. Gets done as part of the restore step when using the CLI as well.)

rkoeninger commented 6 years ago

If the paket and proj format work can't be separated, then might as well move forward with the paket branch.

Not sure why paket must be used, though.

jaccarmac commented 6 years ago

The AppVeyor build for the non-Paket branch keeps restoring the wrong version of FSharp.Core. I have no idea why that's happening, if that can be resolved I'm pretty sure the rest of the build would work without Paket. Have any insights on that?

rkoeninger commented 6 years ago

FSharp.Core is usually included with Visual Studio and the new proj format must be taking that copy at a higher priority over one you fetch from NuGet. There must be a way to override reference assemblies in the new format.

Is this an issue in every proj, or just particular ones?

jaccarmac commented 6 years ago

Seems to be an issue for everything.

Does that mean to update to VS2017 we have to use newer versions of dependencies? (For example, FSharp.Compiler.Services targets specific F# versions from what I can see.)

jaccarmac commented 6 years ago

Yeah, updating binding redirects breaks the test DLL. Perhaps I'm misunderstanding binding redirects and need to add still more of them. The two in the repo are the only ones necessary for the Paket build to work with 4.0.0.1. So right now I'm reading the solution as either.

Upgrade FSharp.Core and don't use Paket.
Use Paket and upgrade FSharp.Core later.

rkoeninger commented 6 years ago

Binding redirects should only come into play when you have conflicting transitive dependencies. So you're using libraries X and Y. X wants Z version 1.2 and Y wants Z vesrion 1.5. You might resolve this by adding a binding redirect of 1.2.0 -> 1.5.0 for library Z. I think that's how it works.

Let's just try to avoid the need for them at all, updating FSharp.Core, the F# language version and the FSharp compiler library so all the versions match.

jaccarmac commented 6 years ago

Cool, I'll try that. FYI, I also ran into fsharp/FSharp.Compiler.Service#857 when updating the dependency for .NET Core purposes. So that's a PR I'll need to land upstream before everything plays nice. Hopefully won't affect the .NET 4.5 update.

rkoeninger commented 6 years ago

Version 8.0.0 of Fsharp.Compiler.Service, which is currently being used, depends on 4.0.0.1 of FSharp.Core, which might explain the version clash.

jaccarmac commented 6 years ago

On top of that even with redirects I'm getting errors. See the more recent AppVeyor builds for examples. I guess pinning FSharp.Core isn't possible unless you're using Paket?

jaccarmac commented 6 years ago

Alright, I have the Travis build working with updated versions of things. AppVeyor is failing now with, of all things, a stack overflow. Would you mind taking a look at https://github.com/jaccarmac/ShenSharp/commit/786dcd671cac8a983dd8d83c9fa6900fde3eb8cd in VS2017?

rkoeninger commented 6 years ago

The code to identify the version of F# is a little off: *language* = "F# 5.0"

StackOverflow when compiling kernel, must be new version of FSharp, Compiler.... I don't know.

It seems like multiple things need to be figured out here and some of this project will need to be re-done. Like I said the other week, I wanted to break the upgrade down into discrete steps, but it's hard to untangle all these issues: proj format, core, fsharp version, fsharp compiler, nuget/paket...

I'm considering re-writing this using FAKE and saying the official way to work on it is with VS Code instead of VS. I like intellisense, but at what cost?

jaccarmac commented 6 years ago

I'll vouch for VSCode having nice Intellisense. When I got my hacked-together build working with .NET Core the completion in Ionide was perfectly functional.

Haven't worked with FAKE yet but Paket was great to use, so my experience with the open source F# stuff so far has been sunny.

jaccarmac commented 5 years ago

Interested in revisiting this as I haven't hacked in .NET land for a while. Hopefully the tooling has matured a bit, though I haven't fiddled with my branches just yet. Are you still interested in a smooth transition to Core or has it been long enough since a release that a clean break would be less of a headache?

rkoeninger commented 5 years ago

@jaccarmac Honestly, I haven't thought about this for a while. Maybe it would be easier to move to 'Core if the kernel compilation was left out but I'd have to double-check what the startup time and overall performance impact would be. (Shen Kernel is compiled F# code, code loaded later is just an interpreted AST).

jaccarmac commented 5 years ago

Yeah, there was definitely something wrong with the F# compiler tools that I was intending to patch upstream but never did. For some reason I never mentioned it in this thread, so I'll have to hunt a little.

rkoeninger commented 4 years ago

@jaccarmac I'm back on this. Just started working off the master branch. Having a problem when generating the Shen.Kernel.dll on travis: https://travis-ci.org/rkoeninger/ShenSharp/builds/646164510.

It says:

Unhanded exception. System.Exception: unknown (1,1)-(1,1) parse error Assembly reference 'mscorlib.dll' was not found or is invalid

And when I look at the generated assembly built on Windows, I see it has an necessary (?) reference to mscorlib.dll. I'm not sure how to invoke the compiler service to prevent it from adding that.

Is this the upstream issue you referred to?

jaccarmac commented 4 years ago

If I recall correctly it was something like that. Wish I had commented on the issue I found so I could find it again, because I never got as deep as inspecting the assemblies. From what I remember of the compiler service code, there was a missing branch for creating .NET Core assemblies. It did have to do with libraries, but I don't recall if mscorlib in particular was a pain point.

jaccarmac commented 4 years ago

https://github.com/fsharp/FSharp.Compiler.Service/issues/863#issuecomment-596809599

Thankfully this popped back into my notifications, I had missed that it was already linked.

Also found dotnet/roslyn#16211. AFAICT, mscorlib is pretty important, but I'm not familiar with the intricacies of how it interacts with .Net Core and haven't done any Core work in a while; The tooling may have evolved greatly.

jaccarmac commented 3 years ago

I see that the README has been updated: Is this issue obsolete now or is the .NET Core support still aspirational?

rkoeninger commented 3 years ago

Well, in the years since starting this initiative, .NET Core isn't even new, now targeting .NET 5 is the goal. (and .NET 6 preview is out).

The main branch was left in a broken state and I hardly remember what was wrong with it. I think it was a compatibility problem the FCS library again.

In the meantime, I started thinking about re-writing the whole thing in one of two ways:

An Expression-tree based compiler that can easily embed references to globals with Expression.Constant(object) can be fairly accessible to implement.
A direct IL-generating compiler that can take advantage of the CLR's .tail call instruction, removing the need for Trampolines. Not sure how to embed global pointers yet, and there would be a lot of messy details of generating jumps in or out of trys.

Either way, there's be less benefit to doing either approach in F#, so might just go with C#.

jaccarmac commented 10 months ago

I've started beating the dead horse once again, finding that, at least seemingly, I can get past the mscorlib errors by upgrading to .NET 5 and Compiler Service 38. There are still errors and I figure I bungled the long identifier code.

jaccarmac commented 10 months ago

On to the update-to-41 and https://github.com/dotnet/roslyn/issues/16211 is looking like something I need to understand more deeply. As I grok it, the listing of assemblies in the loader shouldn't be a problem, as they're just names.

jaccarmac commented 10 months ago

Checked the generated F# against the old version and that doesn't seem to have changed.

The hosted compiler docs are a little all over the place (https://fsharp.github.io/fsharp-compiler-docs/fcs/compiler.html); In any case we'll eventually have to move to the string-based Compile format, and playing with an example based on https://github.com/dotnet/fsharp/issues/13923 has gotten me different errors, still no working DLL.

jaccarmac commented 10 months ago

I had missed the whole point of Writer so was tilting at windmills a bit :man_facepalming:

But DLL-from-AST is gone from FCS (https://github.com/dotnet/fsharp/pull/13966), which means a like approach may be the way to go. Fantomas (https://fsprojects.github.io/fantomas/docs/end-users/GeneratingCode.html) seems like a more robust way than the custom printers; While I haven't looked deeply at the generator and generated source, errors against the source code suggest that the globals-handling isn't getting printed properly.

Runtime things still don't work properly. I get a long list of warnings and then a failure.

/usr/share/dotnet/sdk/6.0.417/Microsoft.Common.CurrentVersion.targets(2302,5): warning MSB3277: Found conflicts between different versions of "System.Runtime" that could not be resolved. [/ShenSharp/src/Shen/Shen.fsproj]
/usr/share/dotnet/sdk/6.0.417/Microsoft.Common.CurrentVersion.targets(2302,5): warning MSB3277: There was a conflict between "System.Runtime, Version=4.1.2.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" and "System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a". [/ShenSharp/src/Shen/Shen.fsproj]
/usr/share/dotnet/sdk/6.0.417/Microsoft.Common.CurrentVersion.targets(2302,5): warning MSB3277:     "System.Runtime, Version=4.1.2.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" was chosen because it was primary and "System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" was not. [/ShenSharp/src/Shen/Shen.fsproj]
/usr/share/dotnet/sdk/6.0.417/Microsoft.Common.CurrentVersion.targets(2302,5): warning MSB3277:     References which depend on "System.Runtime, Version=4.1.2.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" [/usr/share/dotnet/packs/NETStandard.Library.Ref/2.1.0/ref/netstandard2.1/System.Runtime.dll]. [/ShenSharp/src/Shen/Shen.fsproj]
/usr/share/dotnet/sdk/6.0.417/Microsoft.Common.CurrentVersion.targets(2302,5): warning MSB3277:         /usr/share/dotnet/packs/NETStandard.Library.Ref/2.1.0/ref/netstandard2.1/System.Runtime.dll [/ShenSharp/src/Shen/Shen.fsproj]
/usr/share/dotnet/sdk/6.0.417/Microsoft.Common.CurrentVersion.targets(2302,5): warning MSB3277:           Project file item includes which caused reference "/usr/share/dotnet/packs/NETStandard.Library.Ref/2.1.0/ref/netstandard2.1/System.Runtime.dll". [/ShenSharp/src/Shen/Shen.fsproj]
/usr/share/dotnet/sdk/6.0.417/Microsoft.Common.CurrentVersion.targets(2302,5): warning MSB3277:             /usr/share/dotnet/packs/NETStandard.Library.Ref/2.1.0/ref/netstandard2.1/System.Runtime.dll [/ShenSharp/src/Shen/Shen.fsproj]
/usr/share/dotnet/sdk/6.0.417/Microsoft.Common.CurrentVersion.targets(2302,5): warning MSB3277:     References which depend on "System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" []. [/ShenSharp/src/Shen/Shen.fsproj]
/usr/share/dotnet/sdk/6.0.417/Microsoft.Common.CurrentVersion.targets(2302,5): warning MSB3277:         /ShenSharp/kernel/dotnet/Debug/Shen.Kernel.dll [/ShenSharp/src/Shen/Shen.fsproj]
/usr/share/dotnet/sdk/6.0.417/Microsoft.Common.CurrentVersion.targets(2302,5): warning MSB3277:           Project file item includes which caused reference "/ShenSharp/kernel/dotnet/Debug/Shen.Kernel.dll". [/ShenSharp/src/Shen/Shen.fsproj]
/usr/share/dotnet/sdk/6.0.417/Microsoft.Common.CurrentVersion.targets(2302,5): warning MSB3277:             Shen.Kernel [/ShenSharp/src/Shen/Shen.fsproj]
Unhandled error: Method not found: 'Kl.Function Kl.Function.NewCompiled(System.Int32, Microsoft.FSharp.Core.FSharpFunc`2<Kl.Globals,Microsoft.FSharp.Core.FSharpFunc`2<Microsoft.FSharp.Collections.FSharpList`1<Kl.Value>,Kl.Value>>)'.

I suspect that the --noframework argument added in feb9dfb8968ddc2450d2c3109214c0611eb127cf is not exactly what's needed? With it and the deps list, the DLL compiles, but all indications are that the DLL is built against Kl.Make's runtime instead of netstandard2.1.

At this point, I need to fix my development environment so I can actually get inside the code and debug it; I'm not good enough to pull apart DLLs for the answers.

jaccarmac commented 10 months ago

Poking around in ILSpy and fsi suggests that the kernel DLL is generating calls to Function.NewCompiled. Not sure why that's happening; The source code doesn't seem to be generating AST nodes which would do that. Could be an F# version mismatch which I couldn't figure out how to fix (with explicit assembly references). FSC says that it compiles against F# 3 by default (https://github.com/fsharp/fsharp-compiler-docs/issues/156). I'm also looking at changes to the function-building code, especially f6e1496c4533b9933a6c5d4e44c44a5133985a4a.

jaccarmac commented 10 months ago

As of 90200b6c4ee0ac14448c0734dc623d25944aa43d, I can boot into a REPL (with slightly garbage output) and the test suite succeeds! The last step was making Shen.Kernel an explicit project and building it as part of the REPL chain.

Interestingly enough, pretty-printing the kernel source code with Fantomas causes more errors around the sequential initializers at the end; Choking on parens I'm pretty sure.

rkoeninger / ShenSharp

.Net Core Support #6