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

Consider setting environment variable when Terminal hosts cmd / powershell #840

Closed
binarycrusader opened this issue May 16, 2019 · 11 comments · Fixed by #897
Closed

Consider setting environment variable when Terminal hosts cmd / powershell #840

binarycrusader opened this issue May 16, 2019 · 11 comments · Fixed by #897
Assignees
Labels
Area-Interop Communication between processes Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@binarycrusader
Copy link
Member

Windows Terminal has support for unicode character display, etc. that the old cmd / powershell windows do not. This makes it difficult to create a shell profile (such as Microsoft.Powershell_profile.ps1) that works compatibly with both the old cmd / powershell windows and the new Windows Terminal.

If an environment variable was set by Terminal when hosting cmd / powershell / wsl, etc. it would allow users to detect they are hosted by the new Terminal and then enable use of unicode character display, etc. compatibly.

For example, my powershell profile prompt has been altered to include some emojis. In the old powershell window, this happens:

image

In the new Windows Terminal:

image

I could make my profile compatible with both if I had an environment variable to check.

@zadjii-msft
Copy link
Member

This is a good idea.

We've previously had discussions about the terminal setting TERM, but we probably wouldn't want to overload the meaning of TERM=xterm-256colors for just detecting the terminal.

I know there are plans of getting the DxRenderer that the Terminal uses in to conhost properly, once it's bugs are worked out. In that case, the emoji would render correctly. That however would be harder to detect, because I'm sure conhost isn't going to set some variable to conhost-dx (vs conhost-gdi).

@miniksa for his thoughts.

@zadjii-msft zadjii-msft added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label May 16, 2019
@DHowett-MSFT
Copy link
Contributor

I’d like to have this too, actually, but for a different reason.

If you look at Terminal.app and iTerm, they expose a “session” environment variable. It’s a way for a client process to both detect the presence of the terminal emulator and associate session-specific data. This is really useful when it comes to automatic restart, where the shell can be somewhat complicit in resuming where you left off.

Overall, we need something here: we should probably set TERM because that’s the right thing to do, and we should probably identify ourselves via another environment variable.

@miniksa
Copy link
Member

miniksa commented May 16, 2019

Yes, I would like to switch conhost.exe over to the DX renderer by default which would eliminate some of the pressure of this change because it would understand emoji on the other side of the fence too. It's just probably going to be a longer-term thing because switching that over will probably cause a stream of bugs from the OS product that I don't want to deal with right now because I want to focus on the out-of-box GitHub work in the short-term.

However, I am not opposed to setting environment variables to provide some semblance of understanding over the environment that the client application is living within.

I would almost think we would do TERM=xterm-256color because that's what we're striving for and if we can't handle the sequences in that profile, it's a bug at this point.

And then perhaps we do WINTERM=1 or something for now. Basically just the equivalent to TRUE/I am here. And if we want to evolve that later... sure.

@oising
Copy link
Collaborator

oising commented May 16, 2019

Just so y'all know, there is already a WinTerm out there: https://sourceforge.net/projects/winterm/

@miniksa
Copy link
Member

miniksa commented May 16, 2019

Crap. OK. Think of something else.

@binarycrusader
Copy link
Member Author

binarycrusader commented May 16, 2019

I was considering just setting ‘WT_SESSION=<random uuid>‘

@DHowett-MSFT DHowett-MSFT added Product-Terminal The new Windows Terminal. Area-Interop Communication between processes labels May 16, 2019
@binarycrusader
Copy link
Member Author

binarycrusader commented May 17, 2019

Where should this be done? From poking at the code, my best hacky guess is something like this just before the CreateProcessW call in Entrypoints::StartConsoleForCmdLine:

// Set unique session identifier.
GUID sessionGUID;
RETURN_IF_FAILED(CoCreateGuid(&sessionGUID));

WCHAR wszSessionGUID[MAX_PATH]{};
(void)StringFromGUID2(sessionGUID, wszSessionGUID, static_cast<int>(std::size(wszSessionGUID)));
wszSessionGUID[wcslen(wszSessionGUID) - 1] = '\0';

WCHAR* pwszSessionGUID{&wszSessionGUID[1]};
RETURN_IF_WIN32_BOOL_FALSE(SetEnvironmentVariableW(L"WT_SESSION", pwszSessionGUID));

However, I suspect there's a better place to put this. Also, even if that is the right place, it seems wrong somehow to set WT_SESSION directly there. I haven't been able to figure out the execution flow from when a new tab is created to StartConsoleForCmdLine. I also suspect a cleaner implementation here would be to extend ConsoleArguments to include environment variables and have the caller pass it.

I could use some hints on where this is best done.

@miniksa
Copy link
Member

miniksa commented May 17, 2019

ConhostConnection in the TerminalConnection library is more of where each channel is established per tab to the client application (and conhost-in-conpty mode that services it).

You might have the ConhostConnection object generate its own GUID on construction and then pass that as an additional environment variable into the CreateConPty call.

In the conpty-universal.h header, maybe add an optional last parameter that is additional environment information to stitch to the block and if the arg is present, GetEnvironmentStrings to get the current environment block, stitch your new string to the end with the session GUID, and pass that as the lpEnvironment parameter in the CreateProcessW call in there that starts up the ConPTY.

Then since the ConPTY/Conhost won't be otherwise messing with lpEnvironment (it's probably null inside Entrypoints::StartConsoleForCmdLine causing an inherit), the final child shell process will just inherit that down and have it available.

Pinging @zadjii-msft for confirmation that this sounds correct.

@zadjii-msft
Copy link
Member

Yea that sounds right to me

@DHowett-MSFT
Copy link
Contributor

In the future, we'll probably want to support a generic "connection arguments" type that can be pushed down into each connection.

@b-hayes
Copy link

b-hayes commented Dec 10, 2020

I was considering just setting ‘WT_SESSION=‘

Im guessing this was implimented for this purpose as I have seen it and tried using it for this purpose however, if i launch an applicaiton with embeded terminals they are also getting a random WT_SESSION value.

Is there any way to stop nested processes from inheriting this WT_SESSION ?
( I say inherit but the values are actually dirfferent but they shouldnt have it at all ).

eg. to replicate

  1. open windows terminal with ubuntu wsl.
  2. launch IDE (im using PhpStorm64.exe but vscode and others ahve the same result)
  3. open the IDE's embeded terminal
  4. echo $WT_SESSION #shows a session id
  5. open the same IDE from windows start menu
  6. echo $WT_SESSION #no session id

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Interop Communication between processes Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants