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

Include Roslyn devkit dependencies in C# extension to avoid versioning issues with devkit #6681

Merged
merged 3 commits into from
Nov 27, 2023

Conversation

dibarbet
Copy link
Member

@dibarbet dibarbet commented Nov 17, 2023

Requires server side change - dotnet/roslyn#70875

These optional devkit dependencies are currently shipped inside the devkit extension, but loaded into the language server process.
That makes it hard to use internal Roslyn APIs in these dependencies because:

  1. The versions of the C# extension and devkit extension are not guaranteed to line up. Its possible to have almost any version of C# with almost any version of devkit (without adding additional runtime checks).
  2. Even if we added runtime checks, using internal Roslyn APIs would likely mean incrementing the required version each extension update.

This PR updates the C# extension to instead ship its own devkit dependencies. They are not loaded when using C# standalone. This allows us to use internal APIs in the devkit deps without requiring a devkit version update each time they change.

We considered a runtime download of the devkit dependencies (similar to how O# is acquired), but the size of the dependencies (small, around 2 MB) and the complexity of publishing to a CDN led us to this approach.

Resolves dotnet/roslyn#69467

TODO - Draft until server side change goes in.

@davidwengier
Copy link
Contributor

FYI @ryzngard
If the C# extension can get away with these not being a runtime download, then maybe we can follow suit and simplify things on the Razor side.

@dibarbet dibarbet marked this pull request as ready for review November 21, 2023 22:26
@dibarbet dibarbet requested a review from a team as a code owner November 21, 2023 22:26
args.push(extensionPath);
}
const clientRoot = __dirname;
const devkitDepsPath = path.join(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same way we get the language server path

.gitignore Outdated
@@ -3,6 +3,7 @@ obj
node_modules
out
.roslyn/
.roslynDevkit/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should DevKit generally be a capitalized K if it's two words? Question applies to the PR in general. I mostly worry if a case sensitivity issue breaks us somewhere... 😄

async function acquireRoslynDevkit(packageJSON: any, interactive: boolean): Promise<string> {
const roslynVersion = packageJSON.defaults.roslyn;
const packagePath = await acquireNugetPackage(
`Microsoft.VisualStudio.LanguageServices.Devkit`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Binary name is DevKit but the package name is Devkit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dibarbet dibarbet merged commit b4b3752 into dotnet:main Nov 27, 2023
13 checks passed
@dibarbet dibarbet deleted the ship_devkit_deps branch November 27, 2023 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ship Roslyn devkit assembly as a downloadable component
3 participants