Conversation
…ng an EXE. Needs a relocatable DLL.
Includes cherry-pick of Mike's changes to let intellisense work
Also do a whole bunch of stuff to COMify the console handoff.
…. it finally works with the correct ndr version.... still need to manually patch that. also need to dump dummy call to interface. also fixed arg parser sort of.
…tabs are made for embedding... and it totally works. Except Terminal can't read from the pipes it was given. So gotta figure that one out.
…rsion explicitly.
carlos-zamora
left a comment
There was a problem hiding this comment.
Don't forget to put the copyright headers on some of these (figured this would be easier than putting the same comment over and over again).
Overall, looks good though.
| } | ||
|
|
||
| [[nodiscard]] HRESULT ConsoleCreateIoThreadLegacy(_In_ HANDLE Server, const ConsoleArguments* const args) | ||
| HRESULT ConsoleCreateIoThread(_In_ HANDLE Server, |
There was a problem hiding this comment.
Adding method comments would be nice for this file.
There was a problem hiding this comment.
I mean, I know the discussion is like 6 months gone by at this point, but doc comments really would help now that we're gonna be checking this in for real
… out of proc com server handling a bit for the exe server delegation conhost.
This comment has been minimized.
This comment has been minimized.
…. add a state check for the callback for handoff.
DHowett
left a comment
There was a problem hiding this comment.
42/50, i've reviewed the terminal side. Blocking over the discussion about HANDLE on the internal interface w/ or w/out the casting
DHowett
left a comment
There was a problem hiding this comment.
50/50. I'll sign off after the earlier comments!
| @@ -0,0 +1,334 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
ugh, nobody can tell that this file was renamed so it's going to become a real pain to un-merge-conflict when Dragos' changes come back out. It's also hard to review.
This is not a complaint about your work. This is the mechanics of git!
There was a problem hiding this comment.
for easier reviewing, check out this branch and...
git diff origin/main:src/host/exemain.cpp HEAD:src/host/exe/exemain.cpp
|
🎉 |
Stories like these are why I browse open source, good luck with the rest of the fire.😅 |
By default from ARM64 architecture projects, `WIN32` is not defined. It is supposed to be for this proxy stub to work. So I've set it with the preprocessor for this project. ## PR Checklist * [x] Closes release build failure after #7489 * [x] I work here. * [x] Built on my machine. ## Detailed Description of the Pull Request / Additional comments `WIN32` appears to convey two meanings depending on who you are: - To most of Windows, `WIN32` appears to mean the Win32 API surface and sometimes the major OS version that goes with it. (Specifically in contrast to 16-bit Windows.) - To others, `WIN32` appears to mean a 32-bit processor or a synonym of `x86`. This is generally not a problem for a few reasons: - VS defines `WIN32` in the default targets/props only for the `x86` processor type. **BUT** - Windows defines `WIN32` if it's not already defined in both `minwinbase.h` and `ole2.h` which generally speaking manage to get compiled into practically everything especially since `minwinbase.h` tends to sneak itself in somehow through `windows.h` and that's **THE** include to use the Windows API surface. - Windows also defines `WIN32` for itself unconditionally and relatively globally when building itself. However, it's a problem here because: - `rpcproxy.h` is the only header included in `dlldata.c`, a file generated automatically by `midl.exe` in the SDK when making a proxy stub. - `rpcproxy.h` only defines its contents for a proxy when `WIN32` or `_M_AMD64` are found. - Therefore, it's defined pretty naturally for x86 and AMD64 targets from VS, but not for ARM64. - ARM64 support is pretty new and those who are attempting to build for ARM64 and against the public SDK with Visual Studio for a classic COM proxy... seems like a relatively unlikely combination. I will follow up with the Visual Studio, Windows SDK, and MIDL/COM teams to try to remove this pitfall from the public tooling. But for now, this is the fix.
Broadly, the tests were broken by #7489 because there were no _startupActions. They relied on the removed codepath that assumed wt.exe always set actions, or `AppCommandlineArgs::ValidateStartupCommands` created one by default. These tests are still broken: * `TerminalAppLocalTests::TabTests::TryZoomPane [Failed]` * `TerminalAppLocalTests::TabTests::MoveFocusFromZoomedPane [Failed]` * `TerminalAppLocalTests::TabTests::CloseZoomedPane [Failed]` Re:#9659
There are a couple places where we now bail immediately on startup, if we think the window is going to get created without any tabs. We do that to prevent a blank window from flashing on the screen when launching auto-elevate profiles. Unfortunately, those broke defterm in a particularly hard to debug way. In the defterm invocation, there actually aren't any tabs when the app completes initialization. We use the initialization to actually accept the defterm handoff. So what would happen is that the window would immediately close itself gracefully, never accepting the handoff. In my defense, #8514, the original auto-elevated PR, predates defterm merging (906edf7) by a few months, so I totally forgot to test this when rolling it into the subsequent iterations of that PR. * Related to: * #7489 * #12137 * #12205 * [x] Closes #12267 * [x] I work here * [ ] No tests on this code unfortunately * [x] Tested manually Includes a semi-related code fix to #10922 to make that quieter. That is perpetually noisy, and when trying to debug defterm, you've only got about 30s to do that before it bails, so the `sxe eh` breaks in there are quite annoying.

References
PR Checklist
Detailed Description of the Pull Request / Additional comments
conhost.exepropsheet or with the registry keys. Finishing the setting inside Windows Terminal will be a todo after this is complete. The OS Settings panel work to surface this setting is a dependency delivered by another team and you will not see it here.Validation Steps Performed