Conditionally deploy Roslyn dependencies #8086
Conversation
|
Does this mean that ordinary people with non-internal preview builds would not be able to debug razor unless they add this switch manually as well? |
The state would essentially be the same as it was before October 25th, when the Roslyn dependencies were added specifically to fix integration tests. Sometimes there can be a situation where Razor depends on a very specific version of Roslyn that hasn't shipped in VS yet. The hope is that this would generally be very rare, but the current churn in the compiler might make it more common in the short term. |
DustinCampbell
force-pushed
the
conditionally-deploy-roslyn-deps
branch
from
e9c530c to
9ed8950
Compare
|
You know what would be cool? An entry in |
I don't see how that would work, since this needs to be set at build-time, not debug-time. We could add a configuration to the solution, but that feels pretty heavyweight. |
|
Oops, you're right. One day I'll learn not to post comments from my phone on Saturday mornings :) |
Do we know whether this is a benefit or detriment to community contributors? The change to add extra dependencies to fix integration tests was submitted just two and a half months ago. Was debugging again preview builds broken at that point? @DoctorKrolic, I'm guessing that you could speak best to your experience. There's also a potential philosophical debate here. Should the repo be structured to make be most convenient for active daily developers or for community contributors? |
|
FWIW, I just tried the following experiments:
So, with the current releases, this change works fine for external contributors. If a later preview exhibits the problems we're experiencing with IntPreview, then it's important that this change makes it in to keep things working for them. I haven't seen any evidence that the changes to add Roslyn deps were ever necessary for local machine debugging. They were only added to address integration tests in CI. To the best of my knowledge, local machine debugging still worked fine even when integration tests were on the floor back in October. |
|
@DustinCampbell Thanks for verifying this PR against latest public preview build!
Well, I haven't contributed to the razor in a while, but from my previous experience after some dependency update or smth like that razor VS-based debugging can unexpectedly stop working for public builds (e.g. #5891, #6094). So if it isn't costly to verify that, it is probably a good thing to do for changes like this one. Again, thanks for doing it here! |
|
Thanks @DoctorKrolic -- much appreciated! It sounds like having an easy way to set this would be helpful, even if it's just using an environment variable to control the build. I think it's probably a rare situation when we actually need to replace Roslyn in an official preview release in order to debug or run integration tests locally against |
| @@ -3,6 +3,11 @@ | |||
|
|
|||
| using Microsoft.VisualStudio.Shell; | |||
|
|
|||
| [assembly: ProvideCodeBase(CodeBase = @"$PackageFolder$\Microsoft.VisualStudio.LanguageServer.Protocol.dll")] | |||
There was a problem hiding this comment.
Why spare these from conditionality?
There was a problem hiding this comment.
Because they existed before the change to fix VS integration tests. To my knowledge, there haven't been any failures due to these. If we find that these are problematic, we can address them separately. However, since they don't actually contain behavior, I suspect that they'll be OK.
|
I went ahead and made an update to the "StartVS" script to make it easy to control whether Roslyn dependencies are deployed on a local box. I also added the ability to choose an installation of VS to launch, which can be helpful. (I used it to launch both 17.5 P2 and the IntPreview.) Please go ahead and destroy my PowerShell code. 😄 |
Razor tooling deploys Roslyn dependencies to allow integration tests and VS hive debugging to function against VS preview builds. However, on local dev boxes, where VS IntPreview should be used for dogfooding, this can result in hard-to-diagnose problems when some Roslyn dependencies are loaded from the Razor tooling deployment and others are loaded from the VS install. This change only deploys Roslyn dependencies if the `IncludeRoslynDeps` MSBuild property is set to true. This is needed for running integration tests in CI, so the job has been updated to set that property.
DustinCampbell
force-pushed
the
conditionally-deploy-roslyn-deps
branch
from
2a23a24 to
ec91360
Compare
This change updates the StartVS script in the following ways: * Ported batch file to a PowerShell script. The batch file is still there. It just runs the PowerShell script. * Added a `-chooseVS` switch that displays a menu of Visual Studio instances that are installed and allows the user to pick one to launch. * Added an `-includeRoslynDeps` switch that sets an environment variable that controls whether Roslyn dependencies are included with the Razor extension or not.
DustinCampbell
force-pushed
the
conditionally-deploy-roslyn-deps
branch
from
ec91360 to
224c88c
Compare
Razor tooling deploys Roslyn dependencies to allow integration tests and VS hive debugging to function against VS preview builds. However, on local dev boxes, where VS IntPreview should be used for dogfooding, this can result in hard-to-diagnose problems when some Roslyn dependencies are loaded from the Razor tooling deployment and others are loaded from the VS install.
This change only deploys Roslyn dependencies if the
IncludeRoslynDepsMSBuild property is set to true. This is needed for running integration tests in CI, so the job has been updated to set that property.