Update vssdk versions #27027

jmarolf · 2018-05-21T20:00:09Z

Updates roslyn to use the latest editor nuget packages

Known issues that require follow-up

Product bug breaking Line Commit: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/635902 (with PR https://devdiv.visualstudio.com/DevDiv/_git/VS-Platform/pullrequest/128892)
- Also needs explicit tests rather than the implicit tests we were lucky to hit: Add Tests for product bug: VB Line Commit happening without the caret leaving the line #27988
Test bug around SplitStringLiteral: Unit Tests: Caret position not properly restored after undo when the original caret position doesn't match the location of the text change #28033

sharwell · 2018-05-21T22:13:17Z

build/Rulesets/Roslyn_BuildRules.ruleset

+    <Rule Id="VSTHRD001" Action="None" /> <!-- don't enforce useage of JTF -->
+    <Rule Id="VSTHRD002" Action="None" /> <!-- don't enforce useage of JTF -->
+    <Rule Id="VSTHRD010" Action="None" /> <!-- don't enforce useage of JTF -->
+    <Rule Id="VSTHRD103" Action="None" /> <!-- without flow analysis this rule has a lot of false positives and provides little value -->


❔ Was the problem here flow analysis, or lack of support for local functions?

I can investigate the implementation further but fundamentally I have never seen VSTHRD103 catch a product bug. It uses string matching to try and determine if methods have async versions. You can look at the source here: https://github.com/Microsoft/vs-threading/blob/29d8c6ebb3758ccf1aa1488feb7083ae4744a267/src/Microsoft.VisualStudio.Threading.Analyzers/VSTHRD103UseAsyncOptionAnalyzer.cs

It caught several bugs in GitExtensions during its conversion. Arguably one of the most helpful analyzers of the entire set there.

You indicated in this comment that the analyzer was reporting false positives. I have not seen this occur outside of local functions, and was wondering what you saw.

Well in project system we implement or use methods that are named similar to other a sync methods. As a result we had hundreds of false positives about a synchronous API that is implementing and interface needing to call an async method when that is not the method contract, there was a separate async method that did call the async api. Turning this on for roslyn just seems to trigger on t.Result calls though, so I have no problem turning it on.

sharwell · 2018-05-21T22:14:36Z

build/Rulesets/Roslyn_BuildRules.ruleset

+    <Rule Id="VSTHRD002" Action="None" /> <!-- don't enforce useage of JTF -->
+    <Rule Id="VSTHRD010" Action="None" /> <!-- don't enforce useage of JTF -->
+    <Rule Id="VSTHRD103" Action="None" /> <!-- without flow analysis this rule has a lot of false positives and provides little value -->
+    <Rule Id="VSTHRD200" Action="None" /> <!-- don't enforce that names must end in 'Async' they are either tests or part of a public API -->


💭 Seems like this should eventually be moved to the non-shipping projects rule set, and suppressions added for anything that shipped in the public API.

I feel that this should be enforced via naming styles in editorconfig that allows us to specify our exact rules instead of relying on an analyzer whose rules may not meet our needs.

sharwell · 2018-05-21T22:18:04Z

📝 If there are any remaining cases of async void after this pull request is merged, an issue should be filed to remove them in the future. I have no objection to baselining (via suppressions) within this pull request.

heejaechang · 2018-05-31T18:58:00Z

@jmarolf when this PR will go in? I need this PR to check in BulkFileOperation change.

tagging @jinujoseph

heejaechang · 2018-05-31T19:00:46Z

src/VisualStudio/VisualBasic/Impl/BasicVisualStudio.vbproj

@@ -56,6 +56,7 @@
    <PackageReference Include="Microsoft.VisualStudio.ComponentModelHost" Version="$(MicrosoftVisualStudioComponentModelHostVersion)" />
    <PackageReference Include="Microsoft.VisualStudio.Diagnostics.PerformanceProvider" Version="$(MicrosoftVisualStudioDiagnosticsPerformanceProviderVersion)" />
    <PackageReference Include="Microsoft.VisualStudio.Shell.Design" Version="$(MicrosoftVisualStudioShellDesignVersion)" />
+    <PackageReference Include="Microsoft.VisualStudio.Threading" Version="$(MicrosoftVisualStudioThreadingVersion)" />


I don't know much on this area but is there a reason why Microsoft.VisualStudio.Threading is explicitly added everywhere? I thought this PR is about updating vssdk version. not adding new dependency. is it because of some version conflicts?

Indeed, @jmarolf can we split this out into two PRs?

heejaechang · 2018-05-31T19:03:09Z

build/Rulesets/Roslyn_BuildRules.ruleset

@@ -106,6 +106,13 @@
  <Rules AnalyzerId="Microsoft.NetCore.Analyzers" RuleNamespace="Microsoft.NetCore.Analyzers.ImmutableCollections">
    <Rule Id="RS0012" Action="Warning" />
  </Rules>
+  <Rules AnalyzerId="Microsoft.VisualStudio.Threading.Analyzers" RuleNamespace="Microsoft.VisualStudio.Threading.Analyzers">


so, the MS.VS.Threading.Analyzers package contains not only dll, but analyzers I guess? and that is why we are seeing all these new errors? since this PR is about updating vssdk, shouldn't all those new analyzers disabled by default (since it is new thing) and track that issue in another PR (finding out and decising which analyzer should be enabled and what is not?) so that this PR is not blocked by this?

jasonmalinowski

We've been trying to move to using PackageReferences-flow-across-ProjectReferences to our advantage, which means you should only be adding Microsoft.VisualStudio.Threading in as few places as possible. I'm also with @heejaechang that although the things it's flagging are perhaps worth flagging, I think we should split that out into a separate PR if possible.

jasonmalinowski · 2018-05-31T19:52:20Z

src/EditorFeatures/CSharp/CSharpEditorFeatures.csproj

@@ -43,6 +43,7 @@
  <ItemGroup>
    <Reference Include="System.ComponentModel.Composition" />
    <PackageReference Include="Microsoft.VisualStudio.Language.Intellisense" Version="$(MicrosoftVisualStudioLanguageIntellisenseVersion)" />
+    <PackageReference Include="Microsoft.VisualStudio.Threading" Version="$(MicrosoftVisualStudioThreadingVersion)" />


We do want to rely on flowing through project references: assuming this is being added in EditorFeatures.csproj we shouldn't be adding this here.

jasonmalinowski · 2018-05-31T19:53:04Z

src/EditorFeatures/Core.Wpf/EditorFeatures.Wpf.csproj

@@ -38,6 +38,7 @@
    <PackageReference Include="Microsoft.VisualStudio.Imaging" Version="$(MicrosoftVisualStudioImagingVersion)" />
    <PackageReference Include="Microsoft.VisualStudio.ImageCatalog" Version="$(MicrosoftVisualStudioImageCatalogVersion)" />
    <PackageReference Include="Microsoft.VisualStudio.Text.UI.Wpf" Version="$(MicrosoftVisualStudioTextUIWpfVersion)" />
+    <PackageReference Include="Microsoft.VisualStudio.Threading" Version="$(MicrosoftVisualStudioThreadingVersion)" />


Don't add if it's coming from project references.

I'll fix this up. I agree adding this is wrong

Let us revert this if it is not needed.

In reply to: 192248134 [](ancestors = 192248134)

jasonmalinowski · 2018-05-31T19:53:27Z

src/EditorFeatures/Core/EditorFeatures.csproj

@@ -29,6 +29,7 @@
    <PackageReference Include="Microsoft.VisualStudio.Language.Intellisense" Version="$(MicrosoftVisualStudioLanguageIntellisenseVersion)" />
    <PackageReference Include="Microsoft.VisualStudio.Imaging.Interop.14.0.DesignTime" Version="$(MicrosoftVisualStudioImagingInterop140DesignTimeVersion)" />
    <PackageReference Include="Microsoft.VisualStudio.Text.UI" Version="$(MicrosoftVisualStudioTextUIVersion)" />
+    <PackageReference Include="Microsoft.VisualStudio.Threading" Version="$(MicrosoftVisualStudioThreadingVersion)" />


Why adding this? Does this need nuspec updates as well?

Let us revert this if they are not needed.

In reply to: 192217464 [](ancestors = 192217464)

fixed packages config flow

jmarolf · 2018-06-05T18:00:37Z

@heejaechang just need this to be reviewed to unblock you

jinujoseph · 2018-06-14T03:37:02Z

cc @ivanbasov @dpoeschl for info and review

dpoeschl

There's some whitespace issues in TestObscuringTipManager & TestWaitIndicator (not sure how that happened), but it looks good. If this build is green, I wouldn't block on the formatting.

dpoeschl · 2018-06-20T22:50:08Z

@heejaechang @ivanbasov @sharwell @jasonmalinowski -- Unit tests seem to be "passing" now. 😄 Please re-review.

dpoeschl · 2018-06-20T22:50:24Z

/cc: @jinujoseph

sharwell · 2018-06-20T22:50:55Z

src/EditorFeatures/CSharpTest/SplitStringLiteral/SplitStringLiteralCommandHandlerTests.cs

        }

        [WorkItem(20258, "https://github.com/dotnet/roslyn/issues/20258")]
        [WpfFact, Trait(Traits.Feature, Traits.Features.SplitStringLiteral)]
        public void TestBeforeEndQuote2()
        {
+            // Do not verifyUndo because of https://github.com/dotnet/roslyn/issues/28033
+            // When that issue is fixed, we can reenable verifyUndo
+            // </summary>


💭 Have some stray "XML" elements

sharwell · 2018-06-20T22:52:07Z

src/EditorFeatures/TestUtilities/TestObscuringTipManager.cs

-    [Export(typeof(IObscuringTipManager))]
-    internal class TestObscuringTipManager : IObscuringTipManager
-    {
+{	


💡 Doesn't seem necessary to add a bunch of trailing whitespace...

sharwell · 2018-06-20T22:52:36Z

src/EditorFeatures/TestUtilities/TestWaitIndicator.cs

-        private readonly IWaitContext _waitContext;
-        private readonly Microsoft.VisualStudio.Language.Intellisense.Utilities.IWaitContext _platformWaitContext = new UncancellableWaitContext();
-
+    {	


💡 Bunch of trailing whitespace additions here too

sharwell · 2018-06-20T22:54:19Z

src/EditorFeatures/VisualBasicTest/DocumentationComments/DocumentationCommentTests.vb

@@ -1204,7 +1206,7 @@ End Class
        End Function

        Protected Overrides Function CreateTestWorkspace(code As String) As TestWorkspace
-            Return TestWorkspace.CreateVisualBasic(code)
+            Return TestWorkspace.CreateVisualBasic(code, exportProvider:=ExportProviderCache.GetOrCreateExportProviderFactory(TestExportProvider.EntireAssemblyCatalogWithCSharpAndVisualBasic.WithoutPartsOfType(GetType(CommitConnectionListener))).CreateExportProvider())


@jasonmalinowski I thought you implemented the CommitConnectionListener somewhere else?

Edit: It was 15b76be03f435880c7bda4d0d5f66379159583b6. Would be good to consolidate this PR and the other on approach. #Resolved

@ivanbasov #Resolved

CommitConnectionListener is an existing file. VSSDK attempts to call within some unit test comparing to the previous version. To provide a unity of those tests, it would be better to exclude this class.

In reply to: 196966852 [](ancestors = 196966852)

jasonmalinowski · 2018-07-17T23:17:40Z

@dotnet-bot test ci please.

…k-versions

Our unofficial packages don't correctly pull in some DesignTime dependencies (since they always create packages without any dependencies), but this caused issues once we moved to AsyncPackage since there's new PIA types being involved. For now, to sort this out just move back to the official packages which just do the right thing to start. As a part of this, I also move us to the official Microsoft.VisualStudio.SDK.EmbedInteropTypes package; we had our own clone of the code but it wasn't covering all the assemblies that we now need to embed as well.

jasonmalinowski · 2018-07-19T17:54:02Z

build/Targets/Packages.props

    <MicrosoftVisualStudioEditorVersion>15.8.414-preview</MicrosoftVisualStudioEditorVersion>
    <MicrosoftVisualStudioGraphModelVersion>15.8.27812-alpha</MicrosoftVisualStudioGraphModelVersion>
-    <MicrosoftVisualStudioImageCatalogVersion>15.8.27812-alpha</MicrosoftVisualStudioImageCatalogVersion>


The editor already carries along this as a dependency; in some cases they have a higher version than we did so we were just creating package downgrade issues. This removes them.

jasonmalinowski · 2018-07-19T17:54:21Z

build/Targets/Packages.props

    <MicrosoftVisualStudioShellInteropVersion>7.10.6072</MicrosoftVisualStudioShellInteropVersion>
    <MicrosoftVisualStudioShellInterop100Version>10.0.30320</MicrosoftVisualStudioShellInterop100Version>
    <MicrosoftVisualStudioShellInterop110Version>11.0.61031</MicrosoftVisualStudioShellInterop110Version>
    <MicrosoftVisualStudioShellInterop121DesignTimeVersion>12.1.30328</MicrosoftVisualStudioShellInterop121DesignTimeVersion>
-    <MicrosoftVisualStudioShellInterop140DesignTimeVersion>14.3.26929</MicrosoftVisualStudioShellInterop140DesignTimeVersion>
-    <MicrosoftVisualStudioShellInterop150DesignTimeVersion>15.8.27812-alpha</MicrosoftVisualStudioShellInterop150DesignTimeVersion>
-    <MicrosoftVisualStudioShellInterop153DesignTimeVersion>15.8.27812-alpha</MicrosoftVisualStudioShellInterop153DesignTimeVersion>


These are all dependencies of the main shell packages.

jasonmalinowski · 2018-07-19T17:54:55Z

@ivanbasov @dpoeschl @jinujoseph @jmarolf So I had to merge this with the AsyncPackage changes, which created a bunch of issues. Our 15.8 self-created packages were missing dependencies that caused other build issues. I bit the bullet and fixed it so we're building cleanly again...but against Shell 15.7 packages. We are still using Editor 15.8 packages. My plan is to merge this PR, then upgrade back to the Shell 15.8 packages in a separate PR.

jasonmalinowski · 2018-07-19T17:57:29Z

@dotnet-bot test ci please.

jasonmalinowski · 2018-07-19T17:59:30Z

src/VisualStudio/Core/Def/Microsoft.VisualStudio.LanguageServices.csproj

-       To deal with that, we write this target which finds that added package, removes it from
-       the references list, then adds it back in again with the 'EmbedInteropTypes' flag properly
-       set. -->
-  <Target Name="MarkNuGetVisualStudioPackageWithEmbedInteropType" AfterTargets="ResolvePackageDependenciesForBuild">


This commit moves us to the standard NuGet package for doing exclusions, which already does this for NuGet.VisualStudio.

On our 15.8 machines, there aren't release versions...because we haven't released it yet.

jasonmalinowski · 2018-07-19T19:49:30Z

@dotnet-bot test ci please.

jasonmalinowski · 2018-07-20T17:42:29Z

@dotnet-bot test ci please.

jasonmalinowski · 2018-07-24T18:18:29Z

Right now this is waiting for dotnet/core-eng#3920 to get completed; it seemed the integration test run against the older image was failing due to what looked like editor bugs.

jasonmalinowski · 2018-07-24T21:32:18Z

@dotnet-bot test ci please.

…k-versions

jasonmalinowski · 2018-07-26T21:55:14Z

@dotnet-bot test ci please.

jasonmalinowski · 2018-07-26T23:04:57Z

@dotnet test ci please.

…k-versions

jasonmalinowski · 2018-07-31T00:22:35Z

@dotnet-bot test ci please.

jasonmalinowski · 2018-07-31T16:01:01Z

@dotnet-bot test ci please.

jasonmalinowski · 2018-07-31T19:26:43Z

Merging: the integration test failures on this are because we are also carrying along a change to the netci.groovy; the test ci please jobs were ran to verify everything works.

jmarolf requested review from a team as code owners May 21, 2018 20:00

jmarolf requested review from jaredpar and heejaechang May 21, 2018 20:00

jmarolf requested a review from a team as a code owner May 21, 2018 21:02

sharwell reviewed May 21, 2018

View reviewed changes

jmarolf force-pushed the update-vssdk-versions branch from 32b0351 to b8e3eaf Compare May 22, 2018 11:28

heejaechang reviewed May 31, 2018

View reviewed changes

heejaechang mentioned this pull request May 31, 2018

added BulkFileOperation listener so that we can pause our ambient wor… #27301

Merged

jasonmalinowski previously requested changes May 31, 2018

View reviewed changes

jcouv added the Area-Infrastructure label May 31, 2018

jmarolf force-pushed the update-vssdk-versions branch from b8e3eaf to ce7fe82 Compare June 1, 2018 01:20

jmarolf requested a review from a team as a code owner June 1, 2018 01:20

jmarolf force-pushed the update-vssdk-versions branch from 1bd82dd to a36d881 Compare June 3, 2018 01:29

jmarolf force-pushed the update-vssdk-versions branch from a36d881 to 59957d8 Compare June 14, 2018 00:02

jmarolf force-pushed the update-vssdk-versions branch from 6fbcf49 to 7a8c4a8 Compare June 15, 2018 19:39

dpoeschl removed request for a team and jaredpar June 19, 2018 20:31

dpoeschl approved these changes Jun 20, 2018

View reviewed changes

sharwell reviewed Jun 20, 2018

View reviewed changes

jasonmalinowski changed the base branch from master-vs-deps to dev15.8.x-vs-deps July 16, 2018 22:22

jinujoseph changed the base branch from dev15.8.x-vs-deps to master-vs-deps July 17, 2018 18:49

jasonmalinowski added 3 commits July 18, 2018 15:34

Merge remote-tracking branch 'dotnet/master-vs-deps' into update-vssd…

228126f

…k-versions

Make DiagnosticsWindow an async package

3afb9a5

jasonmalinowski reviewed Jul 19, 2018

View reviewed changes

jinujoseph added this to the 16.0 milestone Jul 19, 2018

jasonmalinowski reviewed Jul 19, 2018

View reviewed changes

Pass -prerelease to the vswhere query

00bb1c2

On our 15.8 machines, there aren't release versions...because we haven't released it yet.

jasonmalinowski mentioned this pull request Jul 23, 2018

Migrate to Microsoft.VisualStudio.Threading #28781

Merged

Merge remote-tracking branch 'dotnet/master-vs-deps' into update-vssd…

55f71e1

…k-versions

Merge branch 'fix-ci' into update-vssdk-versions

11545b2

Merge remote-tracking branch 'dotnet/master-vs-deps' into update-vssd…

baf83f2

…k-versions

jasonmalinowski approved these changes Jul 31, 2018

View reviewed changes

jasonmalinowski merged commit 1434174 into dotnet:master-vs-deps Jul 31, 2018

jasonmalinowski mentioned this pull request Jul 31, 2018

Remove Microsoft.VisualStudio.Text.Internal hack #27490

Merged

jmarolf deleted the update-vssdk-versions branch January 27, 2020 17:06

Update vssdk versions #27027

Update vssdk versions #27027

Conversation

jmarolf commented May 21, 2018 • edited by dpoeschl Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell commented May 21, 2018

heejaechang commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarolf commented Jun 5, 2018

jinujoseph commented Jun 14, 2018

dpoeschl left a comment

Choose a reason for hiding this comment

dpoeschl commented Jun 20, 2018

dpoeschl commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Jun 20, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

dpoeschl Jun 20, 2018 • edited by ivanbasov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski commented Jul 19, 2018

jasonmalinowski commented Jul 19, 2018

Choose a reason for hiding this comment

jasonmalinowski commented Jul 19, 2018

jasonmalinowski commented Jul 20, 2018

jasonmalinowski commented Jul 24, 2018

jasonmalinowski commented Jul 24, 2018

jasonmalinowski commented Jul 26, 2018

jasonmalinowski commented Jul 26, 2018

jasonmalinowski commented Jul 31, 2018

jasonmalinowski commented Jul 31, 2018

jasonmalinowski commented Jul 31, 2018

jmarolf commented May 21, 2018 •

edited by dpoeschl

Loading

sharwell May 22, 2018 •

edited

Loading

sharwell Jun 20, 2018 •

edited by ivanbasov

Loading

dpoeschl Jun 20, 2018 •

edited by ivanbasov

Loading