tintoy / msbuild-project-tools-vscode

VS Code extension for MSBuild intellisense (including PackageReference completion).
MIT License
82 stars 16 forks source link

Consider using vscode-dotnet-runtime #89

Closed tintoy closed 6 months ago

tintoy commented 2 years ago

The vscode-dotnet-runtime extension for VS Code provides support to third-party extensions for downloading and installing various version of the .NET runtime (e.g. to run language servers).

This would enable MSBuild Project tools to transparently change its runtime version dependency without making end-users manually locate, download, and install the required version of the .NET runtime.

DoctorKrolic commented 7 months ago

Reopening since this work was reverted as a hotfix. We should investigate whether it is possible to isolate extension's runtime while also keeping the ability to reliably check information about .NET on the host and either bring this change back or provide a proof that this is not possible

tintoy commented 7 months ago

Check out the 2 links I posted in that other issue - it’s possible to configure the vscode-dotnet extension to use the global .net instance for a given extension:

https://github.com/tintoy/msbuild-project-tools-vscode/issues/137#issuecomment-1824285615

DoctorKrolic commented 7 months ago

TBH this is the opposite of what I want to achieve with this. My idea originally was to completely isolate .NET runtime, which runs language server, so for the end user there is no impact on his global .NET environment. + it seems that the thing you've suggested works on "user level", meaning that as a user I can configure vscode dotnet runtime extension to provide existing .NET installation for a specific extension id I choose, but it isn't clear whether it is possible to do this through code inside another extension. I never expected that implementing this will break detection of host .NET though

tintoy commented 7 months ago

Oh, I see. Yeah, in that case I am not entirely sure whether this idea is practical; the official .NET SDK discovery logic (and most of its underlying machinery and tooling) uses built-in knowledge from the current runtime to discover SDKs (especially each SDK’s associated MSBuild instance).

I suspect we would be swimming against the tide to try using our own isolated runtime but their global SDK (which we would need to do in order to load their project with correct behaviour and metadata).

I can see why you’d want to do that for a “normal”extension but for one that has to utilise the correct SDK tooling and runtime metadata to mimic what they would see if running stuff from the command line it would be very hard to be sure you’d got it right (and wouldn’t break if they changed undocumented behaviour).

tintoy commented 7 months ago

(most of the complexity comes from having to load the correct version of the MSBuild engine and any custom SDKs or tasks they have configured)

DoctorKrolic commented 6 months ago

Ok, I've done a research which seems to have positive results. My idea is to use 2 well-known environment variables: DOTNET_HOST_PATH and DOTNET_ROOT. The good thing is that these variables are also respected by other components of .NET, most importantly for us is that by setting them we can override default location where MSBuildLocator searches for MSBuild. This is the opposite of what I've suggested doing in https://github.com/tintoy/msbuild-project-tools-server/issues/72, but in this case I think it is a good solution, since all shenanigans with .NET's path are our internal implementation detail and are not in user-controlled settings space. Moreover, this will give potential language server users (https://github.com/tintoy/msbuild-project-tools-server/issues/33) more flexibility since they will be able to choose whether they want to use user's .NET or an isolated runtime with DOTNET_HOST_PATH environment variable pointing to user's distribution of .NET. The server side PR is https://github.com/tintoy/msbuild-project-tools-server/pull/86. Client-side PR is expected to come next

tintoy commented 6 months ago

Nice idea! Thanks, will have a look at this tomorrow morning 🙂

tintoy commented 6 months ago

I think if we are going to go down this route then we will, at minimum, need to have a working CI pipeline which runs integration-type tests with various SDK / runtime combinations (in containers, for example).

Maybe using TestContainers?

tintoy commented 6 months ago

I'm happy to look into the testing side if that would be helpful.

tintoy commented 6 months ago

BTW, I can't remember if I showed you before, but our CI runs here:

https://dev.azure.com/tintoy-dev/msbuild-project-tools-vscode/_build?definitionId=2

tintoy commented 6 months ago

Ok CI is now running for both language server and extension package:

https://dev.azure.com/tintoy-dev/msbuild-project-tools-vscode/_build/results?buildId=136&view=artifacts&pathAsName=false&type=publishedArtifacts

Later, I'd like to improve extension-package CI to get the language-server artifact from the appropriate GitHub release but this will do for now.