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

Deploy to RazorDev instead of RoslynDev #8349

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Feb 28, 2023

  • Deploy to RazorDev experimental instance instead of RoslynDev to avoid conflicts with Roslyn
  • Let the integration test harness deploy the dependencies VSIX on demand, instead of relying on the VS SDK to do it as part of the build
  • Always include Roslyn dependencies for integration test deployment

Comment on lines -15 to -16
OldVersionUpperBound = "4.6.0.0",
NewVersion = "4.6.0.0")]
Copy link
Member Author

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")]
Copy link
Member Author

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>
Copy link
Member Author

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">
Copy link
Member Author

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.

Comment on lines +50 to +68
<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>
Copy link
Member Author

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.

Comment on lines +45 to +47
<AssemblyAttribute Include="Xunit.Harness.RequireExtensionAttribute">
<_Parameter1>Microsoft.VisualStudio.RazorExtension.Dependencies.vsix</_Parameter1>
</AssemblyAttribute>
Copy link
Member Author

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'">
Copy link
Member Author

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?

Copy link
Member

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

@davidwengier
Copy link
Contributor

Deploy to RazorDev experimental instance instead of RoslynDev to avoid conflicts with Roslyn

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?

@sharwell
Copy link
Member Author

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.

@davidwengier
Copy link
Contributor

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.

@davidwengier
Copy link
Contributor

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.

@sharwell
Copy link
Member Author

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).

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.

@davidwengier
Copy link
Contributor

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.

@sharwell
Copy link
Member Author

I'll keep that in mind should I ever be interested in moving this out of the draft state.

@sharwell
Copy link
Member Author

sharwell commented Mar 3, 2023

/azp run razor-tooling-ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

3 participants