Rename `Microsoft.Terminal.TerminalControl` to `.Control`; Split into dll & lib #9472

zadjii-msft · 2021-03-12T18:30:36Z

BE NOT AFRAID. I know that there's 107 files in this PR, but almost
all of it is just find/replacing TerminalControl with Control.

This is the start of the work to move TermControl into multiple pieces,
for #5000. The PR starts this work by:

Splits TerminalControl into separate lib and dll projects. We'll
want control tests in the future, and for that, we'll need a lib.
Moves ICoreSettings back into the Microsoft.Terminal.Core
namespace. We'll have other types in there soon too.
- I could not tell you why this works suddenly. New VS versions? New
  cppwinrt version? Maybe we're just better at dealing with mdmerge
  bugs these days.
RENAMES Microsoft.Terminal.TerminalControl to
Microsoft.Terminal.Control. This touches pretty much every file in
the sln. Sorry about that (not sorry).

An upcoming PR will move much of the logic in TermControl into a new
ControlCore class that we'll add in Microsoft.Terminal.Core.
ControlCore will then be unittest-able in the
UnitTests_TerminalCore, which will help prevent regressions like #9455

Detailed Description of the Pull Request / Additional comments

You're really gonna want to clean the sln first, then merge this into
your branch, then rebuild. It's very likely that old winmds will get
left behind. If you see something like

Error    MDM2007    Cannot create type
Microsoft.Terminal.TerminalControl.KeyModifiers in read-only metadata
file Microsoft.Terminal.TerminalControl.

then that's what happened to you.

Wadda ya know, we _can_ have TerminalCore expose types. Maybe we just got that in a recent VS / cppwinrt update, but it works finally

…Control

DHowett · 2021-03-12T19:15:52Z

If we're gonna rename the namespace, now is as good a time as any to rename the DLL file too. Microsoft.Terminal.Control.dll.

DHowett · 2021-03-12T19:16:31Z

(WinRT consumers may contractually (apparently) try to resolve namespace X.Y.Z by doing LoadLibrary(X.Y.Z.dll), X.Y.dll, and X.dll)

DHowett · 2021-03-12T19:50:25Z

I still don't understand the thesis. If we make the interactivity stuff part of Core, that's what we test. Are we doing work today (make the UWP Control a lib) that we'll be undoing later?

zadjii-msft · 2021-03-12T19:54:57Z

If we're gonna rename the namespace, now is as good a time as any to rename the DLL file too. Microsoft.Terminal.Control.dll.

yea okay

Are we doing work today (make the UWP Control a lib) that we'll be undoing later?

probably not? We might be adding tests in the future - when we do, we'll be happy that we have a lib. But we will be moving most of the testable code into M.T.Core.lib, which is already easily testable, so that's not the biggest worry. What I am worried about is that the delicate house of cards we have will fall over if I revert part of it. I did the dll->dll&lib split first, then did the Control.ICoreSettings -> Core.ICoreSettings thing. Part of me is worried that will no longer work if I revert the lib&dll split

OpenConsole.sln

DHowett · 2021-03-12T19:59:16Z

OpenConsole.sln

+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.AuditMode|Any CPU.ActiveCfg = Debug|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.AuditMode|ARM.ActiveCfg = AuditMode|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.AuditMode|ARM64.ActiveCfg = Release|ARM64
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.AuditMode|DotNet_x64Test.ActiveCfg = Debug|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.AuditMode|DotNet_x86Test.ActiveCfg = Debug|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.AuditMode|x64.ActiveCfg = Release|x64
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.AuditMode|x86.ActiveCfg = Release|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Debug|Any CPU.ActiveCfg = Debug|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Debug|ARM.ActiveCfg = Debug|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Debug|ARM64.ActiveCfg = Debug|ARM64
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Debug|ARM64.Build.0 = Debug|ARM64
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Debug|DotNet_x64Test.ActiveCfg = Debug|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Debug|DotNet_x86Test.ActiveCfg = Debug|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Debug|x64.ActiveCfg = Debug|x64
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Debug|x64.Build.0 = Debug|x64
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Debug|x86.ActiveCfg = Debug|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Debug|x86.Build.0 = Debug|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Release|Any CPU.ActiveCfg = Release|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Release|ARM.ActiveCfg = Release|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Release|ARM64.ActiveCfg = Release|ARM64
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Release|ARM64.Build.0 = Release|ARM64
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Release|DotNet_x64Test.ActiveCfg = Release|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Release|DotNet_x86Test.ActiveCfg = Release|Win32
+		{CA5CAD1A-5555-4AC7-AC72-6CA5B3AB89ED}.Release|x64.ActiveCfg = Release|x64


please make absolutely certain your project configurations are correct for a new project. We had some issues earlier where new projects were set to build for DotNet_x86Test and DotNet_x64Test and AnyCPU(!) on debug/release when they were not projects that the wpf control needed. This caused build failures later on when the release pipeline tried to build the WPF control on its own.

I suspect this project is Debug+Release x86+64 only, no audit no dotnet no anycpu...

I straight up copied TerminalControl's original lines here. If this is wrong, how will I know?

it looks like *|DotNet_x64Test.*, *|DotNet_x86Test.*, and *|Any CPU.* all always map to Win32. The Audit lines are all weird:

{CA5CAD1A-F542-4635-A069-7CAEFB930070}.AuditMode|Any CPU.ActiveCfg = Debug|Win32 {CA5CAD1A-F542-4635-A069-7CAEFB930070}.AuditMode|ARM.ActiveCfg = AuditMode|Win32 {CA5CAD1A-F542-4635-A069-7CAEFB930070}.AuditMode|ARM64.ActiveCfg = Release|ARM64 {CA5CAD1A-F542-4635-A069-7CAEFB930070}.AuditMode|DotNet_x64Test.ActiveCfg = Debug|Win32 {CA5CAD1A-F542-4635-A069-7CAEFB930070}.AuditMode|DotNet_x86Test.ActiveCfg = Debug|Win32 {CA5CAD1A-F542-4635-A069-7CAEFB930070}.AuditMode|x64.ActiveCfg = Release|x64 {CA5CAD1A-F542-4635-A069-7CAEFB930070}.AuditMode|x86.ActiveCfg = Release|Win32

presumably though should all map to either Audit, Debug or Release, but presumably all the same thing.

Then again, TerminalAppLib is the same way:

{CA5CAD1A-9A12-429C-B551-8562EC954746}.AuditMode|Any CPU.ActiveCfg = Debug|Win32 {CA5CAD1A-9A12-429C-B551-8562EC954746}.AuditMode|ARM.ActiveCfg = AuditMode|Win32 {CA5CAD1A-9A12-429C-B551-8562EC954746}.AuditMode|ARM64.ActiveCfg = Release|ARM64 {CA5CAD1A-9A12-429C-B551-8562EC954746}.AuditMode|DotNet_x64Test.ActiveCfg = Debug|Win32 {CA5CAD1A-9A12-429C-B551-8562EC954746}.AuditMode|DotNet_x86Test.ActiveCfg = Debug|Win32 {CA5CAD1A-9A12-429C-B551-8562EC954746}.AuditMode|x64.ActiveCfg = Release|x64 {CA5CAD1A-9A12-429C-B551-8562EC954746}.AuditMode|x86.ActiveCfg = Release|Win32

so maybe this is fine

src/cascadia/TerminalApp/TerminalAppLib.vcxproj

DHowett · 2021-03-12T20:01:38Z

src/cascadia/TerminalApp/dll/TerminalApp.vcxproj

+    <!-- Manually add a reference to TerminalControl here. We need this so
+    MDMERGE will know where the TermControl types are defined. However, we need
+    to do it exactly like this so the packaging project won't roll up
+    TermControl's .xbf's from both the TermControl project and this one. -->
+<!--     <Reference Include="Microsoft.Terminal.Control">
+      <HintPath>$(OpenConsoleCommonOutDir)TerminalControl\Microsoft.Terminal.Control.winmd</HintPath>
+      <IsWinMDFile>true</IsWinMDFile>
+      <Private>false</Private>
+      <CopyLocalSatelliteAssemblies>false</CopyLocalSatelliteAssemblies>
+    </Reference> -->
+


This comment spells doom and gloom if we don't do this thing, but then it's commented out. Why do we NOT need it?

I'm gonna be real with you - no idea why this works now. maybe it thinks the .xbfs belong to TerminalControlLib now, not TerminalControl.dll?

src/cascadia/TerminalControl/TerminalControlLib.vcxproj

DHowett · 2021-03-12T20:05:56Z

src/cascadia/TerminalControl/TerminalControlLib.vcxproj

-      <WinMDReferenceToCompile Remove="@(WinMDReferenceToCompile)" Condition="'%(Filename)' == '$(RootNamespace)'"/>
-    </ItemGroup>
-  </Target>
+  <Import Project="$(SolutionDir)build\rules\CollectWildcardResources.targets" />


shocked that resources might just "work", scared too

DHowett · 2021-03-12T20:06:49Z

src/cascadia/TerminalControl/dll/TerminalControl.vcxproj

+         Microsoft namespace. This is, obviously, not great. None of our other
+         projects compile properly when they depend on this "Microsoft.winmd."
+    -->
+    <CppWinRTNamespaceMergeDepth>3</CppWinRTNamespaceMergeDepth>


we should not be doing any namespace merging in this DLL file -- are we?

src/cascadia/TerminalControl/dll/pch.cpp

src/cascadia/WindowsTerminal/WindowsTerminal.vcxproj

…rminalcontrol-into-lib # Conflicts: # src/cascadia/TerminalApp/TerminalSettings.h # src/cascadia/TerminalSettingsModel/TerminalSettings.cpp

DHowett

108/108, looks fine to me. Mechanical. Makes our build times slower, I suspect, but what doesn't these days? Thanks for doing my renames. 😄

DHowett · 2021-03-17T19:24:42Z

@microsoft/windows-console-team this is a mechanical change -- can we get it reviewed?

DHowett · 2021-03-17T19:35:08Z

Body and title edited to fit commit style (ignore me)

ghost · 2021-03-17T20:47:21Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

TIL that the `<None Include="Foo.def" />` line in our projects is actually totally meaningless. The important line is the one that's in `cppwinrt.build.pre.props`, where we declare ```xml <ModuleDefinitionFile Condition="Exists('$(ProjectName).def')">$(ProjectName).def</ModuleDefinitionFile> ``` So if you change a project's name, and not the `.def` file, then the linker will just _not use the `.def` file at all_. More importantly, this seemingly doesn't matter in debug builds. In a Debug build, the linker will happily still include `WINRT_CanUnloadNow` and `WINRT_GetActivationFactory` in the exports from the dll, even without the `.def`. But in a Release build, the linker is much more agressive about pruning symbols that aren't referenced, and without those two, NONE of the symbols are eventually referenced. This PR fixes `Microsoft.Terminal.Control` by renaming the `.def`, and makes it marginally harder for someone to make the same mistake in the future. ## References * Regressed in #9472 ## PR Checklist * [x] Closes #9529 * [x] I work here

Does what it says on the can. This is a follow up to #9472. Now that we have a control .lib, we can add tests for it. Unfortunately, the `TermControl` itself is a horrible mess. So this new unittest lib is empty for now. I'm working on actual tests as a part of #6842, but this PR is here to keep the diffs smaller. Also, apparently `server.vcxproj` had the wrong GUID in it. * [x] I work here * [x] Adds tests

zadjii-msft added 6 commits March 12, 2021 06:56

Hokay, so that gets the control as a lib/dll split

2402c59

its-all-coming-together.gif

24765d7

can I get a big woop woop from all my homies

54f443b

Wadda ya know, we _can_ have TerminalCore expose types. Maybe we just got that in a recent VS / cppwinrt update, but it works finally

Fix the tests

322d511

Rename Microsoft.Terminal.TerminalControl to just Microsoft.Terminal.…

8f8cf59

…Control

some dead code

eee5bee

DHowett requested changes Mar 12, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Mar 12, 2021

zadjii-msft added 2 commits March 16, 2021 10:35

Change the GUID, project name

04d403c

Merge remote-tracking branch 'origin/main' into dev/migrie/r/split-te…

e8d06d7

…rminalcontrol-into-lib # Conflicts: # src/cascadia/TerminalApp/TerminalSettings.h # src/cascadia/TerminalSettingsModel/TerminalSettings.cpp

DHowett approved these changes Mar 16, 2021

View reviewed changes

oh! IT CAUGHT AN ISSUE! YAY!

f8e969e

zadjii-msft added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Needs-Second It's a PR that needs another sign-off labels Mar 16, 2021

ghost requested review from miniksa, carlos-zamora, leonMSFT and PankajBhojwani March 16, 2021 19:57

DHowett changed the title ~~Rename Microsoft.Terminal.TerminalControl to Microsoft.Terminal.Control; Split Control into dll & lib~~ Rename Microsoft.Terminal.TerminalControl to .Control; Split into dll & lib Mar 17, 2021

carlos-zamora approved these changes Mar 17, 2021

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 17, 2021

ghost merged commit d749df7 into main Mar 17, 2021

ghost deleted the dev/migrie/r/split-terminalcontrol-into-lib branch March 17, 2021 20:47

zadjii-msft mentioned this pull request Mar 18, 2021

Fix build break where Microsoft.Terminal.Control.dll is empty #9537

Merged

2 tasks

zadjii-msft mentioned this pull request Mar 31, 2021

Create a control unittesting project #9677

Merged

2 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename `Microsoft.Terminal.TerminalControl` to `.Control`; Split into dll & lib #9472

Rename `Microsoft.Terminal.TerminalControl` to `.Control`; Split into dll & lib #9472

zadjii-msft commented Mar 12, 2021 •

edited by DHowett

Loading

DHowett commented Mar 12, 2021

DHowett commented Mar 12, 2021

DHowett commented Mar 12, 2021

zadjii-msft commented Mar 12, 2021

DHowett Mar 12, 2021

zadjii-msft Mar 16, 2021

zadjii-msft Mar 16, 2021

DHowett Mar 12, 2021

zadjii-msft Mar 16, 2021

DHowett Mar 12, 2021

zadjii-msft Mar 16, 2021

DHowett Mar 12, 2021

DHowett left a comment

DHowett commented Mar 17, 2021

DHowett commented Mar 17, 2021

ghost commented Mar 17, 2021

Rename Microsoft.Terminal.TerminalControl to .Control; Split into dll & lib #9472

Rename Microsoft.Terminal.TerminalControl to .Control; Split into dll & lib #9472

Conversation

zadjii-msft commented Mar 12, 2021 • edited by DHowett Loading

Detailed Description of the Pull Request / Additional comments

DHowett commented Mar 12, 2021

DHowett commented Mar 12, 2021

DHowett commented Mar 12, 2021

zadjii-msft commented Mar 12, 2021

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

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Mar 17, 2021

DHowett commented Mar 17, 2021

ghost commented Mar 17, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Rename `Microsoft.Terminal.TerminalControl` to `.Control`; Split into dll & lib #9472

Rename `Microsoft.Terminal.TerminalControl` to `.Control`; Split into dll & lib #9472

zadjii-msft commented Mar 12, 2021 •

edited by DHowett

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.