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

Consume newer Roslyn APIs for language services #9455

Merged
merged 22 commits into from
May 2, 2024

Conversation

drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented Apr 29, 2024

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1881089

Roslyn recently introduced async API for obtaining and releasing "batch" objects for applying updates in dotnet/roslyn#72424.

This change increases the package versions, uses the newer API, fixes some obsolete usages, and gets things building by adding a few package references in order to break the tie on some assembly version conflicts during build.

The package Microsoft.VisualStudio.Internal.MicroBuild.Swix was removed, as it conflicted with behaviour inherited via microsoft.visualstudioeng.microbuild.plugins.swixbuild, which now comes in transitively, resulting in errors running swc. Having both loaded was doubling up the items used to populate arguments, and it was throwing An item with the same key has already been added.

Microsoft Reviewers: Open in CodeFlow

Roslyn recently introduced async API for obtaining and releasing "batch" objects for applying updates.

This increases the package versions, uses the newer API, fixes some obsolete usages, and gets things building by adding a few package references in order to break the tie on some assembly version conflicts during build.

The package `Microsoft.VisualStudio.Internal.MicroBuild.Swix` was removed, as it conflicted with behaviour inherited via `microsoft.visualstudioeng.microbuild.plugins.swixbuild`, which now comes in transitively, resulting in errors running `swc`. Having both loaded was doubling up the items used to populate arguments, and it was throwing _An item with the same key has already been added._
@drewnoakes drewnoakes added Feature-Language-Service Populating the Roslyn workspace with references, source files, analyzers, etc Tenet-Performance This issue affects the "Performance" tenet. Performance-Scenario-Solution-Open This issue affects solution open performance. Performance-Scenario-General This issue affects performance in general. labels Apr 29, 2024
@drewnoakes drewnoakes requested a review from ToddGrun April 29, 2024 15:27
@drewnoakes drewnoakes requested a review from a team as a code owner April 29, 2024 15:27
@drewnoakes
Copy link
Member Author

Looks like I'll need to find another workaround for the conflict between:

  1. microsoft.visualstudioeng.microbuild.plugins.swixbuild (1.1.286)
  2. microsoft.visualstudio.internal.microbuild.swix (2.0.115)

@drewnoakes
Copy link
Member Author

drewnoakes commented Apr 30, 2024

Adding notes here as I investigate the SWIX conflicts that fall out of these package upgrades:

The error I see is:

  Unhandled Exception: System.ArgumentException: An item with the same key has already been added.
     at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource)
     at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
     at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
     at WixToolset.Simplified.SimplifiedWixCompiler.Run(Messaging messaging)
     at WixToolset.Simplified.SimplifiedWixCompiler.Main(String[] args)
D:\NuGetPackages\microsoft.devdiv.tasks2\17.4.0-preview-2-32803-137\build\swix.targets(154,5): error MSB6006: "swc.exe" exited with code -532462766.

I believe that exception is coming from here where "preprocessor defines" are being added.

Looking at the binlog shows the task does indeed have duplicates:

image

Task logs its CommandLineArguments as:

CompileVsix:
  D:\NuGetPackages\microsoft.visualstudio.internal.microbuild.setuptooling\2.0.115\build\..\tools\bin\swc.exe
   -arch neutral
   -ext D:\NuGetPackages\microsoft.visualstudio.internal.microbuild.setuptooling\2.0.115\build\..\ext\DevDivSwixExtension.dll
   -ext D:\NuGetPackages\microsoft.visualstudio.internal.microbuild.setuptooling\2.0.115\build\..\tools\VsixSwixExtension.dll
   -o d:\repos\project-system\artifacts\Debug\VSSetup\Insertion\Microsoft.VisualStudio.ProjectSystem.Managed.CommonFiles.vsix
   -d VisualStudioXamlRulesDir=d:\repos\project-system\artifacts\Debug\VSSetup\Rules\
   -d VisualStudioExtensionSetupDir=d:\repos\project-system\setup\
   -d Chip=AnyCPU
   -d MicrosoftPackageNamePrefix=Microsoft.
   -d "MicrosoftPublisher=CN=Microsoft Corporation, O=Microsoft Corporation, L=Redmond, S=Washington, C=US"
   -d "VSDefaultProgramPath=Software\Microsoft\VisualStudio_[InstanceId]\Capabilities"
   -d VsDdeApplication=VisualStudio.15.0
   -d Lang=enu
   -d Codepage=1252
   -d Culture=en
   -d LangCountry=en-US
   -d LCID=1033
   -d Chip=AnyCPU
   -d MicrosoftPackageNamePrefix=Microsoft.
   -d "MicrosoftPublisher=CN=Microsoft Corporation, O=Microsoft Corporation, L=Redmond, S=Washington, C=US"
   -d "VSDefaultProgramPath=Software\Microsoft\VisualStudio_[InstanceId]\Capabilities"
   -d VsDdeApplication=VisualStudio.15.0
   -d Lang=enu
   -d Codepage=1252
   -d Culture=en
   -d LangCountry=en-US
   -d LCID=1033
   d:\repos\project-system\artifacts\Debug\VSSetup.obj\Microsoft.VisualStudio.ProjectSystem.Managed.CommonFiles\swr\core.neutral.swr d:\repos\project-system\setup\Microsoft.VisualStudio.ProjectSystem.Managed.CommonFiles\CommonFiles.swr d:\repos\project-system\setup\Microsoft.VisualStudio.ProjectSystem.Managed.CommonFiles\ext.xproj.swr D:\NuGetPackages\microsoft.visualstudio.internal.microbuild.setuptooling\2.0.115\build\..\tools\VsixExternalFolders.swr
   -t vsix

Where -d is a define, and we see the duplicates.

These defines are coming from PackagePreprocessorDefinitions items in the build. Digging through the binlogs shows that two build files are reassigning the property:

  1. D:\NuGetPackages\microsoft.visualstudio.internal.microbuild.swix\2.0.115\build\Microsoft.Wix4.Swix.Tools.cleaned.targets
  2. D:\NuGetPackages\microsoft.visualstudioeng.microbuild.plugins.swixbuild\1.1.286\build\Microsoft.Wix4.Swix.Tools.targets

They have near-identical code. Indeed one appears to have been copied from the other. My assumption is that they're not intended to be used together.

I had previously tried removing microsoft.visualstudio.internal.microbuild.swix, which worked locally but did not work in CI, giving error:

error MSB4057: The target "CreateManifestResourceNames" does not exist in the project. [D:\a_work\1\s\setup\Microsoft.VisualStudio.ProjectSystem.Managed.CommonFiles\Microsoft.VisualStudio.ProjectSystem.Managed.CommonFiles.swixproj]

So now my plan is to find out why we're bringing in the other (microsoft.visualstudioeng.microbuild.plugins.swixbuild) and see if we can prevent that.

@drewnoakes
Copy link
Member Author

microsoft.visualstudioeng.microbuild.plugins.swixbuild is imported from a NuGet package:

image

There's no mention of that package in the dependencies tree though. Looking in the assets file (D:\repos\project-system\artifacts\Debug\obj\Microsoft.VisualStudio.ProjectSystem.Managed.CommonFiles\project.assets.json) also shows no direct mention of microsoft.visualstudioeng.microbuild.plugins.swixbuild, so I'm unclear on how this is being added and still ended up in the .g.props file.

Pushed another experimental commit.

@drewnoakes
Copy link
Member Author

That's making progress.

Now we have test failures. I assume this is due to updates to Microsoft.VisualStudio.Threading.

Microsoft.VisualStudio.ProjectSystem.Properties.AssemblyInfoPropertiesProviderTests.SourceFileProperties_SetPropertyValueAsync line 184

Microsoft.VisualStudio.Threading.JoinableTaskContextException : An attempt to switch to the main thread failed to reach the expected thread. Was the JoinableTaskContext initialized on the wrong thread or with a SynchronizationContext whose Post method does not execute its delegate on the main thread?

Stack Trace: 

    MainThreadAwaiter.GetResult()
    SourceAssemblyAttributePropertyValueProvider.SetPropertyValueAsync(String value) line 102
    AssemblyInfoProperties.SetPropertyValueAsync(String propertyName, String unevaluatedPropertyValue, IReadOnlyDictionary`2 dimensionalConditions) line 102
    InterceptedProjectProperties.SetPropertyValueAsync(String propertyName, String unevaluatedPropertyValue, IReadOnlyDictionary`2 dimensionalConditions) line 80
    AssemblyInfoPropertiesProviderTests.SourceFileProperties_SetPropertyValueAsync(String code, String propertyName, String propertyValue, String expectedCode) line 191

There are test failures with RenamerTests too, along the same lines.

@drewnoakes
Copy link
Member Author

That exception is thrown here:

https://github.com/microsoft/vs-threading/blob/e4417b768162ac74b43e251320d11de771e7653b/src/Microsoft.VisualStudio.Threading/JoinableTaskFactory.cs#L854

I was able to fix these issues by suppressing the synchronization context for the tests in question.

We were seeing a new error in the tests modified here:

> An attempt to switch to the main thread failed to reach the expected thread.

Looking at the JTC source in https://github.com/microsoft/vs-threading/blob/e4417b768162ac74b43e251320d11de771e7653b/src/Microsoft.VisualStudio.Threading/JoinableTaskFactory.cs#L854 shows that if the synchronisation context is null, then validation is skipped.

This change adds a new utility method that allows the current `SynchronizationContext` to be `null` for a given scope. We add it around the two tests that were failing with this exception.
@drewnoakes
Copy link
Member Author

@tmeschter @ToddGrun this is ready for review now.

@ToddGrun
Copy link
Contributor

ToddGrun commented May 1, 2024

The Roslyn interaction part LGTM

@drewnoakes drewnoakes merged commit 6d6bb5b into dotnet:main May 2, 2024
5 checks passed
@drewnoakes drewnoakes deleted the bump-roslyn branch May 2, 2024 09:45
@dotnet-policy-service dotnet-policy-service bot added this to the 17.10 milestone May 2, 2024
@drewnoakes drewnoakes modified the milestones: 17.10, 17.11 May 2, 2024
@drewnoakes
Copy link
Member Author

We are reverting this because it's broken build signing and is blocking other work. Will revisit.

@drewnoakes drewnoakes restored the bump-roslyn branch May 20, 2024 05:18
drewnoakes added a commit to drewnoakes/project-system that referenced this pull request May 20, 2024
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1881089

Roslyn recently introduced async API for obtaining and releasing "batch" objects for applying updates in dotnet/roslyn#72424.

This change increases the package versions, uses the newer API, fixes some obsolete usages, and gets things building by adding a few package references in order to break the tie on some assembly version conflicts during build.

This is another iteration of dotnet#9455, which was reverted due to issues it created during signing. Unlike that prior PR, this does not remove Microsoft.VisualStudio.Internal.MicroBuild.Swix`, so I expct the same issues about duplicated items to appear.
drewnoakes added a commit to drewnoakes/project-system that referenced this pull request May 20, 2024
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1881089

Roslyn recently introduced async API for obtaining and releasing "batch" objects for applying updates in dotnet/roslyn#72424.

This change increases the package versions, uses the newer API, fixes some obsolete usages, and gets things building by adding a few package references in order to break the tie on some assembly version conflicts during build.

This is another iteration of dotnet#9455, which was reverted due to issues it created during signing. Unlike that prior PR, this does not remove Microsoft.VisualStudio.Internal.MicroBuild.Swix`, so I expct the same issues about duplicated items to appear.
@drewnoakes drewnoakes deleted the bump-roslyn branch June 17, 2024 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature-Language-Service Populating the Roslyn workspace with references, source files, analyzers, etc Performance-Scenario-General This issue affects performance in general. Performance-Scenario-Solution-Open This issue affects solution open performance. Tenet-Performance This issue affects the "Performance" tenet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants