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

Duplicate WT_SESSION and WT_PROFILE_ID in WSLENV env var #7130

Open
abarani opened this issue Jul 30, 2020 · 4 comments
Open

Duplicate WT_SESSION and WT_PROFILE_ID in WSLENV env var #7130

abarani opened this issue Jul 30, 2020 · 4 comments
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Milestone

Comments

@abarani
Copy link

abarani commented Jul 30, 2020

Environment

Windows build number: 10.0.19041.388
Windows Terminal version (if applicable): 1.1.2021.0

Steps to reproduce

  • Open Windows Terminal
  • setx WSLENV "${env:WSLENV}:TEST/p"
  • Close and reopen Windows Terminal
  • echo $env:WSLENV

Expected behavior

WT_SESSION:WT_PROFILE_ID:TEST/p

Actual behavior

WT_SESSION:WT_SESSION::WT_PROFILE_ID:TEST/p:WT_PROFILE_ID

I think the right thing to do here is to split WSLENV by : and check those vars are already set instead of just simply prepending and appending a string.

Probable related PRs:

@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 Jul 30, 2020
@DHowett
Copy link
Member

DHowett commented Jul 30, 2020

Good catch! There's an inherent danger in using setx to immortalize the contents of an environment variable at runtime, because even TEST/p is going to get duplicated if you keep doing that ...

I'm not sure it harms anything other than aesthetics, but I'll tag this one up as a code health issue.

@DHowett DHowett added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. 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 Jul 30, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 30, 2020
@DHowett DHowett added this to the Terminal Backlog milestone Jul 30, 2020
@zadjii-msft zadjii-msft modified the milestones: Terminal Backlog, Backlog Jan 4, 2022
@huiyooumich
Copy link
Contributor

Hello, this is my first time making a contribution here! I would like to work on this.

@zadjii-msft
Copy link
Member

zadjii-msft commented Mar 21, 2022

@huiyooumich awesome, go for it! I don't think you'd be stepping on anyone's toes here. I'm pretty sure ConptyConnection::_LaunchAttachedClient in ConptyConnection.cpp is where we're setting up the environment variables. Let us know if you need any pointers ☺️

@huiyooumich
Copy link
Contributor

Unfortunately, I have stopped working on this due to lack of time. But thank you so much for the advice and I hope if anyone else wants to give this a go they'll find it a lot easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

4 participants