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

Terminal Preview 1.3 breaks Windows PowerShell 5 (32-bit x86)'s history save and view and its command line colors #7418

Closed
metathinker opened this issue Aug 26, 2020 · 17 comments · Fixed by #12140
Assignees
Labels
Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Powershell Issues in the modern command interpreter, Powershell.exe. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@metathinker
Copy link
Contributor

metathinker commented Aug 26, 2020

Environment

Windows build number: Microsoft Windows [Version 10.0.19042.422] for x64
Windows Terminal version (if applicable): 1.3.2382.0

Using the default Terminal settings.json file and without any PowerShell profile files.

Steps to reproduce

  1. Make sure that Windows Terminal Preview is registered to be run as wt.exe in Settings -> Apps -> Apps and features -> App execution aliases, and log out and log in if needed to make the setting fully take effect.
  2. Run this command from Start -> Run: wt.exe "C:\Windows\SysWOW64\WindowsPowerShell\v1.0\powershell.exe"
  3. Type get-help gv -ful with one L, press Tab.
  4. Observe get-help gv -Full with 2 letters L, and the colors of that string.
  5. Press Enter to run the command.
  6. Exit Terminal and restart it using the same command you previously used in Start -> Run.
  7. Press the up arrow key repeatedly.

Expected behavior

  • In the get-help command you ran, get-help was in yellow and -Full was in a darker shade of gray than the other text.
  • After relaunching Terminal, you could use the up arrow key to see your command history, including the get-help command you just ran.

Actual behavior

  • In the get-help command, all the text was the same color.
  • The command history was not accessible.
  • If you now run PowerShell (x86) using the old console window, you see that the get-help command and other commands you ran in PowerShell within Terminal were not recorded in the history.
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 26, 2020
@DHowett
Copy link
Member

DHowett commented Aug 26, 2020

This is absolutely wild. PSModulePath contains the unexpanded token, %ProgramFiles%, which gets either inherited fully-expanded (from Explorer or pre-1.3 WT) or inherited unexpanded (post-1.3 WT) and expanded at CreateProcess-time.

It looks like PowerShell (x86) has been working thanks to a fluke this whole time. Neat!

@paulomorgado
Copy link

The PSReadline module can't be found on 32-bit Windows PowerShell on Windows Terminal 1.3.

get-module -l | measure gives me a count of 143 on Windows Terminal 1.3 and 497 on Winodes Terminal 1.2 for PowerShell 32-bit.

On Windows Terminal 1.2 get-module -l shows modules from ´C:\Program Files\WindowsPowerShell\Modules` and C:\Program Files (x86)\WindowsPowerShell\Modules\ and on Windows Terminal 1.3 it only shows modules from C:\Program Files (x86)\WindowsPowerShell\Modules\.

@paulomorgado
Copy link

If I explicitly import C:\Program Files\WindowsPowerShell\Modules\PSReadline\2.0.0\PSReadLine.psm1, it imports without any problems.

@zadjii-msft zadjii-msft added this to the Terminal v1.4 milestone Aug 27, 2020
@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. Priority-1 A description (P1) labels Aug 27, 2020
@DHowett
Copy link
Member

DHowett commented Aug 27, 2020

It's very curious that PowerShell ships every module in both places except PSReadline. It is additionally concerning that this has simply worked forever (except perhaps there have been maybe ten users using weird configurations who have gotten the 32-bit module path and written it off as a fluke.)

We can fix this by expanding the environment variables in our newly-minted block before passing them off, but ... I'd like to understand how PowerShell expected this to work 😄

@zadjii-msft
Copy link
Member

Summoning @SteveL-MSFT - Do you have any idea why PSReadline is the special case here?

@zadjii-msft zadjii-msft added Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Powershell Issues in the modern command interpreter, Powershell.exe. labels Aug 28, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 28, 2020
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 28, 2020
@metathinker metathinker changed the title Terminal Preview 1.3 breaks Windows PowerShell 5 (32-bit x86)'s history save and view and command line colors Terminal Preview 1.3 breaks Windows PowerShell 5 (32-bit x86)'s history save and view and its command line colors Sep 1, 2020
@DHowett
Copy link
Member

DHowett commented Sep 21, 2020

I'm reverting the environment fix for 1.3's move to stable this week because we do not otherwise have a good solution for this.

DHowett added a commit that referenced this issue May 24, 2021
Revert "Fix environment block creation (#7401)"

This reverts commit 7886f16.

(cherry picked from commit e46ba65)

Revert "Always create a new environment block before we spawn a process (#7243)"

This reverts commit 849243a.

References #7418

(cherry picked from commit 4204d25)
(cherry picked from commit f8e8572)
DHowett added a commit that referenced this issue Jul 12, 2021
Revert "Fix environment block creation (#7401)"

This reverts commit 7886f16.

(cherry picked from commit e46ba65)

Revert "Always create a new environment block before we spawn a process (#7243)"

This reverts commit 849243a.

References #7418

(cherry picked from commit 4204d25)
(cherry picked from commit f8e8572)
(cherry picked from commit cb4c4f7)
DHowett added a commit that referenced this issue Aug 26, 2021
Revert "Fix environment block creation (#7401)"

This reverts commit 7886f16.

(cherry picked from commit e46ba65)

Revert "Always create a new environment block before we spawn a process (#7243)"

This reverts commit 849243a.

References #7418

(cherry picked from commit 4204d25)
(cherry picked from commit f8e8572)
(cherry picked from commit cb4c4f7)
(cherry picked from commit afb0cac)
DHowett added a commit that referenced this issue Oct 11, 2021
Revert "Fix environment block creation (#7401)"

This reverts commit 7886f16.

(cherry picked from commit e46ba65)

Revert "Always create a new environment block before we spawn a process (#7243)"

This reverts commit 849243a.

References #7418

(cherry picked from commit 4204d25)
(cherry picked from commit f8e8572)
(cherry picked from commit cb4c4f7)
(cherry picked from commit afb0cac)
(cherry picked from commit b25dc74)
@iSazonov
Copy link

iSazonov commented Dec 1, 2021

We can fix this by expanding the environment variables in our newly-minted block before passing them off, but ... I'd like to understand how PowerShell expected this to work 😄

Windows PowerShell (and pwsh too but code was a bit updated) expands env variables in PSModulePath internally.
I can find some GetModulePath methods here https://github.com/PowerShell/PowerShell/blob/c748652c34c3e208fad4d9b8abcf061b7f371c83/src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs#L731

So Windows Terminal has no need to expand env variables. I guess there was a bug in commit reverted.

@DHowett
Copy link
Member

DHowett commented Dec 1, 2021

So Windows Terminal has no need to expand env variables.

Unfortunately, Terminal launches many, many things that are not PowerShell.

In general, though: Terminal is not expanding environment variables. It is using the system API¹ to generate a new environment block, which it then passes to the newly-spawned application.

On launch with a new system environment block, PowerShell x86 receives a module path that contains %ProgramFiles%. It then expands it, as you said. It is incorrect to do so: Powershell x86's module folder (Program Files (x86)\...) does not contain PSReadline.

When we use the old environment block, that value was already expanded, to C:\Program Files. That directory does contain PSReadline. Therefore, PowerShell x86 works fine.

¹ The public one, which is clearly incorrect.

@iSazonov
Copy link

iSazonov commented Dec 2, 2021

@DHowett To be clear, PowerShell modify PSModulePath internally and write back to the process env block.
If we look in cmd.exe x64:

PSModulePath=C:\Program Files\WindowsPowerShell\Modules\;C:\WINDOWS\system32\WindowsPowerShell\v1.0\Modules;

If we look in cmd.exe x32:

PSModulePath=C:\Program Files\WindowsPowerShell\Modules\;C:\WINDOWS\system32\WindowsPowerShell\v1.0\Modules;

If we look in Windows PowerShell x64

C:\Program Files\WindowsPowerShell\Modules\;C:\WINDOWS\system32\WindowsPowerShell\v1.0\Modules;;C:\Program Files\WindowsPowerShell\Modules;C:\WINDOWS\system32\WindowsPowerShell\v1.0\Modules

If we look in Windows PowerShell x32

C:\Program Files\WindowsPowerShell\Modules\;C:\Program Files (x86)\WindowsPowerShell\Modules;C:\WINDOWS\system32\WindowsPowerShell\v1.0\Modules;

As you can see, PowerShell add new paths before old ones.

You could run "broken" Windows Terminal version, start Windows PowerShell and run $env:PSModulePath to see real paths are used.
I guess you will see wrong result. It would be interesting to see the result.

@DHowett
Copy link
Member

DHowett commented Dec 2, 2021

Indeed-

Here's my PSModulePath:

image

Here's what CMD Says:

image

and of course, here's what PowerShell x86 says:

image

And for good measure:

image

Anyway, that's the central thesis of this issue. When PowerShell x86 expands ProgramFiles, it gets the x86 program files. That's totally by design.

When PowerShell (x86) is launched from Explorer, it never sees %ProgramFiles%. It was already expanded by the time it was launched.

When PowerShell (x86) is launched from Task Manager it has the same problem as Terminal (!).


As noted on Aug 26, 2020... "PowerShell x86 has been working properly due to a fluke this whole time"

@iSazonov
Copy link

iSazonov commented Dec 3, 2021

When PowerShell (x86) is launched from Task Manager it has the same problem as Terminal (!).

Ah! Thanks! Now I can reproduce. Although it has nothing to do with this issue at all :-))))) This is the rare case when a simple problem suddenly cloudes our understanding.

What is the user really ask in the issue? - "Why I can not load x86 dll in x64 process?"
Answer: because it is fundamentally impossible!

Should I add more words? :-) Ok!

PS modules can contain dll-s.
The dll-s can be x64 and only right place for them is %Program Files%.
The dll-s can be x86 and only right place for them is %Program Files x86%.
PS x64 must load dll-s from %Program Files% and must not from %Program Files x86%.
PS x32 must load dll-s from %Program Files x86% and must not from %Program Files%.

So for the issue answer should be:
It is neither WT bug nor PowerShell bug. It is by-design. Users must install PSReadline module from PS x86 process to get it work. It is true for other (x86 or platform independent) modules too.

PS: pwsh works the same.

@DHowett
Copy link
Member

DHowett commented Dec 3, 2021

Ah, but here's the catch!

Open up a working x86 PowerShell and ask it where PSReadline has been loaded from.

PSReadline working on x86

PSReadline is an architecture-agnostic .NET assembly with no native PAL DLL!

When x86 PowerShell accidentally loads the "x64" PowerShell's PSReadline, it works properly. That's how the issue of the module path expansion has been hiding this whole time.

@iSazonov
Copy link

iSazonov commented Dec 3, 2021

PSReadline is an architecture-agnostic .NET assembly with no native PAL DLL!

Yes, most modules are like that. I don't even know if we can find a module that is x86 :-) Now everything is x64. So we shouldn't even ask PS x86 to be smarter.

@iSazonov
Copy link

iSazonov commented Dec 3, 2021

And I think you can bring back the reverted commit so that we get more useful feature than that PS x86 by-design issue workaround.

@DHowett
Copy link
Member

DHowett commented Dec 3, 2021

And I think you can bring back the reverted commit so that we get more useful feature than that PS x86 by-design issue workaround.

Unfortunately, any shell breaking when launched by Terminal when it doesn't break when launched by Explorer (Start Menu, Run dialog, desktop shortcut) is an unacceptable failure case for us! We want to make the console experience on Windows better, not worse. 😄

Like... if it didn't break the first thing a user would do with the shell (like, type a command...) I would be more forgiving. But no- we need to do our best to make the out of box experience perfect.

@iSazonov
Copy link

iSazonov commented Dec 3, 2021

I see your point. But:

  • using PS x86 is an edge case today - a small number of users have to use it.
  • it is exclusively PS x86 issue
  • the issue has clear and simple fix - copy PSReadline folder :-)

You could add a workaround in WT itself (run by cmd.exe?).

But the main thing is that you give up a really useful feature that is needed by a large number of users.

@zadjii-msft zadjii-msft modified the milestones: Terminal v2.0, 22H1 Jan 4, 2022
miniksa pushed a commit that referenced this issue Jan 10, 2022
Revert "Fix environment block creation (#7401)"

This reverts commit 7886f16.

(cherry picked from commit e46ba65)

Revert "Always create a new environment block before we spawn a process (#7243)"

This reverts commit 849243a.

References #7418

(cherry picked from commit 4204d25)
(cherry picked from commit f8e8572)
(cherry picked from commit cb4c4f7)
(cherry picked from commit afb0cac)
(cherry picked from commit b25dc74)
DHowett added a commit that referenced this issue Jan 11, 2022
Revert "Fix environment block creation (#7401)"

This reverts commit 7886f16.

(cherry picked from commit e46ba65)

Revert "Always create a new environment block before we spawn a process (#7243)"

This reverts commit 849243a.

References #7418

(cherry picked from commit 4204d25)
(cherry picked from commit f8e8572)
(cherry picked from commit cb4c4f7)
(cherry picked from commit afb0cac)
(cherry picked from commit b25dc74)
(cherry picked from commit 5f7c66b)
@ghost ghost added the In-PR This issue has a related PR label Jan 11, 2022
@ghost ghost closed this as completed in #12140 Jan 12, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jan 12, 2022
ghost pushed a commit that referenced this issue Jan 12, 2022
Revert "Fix environment block creation (#7401)"

This reverts commit 7886f16.

(cherry picked from commit e46ba65)

Revert "Always create a new environment block before we spawn a process (#7243)"

This reverts commit 849243a.

(cherry picked from commit 4204d25)
(cherry picked from commit f8e8572)
(cherry picked from commit cb4c4f7)
(cherry picked from commit afb0cac)
(cherry picked from commit b25dc74)
(cherry picked from commit 5f7c66b)

Fixes #7418
@ghost
Copy link

ghost commented Feb 3, 2022

🎉This issue was addressed in #12140, which has now been successfully released as Windows Terminal Preview v1.13.10336.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Powershell Issues in the modern command interpreter, Powershell.exe. 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.

7 participants
@DHowett @paulomorgado @metathinker @zadjii-msft @iSazonov @cinnamon-msft and others