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

Order of environment variables different in WT #9233

Closed
vefatica opened this issue Feb 20, 2021 · 12 comments · Fixed by #15082
Closed

Order of environment variables different in WT #9233

vefatica opened this issue Feb 20, 2021 · 12 comments · Fixed by #15082
Labels
Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Milestone

Comments

@vefatica
Copy link

Environment

Microsoft Windows 10 Pro for Workstations
10.0.19042.804 (2009, 20H2)
Windows Terminal Preview
Version: 1.6.10412.0

Windows build number: [run `[Environment]::OSVersion` for powershell, or `ver` for cmd]
Windows Terminal version (if applicable):

Any other software?

Steps to reproduce

See below.

Expected behavior

Same behavior whether in WT or not

Actual behavior

Behavior different and undesirable when in WT.

In my three Windows shells (CMD, Powershell, TCC), when running in WT

  1. the environment block is ordered differently, and
  2. new environment variables seem to go in the wrong place

Here's a new CMD.EXE running in a Windows console; this is expected behavior.

image

And here's a new CMD.EXE running in Windows Terminal. Note that the variables __VIP (from the user environment) and WT (newly created) are out of place.

image

@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 Feb 20, 2021
@DHowett
Copy link
Member

DHowett commented Feb 20, 2021

I do not believe that the order of environment variables is a contract anybody needs to uphold. WT requests that the system generate a new environment block for every new subprocess, so this is the order returned by the system.

@DHowett DHowett closed this as completed Feb 20, 2021
@DHowett DHowett added the Resolution-Won't-Fix We're just really obstinate about this. There's probably a good reason. label Feb 20, 2021
@DHowett
Copy link
Member

DHowett commented Feb 20, 2021

I'm marking this "Won't Fix". If you can provide a reason why this matters to any software, I'll be happy to reopen it 😄

@eryksun
Copy link

eryksun commented Feb 20, 2021

See Changing Environment Variables:

All strings in the environment block must be sorted alphabetically by name. The sort is case-insensitive, Unicode order, without regard to locale. Because the equal sign is a separator, it must not be used in the name of an environment variable.

One aspect left unstated by the above quote is that the case-insensitive comparison converts to uppercase. For example, 'A' (ordinal 65) and 'a' (ordinal 97) should sort before '_' (ordinal 95).

An incorrect order may cause GetEnvironmentVariableW and SetEnvironmentVariableW to misbehave, if they assume a particular sorted order when searching for and inserting variables. Hopefully they're more robust than that nowadays.

Windows Terminal calls _wcsicmp, which is locale based (limited to ASCII A-Z in the "C" locale) and converts to lowercase. The expected sort order is possible using CompareStringOrdinal.

@eryksun
Copy link

eryksun commented Feb 20, 2021

so this is the order returned by the system.

To be clear, it is not the order that CreateEnvironmentBlock returns. As shown by vefatica, it is also not the order used by SetEnvironmentVariableW, which inserts WT before __VIP in accordance with the expected uppercase, non-linguistic sort order.

@vefatica
Copy link
Author

The behavior I mentioned seems to conflict with the documentation mentioned by @eryksun.

I don't know about upholding contracts, but what about tradition ... programs working as thay have for decades?

This just plain looks bad.

image

As for

why this matters

it makes environment variables hard to find when they're not where they're expected to be (and where they belong) in the list.

@vefatica
Copy link
Author

I, too, have confirmed (as @eryksun alluded to) CreateEnvironmentBlock() puts __VIP at the end.

@vefatica
Copy link
Author

Please reopen this issue.

@DHowett
Copy link
Member

DHowett commented Feb 20, 2021

Huh, wild.

@DHowett DHowett reopened this Feb 20, 2021
@vefatica
Copy link
Author

Thank you!

@DHowett DHowett removed the Resolution-Won't-Fix We're just really obstinate about this. There's probably a good reason. label Feb 22, 2021
@zadjii-msft zadjii-msft added Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Feb 22, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Feb 22, 2021
@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone Feb 22, 2021
@zadjii-msft zadjii-msft modified the milestones: Terminal v2.0, 22H2 Jan 4, 2022
@zadjii-msft
Copy link
Member

This might've been fixed by #14999. We should double check. Maybe not, if we're still just dumping into an unordered map and then dumping back out.

Actually yea I'm gonna reckon it probably wasn't.

@ianjoneill
Copy link
Contributor

This is fixed by #15082

image

@ianjoneill
Copy link
Contributor

This might've been fixed by #14999. We should double check. Maybe not, if we're still just dumping into an unordered map and then dumping back out.

Actually yea I'm gonna reckon it probably wasn't.

The map was ordered, it was just ordered wrongly 🙂

@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Apr 3, 2023
DHowett pushed a commit that referenced this issue Apr 11, 2023
Existing environment variables can be referenced by enclosing the name
in percent characters (e.g. `%PATH%`).

Resurrects #9287 by @christapley.

Tests added and manually tested.

Closes #2785
Closes #9233

Co-authored-by: Chris Tapley <chris.tapley.81@gmail.com>
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Apr 11, 2023
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 Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants