Interactive projects refactoring #29069

tmat · 2018-08-03T23:59:06Z

Merges interactive projects into corresponding language service projects and InteractiveComponents VSIX to Roslyn.VisualStudio.Setup.

InteractiveHost*.exe now lives under DesktopHost subdirectory of Roslyn.VisualStudio.Setup VSIX install directory. The directory contains the minimal set of dependencies that the host needs to execute scripts.

Recommended review strategy is commit by commit.

tmat · 2018-08-04T00:02:37Z

@jasonmalinowski @ivanbasov PTAL

tmat · 2018-08-04T01:24:08Z

Filed issue on the warning: #29070.

jasonmalinowski · 2018-08-04T01:41:29Z

@tmat What required the "creation" (ish, I recognize it's kinda a rename) of EditorFeatures.Wpf stuff for C# and VB? Looking at what's in there, it looks like there's very little, if anything, that depends on Wpf per se. And the hope was we never did need these projects because we never expect to do language specific UIs, at least any time soon. What would prevent us from simply merging the code entirely?

A different way of looking at it: we want those features in Visual Studio for Mac, right? 😄

tmat · 2018-08-04T06:35:23Z

These few types depend on Interactive Window, which depends on WPF types. Note that InteractiveEvaluator, the base class of CSharpInteractiveEvaluator depends on IW.

src/Interactive/Host/Microsoft.CodeAnalysis.InteractiveHost.csproj

tmat · 2018-08-06T19:23:57Z

src/Setup/DevDivInsertionFiles/BuildDevDivInsertionFiles.vb

@@ -632,6 +632,11 @@ Public Class BuildDevDivInsertionFiles
                        Continue For
                    End If

+                    If partFileName.EndsWith(".resources.dll") Then


@tmeschter FYI. The Roslyn VSIX now includes DesktopHost directory that contains InteractiveHost.exe and satellite assemblies for its dependencies. Since InteractiveHost is launched as a separate process it needs all its dependencies (including satellites) in its directory.

@tmat Don't include the .resource.dll files in the main VSIX--that will cause us to install all of them regardless of the lang packs the user selects in the VS Installer.

I'm in the process of recreating the resource-specific VSIXes--which used to build in the VS repo--in the dotnet/roslyn repo. I'll handle laying out the resource DLLs for InteractiveHost*.exe in the new locations as part of that.

Do you mean that the resources will be included in a different VSIX than the binaries and VS assembly loader is able to find them there?
That won't work for Interactive Host though, since Interactive Host process has no knowledge of Visual Studio.

They will be in a different VSIX, yes, but a VSIX that installs to the same directory as the Language Service VSIX; there won't be any problem finding the resource assemblies.

That's already how our resource assemblies are handled in VS--I'm just moving the generation of those VSIXes out of the VS repo and into the dotnet/roslyn repo.

Oh, I see. OK.

tmat · 2018-08-07T18:10:17Z

test windows_debug_vs-integration_prtest please

tmat · 2018-08-08T16:55:18Z

test this please

sharwell · 2018-08-08T21:59:45Z

src/Compilers/CSharp/Portable/Microsoft.CodeAnalysis.CSharp.csproj

@@ -57,7 +57,7 @@
    <InternalsVisibleTo Include="csc" />
    <InternalsVisibleTo Include="csi" />
    <InternalsVisibleTo Include="Microsoft.CodeAnalysis.CSharp.Scripting" />
-    <InternalsVisibleTo Include="Microsoft.CodeAnalysis.CSharp.InteractiveEditorFeatures" />
+    <InternalsVisibleTo Include="Microsoft.CodeAnalysis.CSharp.EditorFeatures.Wpf" />


@jaredpar for approving this IVT

This is just a project rename.

@tmat Thanks, didn't realize that!

Woah this definitely shouldn't be there. It violates our principals. It's a rename so no blocker here but this needs to go.

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

What principles does it violate?

The compiler and "everything else" should never have IVT access between each other. Yeah this is violated today by debugging and scripting code. But once discovered we've been holding the line against further expansion of this.

I just did a quick check: only thing we seem to need here is BinderFlags and WithTopLevelBinderFlags. Maybe we should just make that public.

Yes, we should make them public. There is no expansion of that in this PR though.

Agree. This just alerted me to the problem and wanted to dive into it. Will file a follow up issue.

src/Deployment/RoslynDeployment.csproj

...ualStudio/VisualStudioInteractiveComponents/Roslyn.VisualStudio.InteractiveComponents.csproj

tmat · 2018-08-10T16:13:01Z

test windows_debug_vs-integration_prtest please

tmat · 2018-08-10T19:39:04Z

test windows_debug_vs-integration_prtest please

tmat · 2018-08-13T23:15:54Z

test windows_release_unit32_prtest please

tmat · 2018-08-13T23:47:45Z

test windows_debug_vs-integration_prtest please

tmat · 2018-08-13T23:47:52Z

test windows_release_vs-integration_prtest please

tmat · 2018-08-13T23:55:36Z

test windows_release_vs-integration_prtest please

tmat · 2018-08-14T01:51:39Z

test windows_debug_vs-integration_prtest please

tmat · 2018-08-15T16:02:55Z

test windows_debug_vs-integration_prtest please

tmat · 2019-02-12T01:10:28Z

@jasonmalinowski Ping

jasonmalinowski · 2019-02-28T20:58:47Z

@tmat Just to better understand this comment from awhile back:

These few types depend on Interactive Window, which depends on WPF types. Note that InteractiveEvaluator, the base class of CSharpInteractiveEvaluator depends on IW.

Is the core issue that at least IInteractiveWindow exposes an IWpfTextView? If it was only a regular ITextView would we have been able to keep a "WPF parts of the interactive window" and "platform independent parts of the interactive window?" I recognize I'm asking for a time machine (or a breaking change), just trying to understand exactly what it would take to support all of this on VS for Mac, at least as a thought experiment.

tmat · 2019-02-28T22:30:44Z

@jasonmalinowski Yes, that is correct. We can look into solving that issue, but it's certainly outside of the scope of this PR.

jasonmalinowski

Some comments. A bunch are just some quick fixes for things that tend to happen when you're doing large refactorings like this.

I am a bit worried though about the linked-file hackery going on that is resulting in more extern aliases and other confusion. This has bit us in the past and is a common thorn in the side. Can't tell if this is making things better or worse, but if there's an opportunity to make things better I think that might be good.

src/EditorFeatures/CSharp.Wpf/Interactive/CSharpInteractiveEvaluator.cs

src/EditorFeatures/CSharp.Wpf/Microsoft.CodeAnalysis.CSharp.EditorFeatures.Wpf.csproj

src/EditorFeatures/Core.Wpf/Microsoft.CodeAnalysis.EditorFeatures.Wpf.csproj

src/EditorFeatures/Core.Wpf/Interactive/InteractiveEvaluator.cs

src/VisualStudio/VisualBasic/Impl/VSPackage.resx

jasonmalinowski · 2019-02-28T23:20:25Z

...ualStudio/VisualStudioInteractiveComponents/Roslyn.VisualStudio.InteractiveComponents.csproj

-    <ProjectReference Include="..\..\Interactive\Host\InteractiveHost32.csproj">
-      <Ngen>false</Ngen>
+    <ProjectReference Include="..\..\Interactive\DesktopHost\InteractiveHost32.csproj">
+      <IncludeOutputGroupsInVSIX>BuiltProjectOutputGroup;BuiltProjectOutputGroupDependencies;GetCopyToOutputDirectoryItems</IncludeOutputGroupsInVSIX>


Isn't this still going to include binaries in this VSIX? This seems to be backwards from what I would have assumed which this is now empty per the comment at the top?

I guess we can remove the binaries.

Ah, no I think we need to keep these here. That's the point of the project - it collects all dependencies and adds them to VSIXSourceItem. This list is then returned from VsixSourceItemsOutputGroup target to the project that includes them.

Oh. Why aren't we just putting the project references for InteractiveHost that are here directly in the setup package then that is consuming the output group? The layer of indirection seems odd, especially because this isn't just pulling in direct InteractiveHost contents either -- since this is also referencing things like Microsoft.CodeAnalysis.CSharp.dll and also apparently including them, it means we're ending up with two paths where files end up in the final setup package.

it means we're ending up with two paths where files end up in the final setup package.

That is indeed the goal :)

But then we can't delete this project? Then it seems sad.

Right, the project is needed.

src/VisualStudio/VisualStudioInteractiveComponents/source.extension.vsixmanifest

src/Workspaces/Core/Portable/Microsoft.CodeAnalysis.Workspaces.csproj

src/Workspaces/Core/Portable/Utilities/TaskExtensions.cs

jasonmalinowski · 2019-02-28T23:34:43Z

but it's certainly outside of the scope of this PR.

Makes sense. I was mostly just heading off the inevitable question when somebody who works on VS for Mac asks "why is this all in the WPF layer?".

tmat · 2019-03-11T17:52:45Z

@jasonmalinowski OK to merge?

jasonmalinowski · 2019-03-12T21:33:35Z

@tmat Still a bunch of unanswered questions in my comments above?

tmat · 2019-03-12T21:41:09Z

@tmat Still a bunch of unanswered questions in my comments above?
Ha! I did not see those in CodeFlow. I'll answer.

tmat · 2019-03-12T21:42:26Z

I am a bit worried though about the linked-file hackery going on that is resulting in more extern aliases and other confusion. This has bit us in the past and is a common thorn in the side. Can't tell if this is making things better or worse, but if there's an opportunity to make things better I think that might be good.

I don't want to spend more time refactoring our extensions and helpers. We can fix that later, but that's outside of the scope of this PR. It's big as it is and I am continuously spending my time rebasing it.

tmat · 2019-03-13T00:37:56Z

@jasonmalinowski I believe all feedback has need resolved.

jasonmalinowski · 2019-03-13T23:23:57Z

@tmat I think only remaining question now is just this comment. I get we need a temporary project for F5 so we have an "empty" VSIX to temporarily replace the base which can go away once we're on a dogfood drop, but the fact this is also being used to flow build dependencies in a novel way to me is surprising. I'm not sure why we're not just moving that to the Roslyn.VisualStudio.Setup directly and skipping that project entirely.

tmat · 2019-03-14T17:43:21Z

We need the Roslyn.VisualStudio.Setup VSIX to have two copies of Microsoft.CodeAnalysis.dll and other binaries that both the InteractiveHost process and the IDE depend on. The resulting layout is like so:

Roslyn Language Services\{Version}
- Microsoft.CodeAnalysis.dll
- Microsoft.CodeAnalysis.EditorFeatures.dll
- Microsoft.CodeAnalysis.EditorFeatures.Text.dll
- Microsoft.CodeAnalysis.EditorFeatures.Wpf.dll
- Microsoft.CodeAnalysis.Features.dll
- Microsoft.CodeAnalysis.InteractiveHost.dll
- Microsoft.CodeAnalysis.Remote.Razor.ServiceHub.dll
- Microsoft.CodeAnalysis.Remote.ServiceHub.dll
- Microsoft.CodeAnalysis.Remote.Workspaces.dll
- Microsoft.CodeAnalysis.Scripting.dll
- Microsoft.CodeAnalysis.Workspaces.dll
- Microsoft.VisualStudio.LanguageServices.CSharp.dll
- Microsoft.VisualStudio.LanguageServices.dll
...
- DesktopHost
  - CSharpInteractive.rsp
  - InteractiveHost32.exe
  - InteractiveHost32.exe.config
  - InteractiveHost64.exe
  - InteractiveHost64.exe.config
  - Microsoft.CodeAnalysis.CSharp.dll
  - Microsoft.CodeAnalysis.CSharp.Scripting.dll
  - Microsoft.CodeAnalysis.dll
  - Microsoft.CodeAnalysis.Features.dll
  - Microsoft.CodeAnalysis.InteractiveHost.dll
  - Microsoft.CodeAnalysis.Scripting.dll
  - Microsoft.DiaSymReader.Native.amd64.dll
  - Microsoft.DiaSymReader.Native.x86.dll
  - System.Buffers.dll
  - System.Collections.Immutable.dll
  - System.Memory.dll
  - System.Numerics.Vectors.dll
  - System.Reflection.Metadata.dll
  - System.Runtime.CompilerServices.Unsafe.dll
  - System.Text.Encoding.CodePages.dll

tmat · 2019-03-18T19:00:25Z

@jasonmalinowski ping

Remove Repl projects Remove Microsoft.VisualStudio.InteractiveServices project Remove Microsoft.CodeAnalysis.InteractiveEditorFeatures Rename InteractiveEditorFeatures to EditorFeatures.Wpf Remove empty resource files Reduce InteractiveFeatures dependencies Merge InteractiveComponents content into Roslyn.VisualStudio.Setup VSIX Rename InteractiveFeatures to InteractiveHost Hook fatal error handlers Workaround warning CS1574 Exclude CreateTemplateManifestsCacheFile Do not include satellite assemblies in DesktopHost directory Remove VB interactive menu item Delete VisualBasic.EditorFeatures.Wpf Delete more unused VB interactive code

This reverts commit 41fb51d.

This reverts commit e0a8124.

* Revert "Revert "Interactive projects refactoring (#29069)"" This reverts commit e0a8124. * Fix bad merge

tmat requested review from a team as code owners August 3, 2018 23:59

tmat force-pushed the InteractiveRefactoring1 branch from 08fada5 to b417817 Compare August 4, 2018 01:05

tmat mentioned this pull request Aug 4, 2018

Incorrectly reported warning CS1574: XML comment has cref attribute that could not be resolved #29070

Open

sharwell reviewed Aug 6, 2018

View reviewed changes

src/Interactive/Host/Microsoft.CodeAnalysis.InteractiveHost.csproj Show resolved Hide resolved

tmat commented Aug 6, 2018

View reviewed changes

tmat force-pushed the InteractiveRefactoring1 branch 2 times, most recently from 3ed45e6 to 8d053a3 Compare August 6, 2018 21:59

tmat force-pushed the InteractiveRefactoring1 branch from c926a19 to 5df1409 Compare August 7, 2018 19:02

sharwell reviewed Aug 8, 2018

View reviewed changes

src/Deployment/RoslynDeployment.csproj Outdated Show resolved Hide resolved

...ualStudio/VisualStudioInteractiveComponents/Roslyn.VisualStudio.InteractiveComponents.csproj Outdated Show resolved Hide resolved

tmat force-pushed the InteractiveRefactoring1 branch 2 times, most recently from e2dedab to 7ea394d Compare August 9, 2018 21:57

tmat force-pushed the InteractiveRefactoring1 branch from 7ea394d to ccf2474 Compare August 13, 2018 19:25

tmat modified the milestones: 16.0.P4, 16.1 Feb 12, 2019

jasonmalinowski reviewed Feb 28, 2019

View reviewed changes

tmat force-pushed the InteractiveRefactoring1 branch from 1944693 to d8680a8 Compare March 10, 2019 01:16

tmat force-pushed the InteractiveRefactoring1 branch from b64eb47 to 7106bb6 Compare March 12, 2019 22:37

tmat force-pushed the InteractiveRefactoring1 branch from 0f099b7 to 4243e9e Compare March 15, 2019 22:10

tmat force-pushed the InteractiveRefactoring1 branch from 4243e9e to 7199a0c Compare March 18, 2019 19:08

jasonmalinowski approved these changes Mar 21, 2019

View reviewed changes

tmat force-pushed the InteractiveRefactoring1 branch from 7199a0c to d3dd2e4 Compare March 21, 2019 23:31

tmat merged commit 41fb51d into dotnet:master Mar 23, 2019

tmat deleted the InteractiveRefactoring1 branch March 23, 2019 00:40

sharwell added a commit that referenced this pull request Mar 23, 2019

Revert "Interactive projects refactoring (#29069)"

e0a8124

This reverts commit 41fb51d.

sharwell mentioned this pull request Mar 23, 2019

Revert "Interactive projects refactoring" #34382

Merged

tmat added a commit to tmat/roslyn that referenced this pull request Mar 24, 2019

Revert "Revert "Interactive projects refactoring (dotnet#29069)""

f279adb

This reverts commit e0a8124.

tmat added a commit that referenced this pull request Mar 25, 2019

Interactive Window refactoring (#34390)

7ab3b77

* Revert "Revert "Interactive projects refactoring (#29069)"" This reverts commit e0a8124. * Fix bad merge

filipw mentioned this pull request Mar 26, 2019

Track nullability in script code #33096

Merged

Interactive projects refactoring #29069

Interactive projects refactoring #29069

Conversation

tmat commented Aug 3, 2018 • edited Loading

tmat commented Aug 4, 2018

tmat commented Aug 4, 2018

jasonmalinowski commented Aug 4, 2018

tmat commented Aug 4, 2018 • edited Loading

tmat Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmeschter Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat commented Aug 7, 2018

tmat commented Aug 8, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat commented Aug 10, 2018

tmat commented Aug 10, 2018

tmat commented Aug 13, 2018

tmat commented Aug 13, 2018

tmat commented Aug 13, 2018

tmat commented Aug 13, 2018

tmat commented Aug 14, 2018

tmat commented Aug 15, 2018

tmat commented Feb 12, 2019

jasonmalinowski commented Feb 28, 2019

tmat commented Feb 28, 2019

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

tmat Mar 12, 2019 • 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

Choose a reason for hiding this comment

jasonmalinowski commented Feb 28, 2019

tmat commented Mar 11, 2019

jasonmalinowski commented Mar 12, 2019

tmat commented Mar 12, 2019

tmat commented Mar 12, 2019

tmat commented Mar 13, 2019

jasonmalinowski commented Mar 13, 2019

tmat commented Mar 14, 2019

tmat commented Mar 18, 2019

tmat commented Aug 3, 2018 •

edited

Loading

tmat commented Aug 4, 2018 •

edited

Loading

tmat Aug 6, 2018 •

edited

Loading

tmeschter Aug 6, 2018 •

edited

Loading

tmat Mar 12, 2019 •

edited

Loading