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

fix #7973: Collect env vars lowercase on windows #8082

Merged
merged 1 commit into from
Jun 29, 2020
Merged

fix #7973: Collect env vars lowercase on windows #8082

merged 1 commit into from
Jun 29, 2020

Conversation

dschafhauser
Copy link
Contributor

What it does

How to test

  1. Set two environment variables with different casing:
setx var_lower VAR_LOWER_value
setx VAR_UPPER VAR_UPPER_value
  1. Start Theia locally on Windows and add the following tasks.json:
{
    "version": "2.0.0",
    "tasks": [
        {
            "label": "Echo",
            "type": "shell",
            "command": "echo Test var_lower=${env:var_lower} VAR_UPPER=${env:VAR_UPPER}",
            "problemMatcher": []
        }
    ]
}
  1. Execute task "Echo".

Output before fix:

Test var_lower=VAR_LOWER_value VAR_UPPER=

Output after fix:

Test var_lower=VAR_LOWER_value VAR_UPPER=VAR_UPPER_value

Review checklist

Reminder for reviewers

@kittaakos kittaakos self-requested a review June 24, 2020 12:41
@kittaakos
Copy link
Contributor

@dschafhauser, I have added the two new env variables with setx,

Untitled

I started the backend, I added the sample Echo task and executed it but I do not see any of the variables; neither the lower nor the upper case.

image

I can confirm, Objects.keys(process.env).forEach does not contain my two variables. Any other ways to verify it? Thanks!

@vince-fugnitto vince-fugnitto added the OS/Windows issues related to the Windows OS label Jun 25, 2020
@akosyakov
Copy link
Member

@kittaakos Do you need to restart Theia from a new terminal to pick up changes to env variables? I am not sure, maybe your Theia does not have it on startup. You could try to debug changed code and see whether process.env has them.

@akosyakov akosyakov requested a review from lmcbout June 29, 2020 07:39
@akosyakov akosyakov added debug issues that related to debug functionality tasks issues related to the task system labels Jun 29, 2020
@lmcbout
Copy link
Contributor

lmcbout commented Jun 29, 2020

Sorry, I don't have a WINDOW environment to test it, someone else will have to test it

@dschafhauser
Copy link
Contributor Author

dschafhauser commented Jun 29, 2020

I can confirm, Objects.keys(process.env).forEach does not contain my two variables. Any other ways to verify it? Thanks!

I think the following way should work more reliably.

Starting with a fresh CMD:

D:\theia> yarn
D:\theia> yarn run rebuild:electron
D:\theia> cd examples/electron
D:\theia\examples\electron> SET var_lower=VAR_LOWER_value
D:\theia\examples\electron> SET VAR_UPPER=VAR_UPPER_value
D:\theia\examples\electron> yarn run start

Could you try this, @kittaakos ?

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have verified it on Windows; it worked. I did the followings:

C:\Users\kittaakos\dev\git\theia>yarn rebuild:electron
yarn run v1.21.1
$ theia rebuild:electron
Processing @theia/node-pty
Processing nsfw
Processing native-keymap
Processing find-git-repositories
√ Rebuild Complete
Done in 66.52s.

C:\Users\kittaakos\dev\git\theia>SET var_lower=VAR_LOWER_value

C:\Users\kittaakos\dev\git\theia>SET VAR_UPPER=VAR_UPPER_value

C:\Users\kittaakos\dev\git\theia>echo %var_lower%
VAR_LOWER_value

C:\Users\kittaakos\dev\git\theia>echo %VAR_UPPER%
VAR_UPPER_value

C:\Users\kittaakos\dev\git\theia>echo %VAR_LOWER%
VAR_LOWER_value

C:\Users\kittaakos\dev\git\theia>echo %var_upper%
VAR_UPPER_value

C:\Users\kittaakos\dev\git\theia>yarn --cwd examples\\electron start
yarn run v1.21.1

image

@kittaakos
Copy link
Contributor

Does anyone want to check on macOS/Linux?

CHANGELOG.md Outdated Show resolved Hide resolved
- Populate local copy of environment variables with lowercase keys if on
Windows platform
- Fix #7973: Environment variables are not retrieved properly on Windows
- Update changelog

Signed-off-by: David Schafhauser <schafhauser.david@gmail.com>
@akosyakov
Copy link
Member

Changes look good to me too, @kittaakos feel free to merge if the build is green.

@kittaakos kittaakos merged commit 68a3453 into eclipse-theia:master Jun 29, 2020
@kittaakos
Copy link
Contributor

Thank you for the fix, @dschafhauser 👍

@dschafhauser dschafhauser deleted the patch-1 branch July 9, 2020 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality OS/Windows issues related to the Windows OS tasks issues related to the task system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants