search

tintoy / msbuild-project-tools-vscode

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

Return isolated runtime #141

Closed DoctorKrolic closed 6 months ago

DoctorKrolic commented 6 months ago

Client side of #89 Closes #89

@tintoy Do not merge this until you test it in WSL, DevContainers etc. I've tested this by upgrading runtime version locally, which leads to the same error without these changes, but I don't have local setup for quick testing in other environments. And it is better when 2 people with different local environments test these chanes independently anyway

tintoy commented 6 months ago

Sorry, was away for Xmas break but will have a look at this today.

tintoy commented 6 months ago

Hmm. I am about to check in a dev containers repro project that doesn't seem to work with this. It produces the output:

Starting MSBuild language service...
Failed to start the MSBuild language server.
Error: Command failed: "C:\Program Files\dotnet\dotnet.exe" "/home/vscode/.vscode-server/extensions/tintoy.msbuild-project-tools-0.5.4-dev/language-server/MSBuildProjectTools.LanguageServer.Host.dll" --probe
/bin/sh: 1: C:\Program Files\dotnet\dotnet.exe: not found

/bin/sh: 1: C:\Program Files\dotnet\dotnet.exe: not found
tintoy commented 6 months ago

@DoctorKrolic - I have added and configured a dev container (with .NET 8 and a sample project) for you to try out (all you need is Docker or Docker Desktop for Windows / Mac, and the Dev Containers extension for VS Code).

To open the dev container:

  1. Build the extension VSIX (.\Publish-LanguageServer.ps1 and then vsce package).
  2. Copy the VSIX file to test/devcontainers/default/msbuild-project-tools.vsix.
  3. In VS Code, execute the command named Dev Containers: Open Folder in Container..., then navigate to test/devcontainers/default. It will build and launch a container, launch VS Code remotely in the container, and install the extension.

It looks like, for some reason, the extension is detecting the dotnet executable from Windows (on the host?) rather than the one in the container.

tintoy commented 6 months ago

Ah, looks like there were issues with my VS Code settings. Now have a different problem (i.e. isolated runtime didn't respect the global.json file in the container root directory, which specifies 8.0.100) and so got this output:

Starting MSBuild language service...
Failed to start the MSBuild language server.
Error: Command failed: "/home/vscode/.vscode-server/data/User/globalStorage/ms-dotnettools.vscode-dotnet-runtime/.dotnet/6.0.25~x64/dotnet" "/home/vscode/.vscode-server/extensions/tintoy.msbuild-project-tools-0.5.4-dev/language-server/MSBuildProjectTools.LanguageServer.Host.dll" --probe
System.Exception: Cannot locate MSBuild engine for .NET SDK v8.0.100. This probably means that MSBuild Project Tools cannot find the MSBuild for the current project instance. It did find the following version(s), though: [].
   at MSBuildProjectTools.LanguageServer.Utilities.MSBuildHelper.DiscoverMSBuildEngine(String baseDirectory, ILogger logger)
   at MSBuildProjectTools.LanguageServer.Program.Main()

System.Exception: Cannot locate MSBuild engine for .NET SDK v8.0.100. This probably means that MSBuild Project Tools cannot find the MSBuild for the current project instance. It did find the following version(s), though: [].
   at MSBuildProjectTools.LanguageServer.Utilities.MSBuildHelper.DiscoverMSBuildEngine(String baseDirectory, ILogger logger)
   at MSBuildProjectTools.LanguageServer.Program.Main()
tintoy commented 6 months ago

I suspect that if you really want to use an isolated runtime you'll also need to use the vscode-dotnet-sdk extension to install the required SDK (possibly based on global.json so we can load it).

tintoy commented 6 months ago

This might help:

https://github.com/dotnet/vscode-dotnet-runtime/blob/0df1d1c4895768608d22f723f160748918248ad0/sample/src/extension.ts#L168C13-L170C93

(you can pass installType: global to resolve a globally-installed SDK if necessary)

Also worth a read:

DoctorKrolic commented 6 months ago

After some investigation I found 2 problems with current state of the PR:

  1. In a given devcontainer (thanks for detailed instructions on how to run it btw) which dotnet results in /usr/bin/dotnet. However, this is not an actual dotnet executable, but a symlink to it. The real path is /usr/share/dotnet/dotnet. Since inside server we set DOTNET_ROOT to be the directory of DOTNET_HOST_PATH this results in incorrect DOTNET_ROOT being set, which doesn't completely break things, but adds some unwanted side effects. There might be a dilemma of where to fix this issue: on the client side or on the server side, and I think it is a client side problem. DOTNET_HOST_PATH is supposed to be the actual path to the dotnet executable after all, not link to it or something. So fixed it by using realpathSync Node's function
  2. This is weird and I found it just because I didn't know what to try next, but the root cause of the issue is that MSBuildLocator only searches for instances which are hot higher than the current runtime version: https://github.com/microsoft/MSBuildLocator/blob/0bd7d2689d9e0e03a4b0e5bcdccf053f61887f90/src/MSBuildLocator/DotNetSdkLocationHelper.cs#L55-L63 For whatever reason this wasn't a problem when I tested this PR on Windows. Fixed by using .NET 8 runtime. We use isolated runtime, so we are free to do so without any consequences for our users. I'll update server to .NET 8 when this PR is merged. For now, it is ok to run .NET 6 app in .NET 8 runtime

Also I deleted devcontainers test code. First, 2 .sln files in the workspace confuse C# extension and second, I don't want such generic test files to be in the central codebase. I now have a local copy, so I can test extension in devcontainers when I need to, but again, I don't think they belong to this repo

tintoy commented 6 months ago

Regarding deletion of test code, I think it is important to be able to run basic smoke tests as part of the CI pipeline (to catch regressions etc) so we may need to revisit that at some point (but not as part of this work).

DoctorKrolic commented 6 months ago

So does my latest version work for you @tintoy ? Did you test other scenarios (WSL and so on)?

tintoy commented 6 months ago

Sorry, only finished work an hour ago - will have a look after dinner 🙂

tintoy commented 6 months ago

Ok, I have tried it in WSL and DevContainer - both seem to work correctly (I did notice that package info from project.assets.json isn't coming through correctly but I don't think it is on Windows either; will have a look at that tomorrow).

DoctorKrolic commented 6 months ago

We really should set up a CI pipeline, at some point, that runs some basic integration tests

Definitely agree, but I am struggling to imagine what can we test other than server startup? We fully rely on MSBuild APIs when it comes to fetching underlying data for completions and other features, so we cannot reliably tell what items are expected at what positions and so on

tintoy commented 6 months ago

we cannot reliably tell what items are expected at what positions and so on

We already have tests along those lines in the language server repository; I imagine that we could create similar tests but driven from VSCode.