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

FSI assembly resolution logic doesn't work for self-contained FCS scenarios. #7701

Closed
baronfel opened this issue Oct 4, 2019 · 12 comments
Closed

Comments

@baronfel
Copy link
Member

baronfel commented Oct 4, 2019

The logic in DotNetFrameworkDependencies is written assuming that the getFSharpCompilerLocation is inside the dotnet SDK root directory tree. This is fine for the compiler, but for FCS deployed as a component in some other application (ie. FSAC):

  • the path reported by getFSharpCompilerLocation is not inside this tree
  • which makes the overall logic for probing the getFrameworkRefsPackDirectoryPath no longer valid
  • resulting in the ref assemblies from the pack directory not being used.

Instead, the full assemblies from the shared directory are used. As a side effect of the getImplementationReferences() code path being used, it becomes necessary to insert a manual reference to mscorlib.dll, without which no fsx script will typecheck. This wouldn't be necessary if the packs codepath was used, because that codepath yields every DLL in the packs folder.

Repro steps

  1. In a project targeting/running on netcoreapp3.0,
  2. call backgroundCompiler.GetProjectOptionsFromScript() with a valid file, setting useSdkRefs to true and assumeDotNetFramework to false.
  3. get the references in the OtherOptions by filtering all options that start with -r:.

Expected behavior

The refs are supplied from the pack references, in my case /usr/local/share/dotnet/packs/Microsoft.NETCore.App.Ref/3.0.0/ref/netcoreapp3.0

Actual behavior

The refs are supplied from the full references, in my case /usr/local/share/dotnet/shared/Microsoft.NETCore.App/3.0.0/

Known workarounds

Reimplement reference location :(

Related information

In addition, these APIs are very hard to use in a cross-TFM manner. They are tightly bound to the execution TFM of the currently-running process so you can't for example ask for the references set of a .netcoreapp3.0 script while running FCS in a .netcoreapp2.1 application. This problem doesn't exist when asking for net4x references because those are an immutable set and are just magically resolved by the system by name.

Provide any related information (optional):

  • Operating system: Max OSX Mojave
  • .NET Runtime kind: .Net core 3.0
  • Editing Tools: FSAC work-in-progress branches
@baronfel
Copy link
Member Author

baronfel commented Oct 4, 2019

I'd be willing to work on a refactor/rewrite of this code, but I'd like to get a get set of requirements before running down that path.

It seems that for the .net core side, you need two things in order to get the full set of necessary refs:

  • root of the dotnet sdk/runtime installations
  • the SDK version (for finding the compiler + FSI interactive dlls)
  • the Runtime version (for finding the ref/implementation dll root paths)
  • the TFM for which to retrieve refs.

If we implemented a function getRefs: root -> sdk -> runtime -> tfm -> string list, such a function could then derive 'compiler paths', 'pack paths', and 'implementation paths' in terms of relative paths from the root, then dive into the TFM-specific subpaths for that and yield all the dlls therein, instead of using the dependencies-of logic we have today.

@cartermp
Copy link
Contributor

cartermp commented Oct 4, 2019

cc @KevinRansom

@KevinRansom
Copy link
Member

I will add support for a dotnet_hostpath environment variable. The host app will be responsible for ensuring that path is set to dotnet.exe.

This is what the dotnet sdk uses as it's final fallback to find dotnet.exe.

Do you think that will work?

@baronfel
Copy link
Member Author

baronfel commented Oct 8, 2019

I've got a pretty fully fleshed out implementation in FSAC that was based off of the FCS version, was going to PR it here if you all thought it was reasonable:

https://github.com/fsharp/FsAutoComplete/blob/master/src/FsAutoComplete.Core/FSIRefs.fs

The main entrypoint is the function where you pass in a dotnet root, sdk version, and runtime version and get the full reference set. This seems to be working quite well for us in tests so far.

@KevinRansom
Copy link
Member

@baronfel,

So … DotNetFrameworkDependencies is an implementation detail of fsi. The probing it does and resolutions that it reaches are not intended to be made available to hosts. Those resolutions are certainly not expected to be a contract to which we wish to hold, or even support. We are satisfied with the probing we do because we know that if the dotnet sdk change stuff, we will be broken and know it before we ship - because we ship inside the sdk - which gives us time to fix things, that is not something we can do when the probing ships externally to the S; in fact: I think that we will probably have exactly this problem with the notebook variant of fsi, which will ship at least initially as a global tool.

The correct way for a host to determine what assemblies to be considered is for it to collect what it thinks the assemblies for references and execution need to be based on user preferences and the platform it knows it is running on and then passing them to the FSI session, that way the code DotNetFrameworkDependencies shouldn't be relied on to grab dependencies. This is how fsi.exe should have been originally, however, even with several refactors it still isn't. Primarily because of desktop compatability I suppose.

Anyway, your PR looks like a fine Api, for you to provide separate from the dotnet/fsharp, in order to collect the dependencies you need for hosts. However, we wouldn't want it to be added in this repo, because it essentially, provides an API that the dotnet SDK should provide and wraps it in fsi specialness.

I would note that dotnet.exe can be xcopy deployed so the two well known locations provided in your example

@baronfel
Copy link
Member Author

baronfel commented Oct 8, 2019

I suppose that if you embed FSI you take it on yourself to provide a set of reference/impl assemblies. I was speaking primarily from the point of view of editor tooling, which whould naturally want to remain as close to the dotnet sdk deployment strategies as possible. In this way I would think that FSAC and VS are very similar. So I was hoping we could come to an API that both could use and benefit from, irrespective of any other non-editor-tooling consumers of the compiler as a service.

@KevinRansom
Copy link
Member

@baronfel, I get that getting a list of framework assemblies is desirable, however, that is an API that the SDK needs to provide, I am reluctant to provide one. For sure if they had one, I would use it :-)

For tests we just run msbuild over a project and dump the referenced paths to a file, and use that. I think an App would want a better API than. I don't think providing that API as a durable commitment is one that the F# team should do, it really depends on the host.

@baronfel
Copy link
Member Author

baronfel commented Oct 9, 2019

The thing that strikes me most about this is now the entire GetProjectOptionsForScript call in FCS loses a lot of its utility. Instead of getting a halfway-decent set of script options that might need a little customization, you get a potentially-empty shell. There's a decent amount of conversation going on in the #compiler channel of the FSSF slack between me, Dave Thomas, and some Fable contributors about how to react to the changes in this method, and it's definitely not a small hurdle for us.

@thinkbeforecoding
Copy link
Contributor

I managed to make FSharp.Compiler.Services work with FSharp.Literate inside dotnet fsi for the doc generation of the RegexProvider, but I had to force the loading using a nasty hack.

By forcing some code execution at the begining of the script, the Assembly is correctly resolved. Once loaded the rest of the script is running as expected. But without it, the Assembly resolution fails miserably..

Here is the script with the hack: fsprojects/FSharp.Text.RegexProvider#31

@KevinRansom
Copy link
Member

This should be addressed by: #7725.

@baronfel
Copy link
Member Author

Cool, thanks for taking a crack at it. It seemed fine to me when I took a look after the merge, though the logic changed from taking every dll in the pack for to just the dependencies of netstandard.dll. I'll get master merged through FCS and compare.

Thanks again!

@KevinRansom
Copy link
Member

@baronfel, those dependencies weren't really the problem. The issue, I think is that it couldn't find the reference assemblies in a wide enough range of scenarios. Whilst the new heuristic is quite horrible. It's pretty easy to reason about and is likely to remain quite stable over time.

@cartermp cartermp modified the milestones: Unknown / not bug, 16.5 Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants