-
Couldn't load subscription status.
- Fork 214
Enable R2R for servicehub components: #12385
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
base: main
Are you sure you want to change the base?
Conversation
chsienki
commented
Oct 23, 2025
- Split out service hub components into x64 and arm64 projects
- Extract out some common VSIX strings
- Only deploy service hub components on official builds
- Setup extension VSIX so that it deploys service hub components for developer builds
- Split out service hub components into x64 and arm64 projects - Extract out some common VSIX strings - Only deploy service hub components on official builds - Setup extension VSIX so that it deploys service hub components for developer builds
| </Metadata> | ||
| <Installation AllUsers="true" Experimental="true"> | ||
| <InstallationTarget Id="Microsoft.VisualStudio.Community" Version="[17.0,)"> | ||
| <ProductArchitecture>amd64</ProductArchitecture> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This says "amd", is that right? If so, consider adding a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, nope thats a typo. Would be helpful if I had an arm machine to work on (though the VS build succeeded so it might not actually matter? shrug)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm going to open an issue to track looking at how much of the vsixmanifest is actually necessary, as this is really an insertion VSIX, not an actual deployable one.
I also think we should rename these project to something like MS.VS.RazorExtension.ServiceHub to make it clearer what they actually are. But I think I'll do that in a follow up.
| @@ -199,6 +186,20 @@ | |||
| </ProjectReference> | |||
| </ItemGroup> | |||
|
|
|||
| <!-- We reference the core components only for developer builds. In the official build they are inserted via the corecomponents VSIX alongside this VSIX --> | |||
| <ItemGroup> | |||
| <ProjectReference Include="..\Microsoft.CodeAnalysis.Remote.Razor\Microsoft.CodeAnalysis.Remote.Razor.csproj" Condition="'$(OfficialBuildId)' == ''"> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking out loud: How does OfficialBuildId compare to CI as a check? I wonder if this will adversely affect things like toolset or integration test pipelines, that aren't official but also aren't developer builds? Or maybe they are??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Toolset shouldn't matter as it doesn't use VS and this change only affects that. Integration presumably does deploy it to VS, though I imagine its more an F5 deploy than an actual VS insertion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I would say its definitely akin to a developer build, so this is probably the right choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This question isn't exactly related to this change, but I want to ask since there's so much nice work happening in this pull request.
Does this still need to target netstandard2.0? Do we actually deploy netstandard2.0 for this layer to any product?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's a great question. I took a look, and I don't think it does. I'm going to open an issue for that and do it in another PR though, because I don't want to break anything 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #12391 to follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bet we still need it for tooling tests, until we've untangled that mess too.