-
Notifications
You must be signed in to change notification settings - Fork 198
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
Deploy to RazorDev instead of RoslynDev #8349
base: main
Are you sure you want to change the base?
Conversation
OldVersionUpperBound = "4.6.0.0", | ||
NewVersion = "4.6.0.0")] |
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.
📝 These were unnecessarily complex. The tooling knows how to derive the values from the referenced assemblies at build time.
OldVersionUpperBound = "4.6.0.0", | ||
NewVersion = "4.6.0.0")] | ||
|
||
[assembly: ProvideCodeBase(CodeBase = @"$PackageFolder$\Microsoft.CodeAnalysis.Workspaces.dll")] |
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 appears to be a duplicate of line 90 above.
@@ -19,13 +19,14 @@ | |||
<!-- Don't automatically include dependencies --> | |||
<IncludePackageReferencesInVSIXContainer>false</IncludePackageReferencesInVSIXContainer> | |||
|
|||
<CreateVsixContainer Condition="'$(BuildDependencyVsix)' == 'true'">true</CreateVsixContainer> | |||
<CreateVsixContainer>true</CreateVsixContainer> |
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.
📝 There is no more need for this to be conditional. Since the integration test harness knows how to deploy the extension on demand, we can control the deployment exclusively from the integration test project that references this.
@@ -8,7 +8,7 @@ | |||
<!-- This is needed to mark this extension as cloud compliant. --> | |||
<AllowClientRole>true</AllowClientRole> | |||
</Metadata> | |||
<Installation AllUsers="true" Experimental="true"> |
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.
📝 We aren't overriding a built-in extension, so this isn't considered an experimental version of an all-users extension.
<Target Name="PrepareVsixProjectReferences" | ||
BeforeTargets="ResolveProjectReferences" | ||
DependsOnTargets="PrepareProjectReferences"> | ||
<MSBuild | ||
Projects="@(_MSBuildProjectReferenceExistent)" | ||
Targets="VSIXContainerProjectOutputGroup" | ||
BuildInParallel="$(BuildInParallel)" | ||
Properties="%(_MSBuildProjectReferenceExistent.SetConfiguration); %(_MSBuildProjectReferenceExistent.SetPlatform); %(_MSBuildProjectReferenceExistent.SetTargetFramework); CreateVsixContainer=true" | ||
Condition="'%(_MSBuildProjectReferenceExistent.CopyVsix)' == 'true'" | ||
ContinueOnError="!$(BuildingProject)" | ||
RemoveProperties="%(_MSBuildProjectReferenceExistent.GlobalPropertiesToRemove)"> | ||
|
||
<Output TaskParameter="TargetOutputs" ItemName="_ProjectReferenceVsixOutputs" /> | ||
</MSBuild> | ||
|
||
<ItemGroup> | ||
<ReferenceCopyLocalPaths Include="@(_ProjectReferenceVsixOutputs)" /> | ||
</ItemGroup> | ||
</Target> |
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 need to verify this is exactly what we want as a general solution, and get this moved over to microsoft/vs-extension-testing.
<AssemblyAttribute Include="Xunit.Harness.RequireExtensionAttribute"> | ||
<_Parameter1>Microsoft.VisualStudio.RazorExtension.Dependencies.vsix</_Parameter1> | ||
</AssemblyAttribute> |
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 is the part to make conditional if there are cases where we don't want to deploy the dependencies.
@@ -68,7 +69,7 @@ | |||
<PackageReference Include="Microsoft.VisualStudio.LanguageServices.Implementation.Symbols" Version="$(Tooling_MicrosoftVisualStudioLanguageServicesPackageVersion)" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup Condition="'$(IncludeRoslynDeps)' == 'true'"> |
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.
📝 It wasn't clear why we need this flag. I've removed it for now, but maybe we need it for something?
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.
FWIW, it was added in #8086
What conflicts? I don't run into conflicts, and there are active benefits of the sharing of hives that I take advantage of regularly for local development. Surely the CI machines start clean and don't care what the hive name is? |
I was running into strange integration test behaviors due to other Roslyn contents existing in the experimental instance. I'm working on updating this to allow F5 to RoslynDev and run the integration tests by default in RazorDev. |
That sounds like a recipe for a different type of disaster, where a run of an integration test fails locally, so I press F5 and am now running different code (or at best, have to build and deploy again). I would rather not make a change to fix an issue unless it is something that regular razor contributors run into. It sounds like you probably could have cleared your RoslynDev hive and be unblocked. |
Perhaps a separate PR would make sense, and provide a better space for discussion, distinct from the effort to improve integration tests in CI, which I assume the rest of the PR is about. |
The whole thing is automatic. Instead of F5 being a keystroke, it's a separate command that shows up in Test Explorer and you can select the scenario you want to launch or debug. The necessary components are detected according to the scenario and deployed automatically if necessary as part of launch. |
Sorry, but when I want to debug my code, I press F5. If any proposed solution doesn't work with that, then its a non-starter, especially given I don't have the problem that it's solving. |
I'll keep that in mind should I ever be interested in moving this out of the draft state. |
/azp run razor-tooling-ci |
Azure Pipelines successfully started running 1 pipeline(s). |
RazorDev
experimental instance instead ofRoslynDev
to avoid conflicts with Roslyn