Let compiler run on more restrictive platforms like WASM #12163
Conversation
This works towards allowing the use of the FSI in cases where System.Diagnostics.Process is not available (like Blazor). This obviously still has caveats, namely that timing *cannot* be turned on, but this gets the job done.
…aces preserved normally so that it is still visible
…tionAssemblies If the platform is "other", then ToolLocationHelper may throw, and generally cause problems. One example of this is the WASM platform, where System.Diagnostics.Process isn't available.
|
|
|
@nike4613 A couple of things
Overall we'd also need to be confident that we could understand and fix failures should we hit restrictions. That's tricky without deeper knowledge of the toolchain. We'd certainly need lots of documentation and links. Also if something goes wrong, we'd likely turn off WASM testing and support. An alternative can be for the part of the F# community who care about running on WASM maintain a fork where you can arrange for WASM-based automated testing to happen. That's what happens for FCS running on Fable, for example. |
|
Unfortunately, as far as I can tell, there is no single source of documentation for the restrictions of any specific platform. However, the BCL has UnsupportedOSPlatforms properties, which generate UnsupportedOSPlatform attributes, and there are also SupportedOSPlatform attributes, all of Roslyn has a platform compatibility analyzer which emits warnings when using an API which are potentially incompatible with some platforms, even taking into account basic runtime checks to My current experience working with Blazor WASM has been that, excluding APIs which are unavailable, if it works on a desktop runtime, then it will work in-browser. I would even argue that if there was a case where that was not true, then that would be a runtime bug in Mono. That being said, I'll probably look into automated testing of WASM assemblies. |
| @@ -370,7 +370,8 @@ module internal FSharpEnvironment = | |||
| let getDefaultFsiLibraryLocation() = Path.Combine(getFSharpCompilerLocation(), fsiLibraryName + ".dll") | |||
|
|
|||
| // Path to the directory containing the fsharp compilers | |||
| let fsharpCompilerPath = Path.Combine(Path.GetDirectoryName(typeof<TypeInThisAssembly>.GetTypeInfo().Assembly.Location), "Tools") | |||
| //let fsharpCompilerPath = Path.Combine(Path.GetDirectoryName(typeof<TypeInThisAssembly>.GetTypeInfo().Assembly.Location), "Tools") | |||
| // ^^ that isn't actually *used* anywhere | |||
There was a problem hiding this comment.
I believe I removed the last thing that used it a few weeks ago. I thought i had removed this too.
|
The PreserveStackTrace change is a good change regardless of WASM. |
| @@ -204,16 +204,16 @@ module internal Utilities = | |||
| [<AutoSerializable(false)>] | |||
| type internal FsiTimeReporter(outWriter: TextWriter) = | |||
| let stopwatch = Stopwatch() | |||
| let ptime = Process.GetCurrentProcess() | |||
| let ptime = lazy Process.GetCurrentProcess() | |||
There was a problem hiding this comment.
Why is this changed to Lazy? I don't really understand why it would be necessary?
There was a problem hiding this comment.
If it isn't, then that call is evaluated eagerly when FsiTimeReporter is constructed, even if its never actually used. I admit, it was more of a quick way to make it behave where Process isn't available, and it would probably be better to lazily construct FsiTimeReporter instead, but this was easier to find and solved Process throwing.
There was a problem hiding this comment.
I think I get it. Is Process.GetCurrentProcess() particularly expensive?
There was a problem hiding this comment.
No, the problem is that on WASM it throws unconditionally. By making it lazy, it's never evaluated until its needed for timing, which is off by default.
There was a problem hiding this comment.
This sort of thing is too subtle, especially without automated testing. If I saw this lazy in code cleanup I'd just remove it.
Is there any boolean condition that can be used to check if GetCurrentProcess is supported or not?
Otherwise I would prefer the use of try/with to protect against the exception.
We really do need automated testing on WASM before we could merge this I think.
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
I'm going to close this because merging it is only possible with full automated testing on WASM, and that's too subtle for this repo, especially given the kinds of changes needed. Like the Fable port of F# Compiler Service, it's currently best done out of repo by a compiler for with automatic merging of changes here. |
This PR does a couple of things:
lazymost accesses anything inSystem.Diagnostics.ProcessEnvironment.OSVersion.PlatforminGetPathToDotNetFrameworkImlpementationAssembliesso that it doesn't call into MSBuild utilities on theOtherplatform, as that tries to spin up some threads which accessSystem.Diagnostics.Processand cause errors when running in Blazor.FSharpEnvironment.fsharpCompilerPathbecause it was not used, and its eager evaluation caused errors in Blazor.I am almost certain that this will need a little bit of cleanup or refactoring, but the increased portability would be incredibly nice to have.