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

Merge PublicTerminalCore into TermControl #15992

Merged
merged 13 commits into from
Sep 20, 2023
Merged

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Sep 18, 2023

This pull request moves HwndTerminal into Microsoft.Terminal.Control.Lib and removes PublicTerminalCore completely.

Microsoft.Terminal.Control.dll now exports the C API from HwndTerminal.

This adds ~100kb to Microsoft.Terminal.Control.dll and ~1400kb to the WPF package (per architecture) but with the coming interactivity platform merge it's going to benefit us big time.

@DHowett
Copy link
Member Author

DHowett commented Sep 18, 2023

TODO:

  • Apply a hack to PGO that allows us to PGO the ABI DLL using the same databases as the non-ABI version (yeah!)

@github-actions

This comment has been minimized.

@DHowett
Copy link
Member Author

DHowett commented Sep 18, 2023

I bet this is going to lose us Audit Mode coverage over HwndTerminal (boo)

<!--LATE LATE LATE-->
<ItemDefinitionGroup>
<Link>
<AdditionalDependencies>delayimp.lib;uiautomationcore.lib;oleaut32.lib;onecoreuap.lib;%(AdditionalDependencies)</AdditionalDependencies>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i can undo this one!

OpenConsole.sln Outdated
@@ -239,7 +239,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Shared", "Shared", "{89CDCC
EndProject
Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "Types.Unit.Tests", "src\types\ut_types\Types.Unit.Tests.vcxproj", "{34DE34D3-1CD6-4EE3-8BD9-A26B5B27EC73}"
EndProject
Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "PublicTerminalCore", "src\cascadia\PublicTerminalCore\PublicTerminalCore.vcxproj", "{84848BFA-931D-42CE-9ADF-01EE54DE7890}"
Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "Microsoft.Terminal.Control.ABI", "src\cascadia\TerminalControl\dll_abi\Microsoft.Terminal.Control.ABI.vcxproj", "{84848BFA-931D-42CE-9ADF-01EE54DE7890}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sold on the naming... Isn't WinRT also an ABI?

Oh btw... Just wondering... How large would be a Microsoft.Terminal.Control.dll that contains both, the WinRT and C API? (= 1 binary = simpler shipping?) Did you check that / have an estimate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't give a single **** about the filename, I just wanted something to put on disk.

It's 2MB. It would be simpler, and it is how I have been prototyping it.

However: It costs the 150MM+ Terminal users in package size, and it costs the multiple millions of Visual Studio users in on-disk install size. It might be worth a minor amount of complication to save that much disk and transit in aggregate.

Oh, and we can use different linker flags for different versions - use onecoreuap for one, delayload for another, etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it stands with HwndTerminal being 100% divergent on the Core and Interactivity layer:

File Size
Terminal.Control.ABI.dll 637952
Terminal.Control.dll 2077696
Terminal.Control.dll plus ABI exports 2100224

I don't have numbers for HwndTerminal + Interactivity platform, but at last check the DLL was like 1200k?

Now that I see the change in Terminal.Control, I no longer think the separate DLL thing is worth doing ;P

the WPF control can safely get larger. VS already isn't small :P

@DHowett DHowett force-pushed the dev/duhowett/move-ptc-into-tc branch from e9d4032 to d58c849 Compare September 18, 2023 18:29
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait I love this. It's all just M.T.Control.dll? That's genius

And VS can just link to this straight up, and not need to worry about pulling in like, WUX.dll or idk some weird CRT thing?

I think this is fine for 100KB that will ultimately be reduced as we further merge the layers

@DHowett
Copy link
Member Author

DHowett commented Sep 20, 2023

Thanks for the reviews! I'll edit the PR body before merging to actually explain what it does, since it diverged a bit.

@DHowett DHowett enabled auto-merge (squash) September 20, 2023 14:59
@DHowett DHowett enabled auto-merge (squash) September 20, 2023 14:59
@DHowett DHowett changed the title Merge PublicTerminalCore into the Control lib Merge PublicTerminalCore into TermControl Sep 20, 2023
@DHowett DHowett enabled auto-merge (squash) September 20, 2023 15:00
@DHowett DHowett merged commit 059f770 into main Sep 20, 2023
15 of 17 checks passed
@DHowett DHowett deleted the dev/duhowett/move-ptc-into-tc branch September 20, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants