Update vssdk versions by jmarolf · Pull Request #27027 · dotnet/roslyn

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

❔ 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

💭 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

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

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

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

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

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

💭 Have some stray "XML" elements

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

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

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

💡 Bunch of trailing whitespace additions here too

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

@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

+    <MicrosoftVisualStudioSDKEmbedInteropTypesVersion>15.0.17</MicrosoftVisualStudioSDKEmbedInteropTypesVersion>
    <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

    <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

-       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 heejaechang and jaredpar 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

Conversation

jmarolf commented May 21, 2018 • edited by dpoeschl Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented May 21, 2018

Uh oh!

heejaechang commented May 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarolf commented Jun 5, 2018

Uh oh!

jinujoseph commented Jun 14, 2018

Uh oh!

dpoeschl left a comment

Choose a reason for hiding this comment

Uh oh!

dpoeschl commented Jun 20, 2018

Uh oh!

dpoeschl commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Jun 20, 2018 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpoeschl Jun 20, 2018 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski commented Jul 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski commented Jul 19, 2018

Uh oh!

jasonmalinowski commented Jul 19, 2018

Uh oh!

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