tintoy / msbuild-project-tools-server

Language server for MSBuild intellisense (including PackageReference completion).
MIT License
59 stars 16 forks source link

MSBuildLocator can query .NET SDK preview versions. #105

Closed nemonuri closed 4 months ago

nemonuri commented 5 months ago

https://github.com/microsoft/MSBuildLocator/issues/271#issuecomment-1933639155

The solution was simple...;;

tintoy commented 4 months ago

Thanks for contributing! Sorry, been super busy with work but am hoping to look at this properly tomorrow šŸ™‚

tintoy commented 4 months ago

Ok, I finally got some time to work on this; I am thinking we could put it behind a feature flag so users can opt-in to the new behaviour šŸ™‚

nemonuri commented 4 months ago

Good! šŸ˜„

tintoy commented 4 months ago

Ok - because of where MSBuild discovery currently takes place, I've had to use an environment variable (MSBUILD_PROJECT_TOOLS_ENABLE_PRERELEASE_SDKS) instead.

tintoy commented 4 months ago

As soon as you open an msbuild-related file, you'll get an error that some v9 assembly cannot be loaded into the process.

Yeah, I agree but people keep asking for this so if we can enable it with the disclaimer that it probably wonā€™t work with most preview SDK versions, then our users can try it out for themselves and see thatā€™s the case?

nemonuri commented 4 months ago

How about this strategy? (See my new commit message)

  1. Build two extension packages

    1. stable version (using stable .NET sdk)
    2. pre-release version (using preview .NET sdk)
  2. When publishing extension packages to VSCode marketplace,

    1. stable version ā†’ stable semver
    2. pre-release version ā†’ pre-release semver

Then, write this text in Readme.md

- Release version: Stable, but does not support latest preview .NET sdks.
- Pre-release vresion: Supports latest preview .NET sdks, but unstable

What do you think about this strategy?

tintoy commented 4 months ago

The main problem is that we canā€™t guarantee it does support preview SDKs - just that it might. But a preview extension may be a useful idea.

tintoy commented 4 months ago

The issue is that we canā€™t really be sure that the language server can support preview SDKs without building the language server against those runtime / SDK / MSBuild versions; there has been a comparatively poor track-record of backward-compatibility at this level of the stack (which is understandable because itā€™s not a particularly common use-case to need to integrate at this level of the stack).

tintoy commented 4 months ago

As for maintaining 2 versions of the extension and language server (one built against the stable SDK and one built against the latest preview SDK), Iā€™m not sure that myself or @DoctorKrolic have time for that at the moment.

But, as long as users realise that the support for preview SDKs is only ā€œcould theoretically supportā€ not ā€œdoes supportā€ then I think we could probably manage to do it šŸ™‚

CC: @baronfel

nemonuri commented 4 months ago

The issue is that we canā€™t really be sure that the language server can support preview SDKs without building the language server against those runtime / SDK / MSBuild versions

I have realized what is main problem.

How about this additional solution? We can write 'Do Build Yourself' manual document (or link) in Readme.md.

Overview of the manual might be like:

You installed pre-release version, but still not work?
Then, build language server yourself!

1. How to download source code
2. How to set build configuration
3. How to build and copy files to 'langauge-server' directory
(...and so on)
tintoy commented 4 months ago

Yes, this might be a better fit for that; if they get it working they can always open a PR later too šŸ™‚

DoctorKrolic commented 4 months ago

I think having 2 versions of the extension and/or the way for users to directly build a preview version for regular use is a massive overkill. I think the quick fix would be to allow language server to start on the most latest version of .NET user have installed. This should be a opt-in setting since we cannot guarantee that everything would work in such case and we don't expect that many people to be on the preview versions anyway. .NET is good at managing binary back compat, so this should work in 90+% of legit use cases. The proper long-term solution would be to extract all MSBuild-related logic into a subprocess and start that process on exactly the same version as demanded by the workspace. I've also thought about that in the past, but currently all MSBuild stuff is backed very deeply into the codebase, so to start the implementation we first need to decouple MSBuild from everything else.

I'll open a PR with my proposed short-term solution later this day.

tintoy commented 4 months ago

Yeah good idea - I was thinking of a separate process too; maybe a simple gRPC API for inter-process communication?

tintoy commented 4 months ago

@DoctorKrolic - maybe something like this:

https://github.com/tintoy/msbuild-project-tools-server/pull/105/commits/fb1c5d5233dc142a5e1bbc0652b27e181c5e478f

(MSBuild discovery happens before LSP session is initialised so canā€™t get access to extension config)

baronfel commented 4 months ago

Did you all try something like we do in Ionide? The VSCode part of our extension does some very simple runtime version detection for the open workspace, and if the detected runtime is greater than the version that Ionide was targeted for we use the .NET Environment variables for rollforward to run our older-targeted app on the newer runtime. This broadly works for us, and should work for you all too - I'd go as far as to say that breakages here should be considered as compat bugs for the MSBuild team to triage.

DoctorKrolic commented 4 months ago

@baronfel Your suggestion is very similar to mine except you don't hold the roll forward part under the settings flag.

I'd go as far as to say that breakages here should be considered as compat bugs for the MSBuild team to triage.

So you don't think that having a setting for that is worth it, right? My conservatizm here with such behavior being opt-in is unnecessary?

My implementation is in extension repo: https://github.com/tintoy/msbuild-project-tools-vscode/pull/152

baronfel commented 4 months ago

Oh great! I'd say that in this case since there's basically zero chance the system will work without rolling forward the conservatism is unnecessary. The user is already in control of what SDK they use via global.json, so if you let them rely on that you sidestep any issues where this tooling and the CLI tooling behave differently.