Skip to content

Enforcing consistent access to environment variables#2236

Merged
bobbrow merged 4 commits intomicrosoft:mainfrom
lygstate:main
Jan 28, 2022
Merged

Enforcing consistent access to environment variables#2236
bobbrow merged 4 commits intomicrosoft:mainfrom
lygstate:main

Conversation

@lygstate
Copy link
Copy Markdown
Contributor

@lygstate lygstate commented Nov 9, 2021 •
edited by bobbrow
Loading

This change addresses item

Fixes #1592
Replacement of #2025

Fixes #2237

Fixes #1987

This changes

The following changes are proposed:

  • Refactoring environmentVariables.ts out for consistence environment variable handling with NodeJS.ProcessEnv

The purpose of this change

  • Get environment variables on win32 properly, that is we can set/get environment variables in case-insensitive way
  • Add backendTests to the CI, and revise the launch.json for faster debugging utils funcitons
  • NOTE: Future direction, moving tests that's not depends on VSCode into backendTests

@lygstate lygstate force-pushed the main branch 7 times, most recently from 93ed60e to 819081f Compare November 10, 2021 06:58
@btgoodwin btgoodwin mentioned this pull request Nov 10, 2021
Closed
@bobbrow
Copy link
Copy Markdown
Member

bobbrow commented Nov 10, 2021

Thank you for continuing to work on this! I'll try to take a look soon, but I currently have some vacation time planned and other time critical things to complete this month which may make that difficult.

@bobbrow bobbrow added this to the On Deck milestone Nov 10, 2021
@bobbrow bobbrow self-requested a review November 10, 2021 18:39
@lygstate lygstate force-pushed the main branch 4 times, most recently from 78d0737 to 57294f5 Compare November 12, 2021 02:11
@lygstate
Copy link
Copy Markdown
Contributor Author

lygstate commented Nov 18, 2021

@bobbrow Hi, ping for review to avoid conflict with the repository

@bobbrow
Copy link
Copy Markdown
Member

bobbrow commented Nov 24, 2021

I will be looking at this today.

bobbrow
bobbrow requested changes Nov 24, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@bobbrow bobbrow left a comment

Choose a reason for hiding this comment

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

I still need to test it, but here is the initial review.

Comment thread package.json
Comment thread src/cmake-tools.ts Outdated
Comment thread src/cmake-tools.ts
Comment thread src/config.ts Outdated
Comment thread src/drivers/driver.ts Outdated
Comment thread src/preset.ts Outdated
Comment thread src/util.ts Outdated
Comment thread src/util.ts Outdated
Comment thread test/backend-unit-tests/cmake/environment.test.ts Outdated
Comment thread test/helpers/vscodefake/extensioncontext.ts
lygstate
lygstate commented Nov 25, 2021
View reviewed changes
Comment thread src/cmake-tools.ts
lygstate
lygstate commented Nov 25, 2021
View reviewed changes
Comment thread src/cmake-tools.ts
@lygstate lygstate force-pushed the main branch 6 times, most recently from 913028c to 5bcf75d Compare November 29, 2021 20:10
bobbrow
bobbrow approved these changes Dec 20, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@bobbrow bobbrow left a comment

Choose a reason for hiding this comment

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

Approved. After enabling the Windows CI #2295, I will merge with this, run one last test pass, and then commit.

@bobbrow bobbrow modified the milestones: On Deck, 1.10.0 Dec 20, 2021
@bobbrow
Copy link
Copy Markdown
Member

bobbrow commented Dec 27, 2021

One last question. I still see a lot of direct interaction with process.env after this change. Is the goal to make all of those accesses consistent too? In other words, should we keep a global Environment that corresponds to process.env and enforce using that everywhere in the code?

@bobbrow bobbrow changed the title Refactoring environmentVariables.ts out Enforcing consistent access to environment variables Dec 27, 2021
@lygstate
Copy link
Copy Markdown
Contributor Author

lygstate commented Dec 28, 2021

process.env

Hi, there is no need modify process.env, I was using Environment to simulate typeof process.env, they indeed are the same thing after all.

@lygstate
Copy link
Copy Markdown
Contributor Author

lygstate commented Jan 25, 2022

Can this be merged now?

@lygstate lygstate force-pushed the main branch 3 times, most recently from 9784603 to d5608a7 Compare January 25, 2022 08:42
@lygstate
Increase timeout value for fixes tests timeout on win32
6fe161e 
The following error are copied from  https://github.com/microsoft/vscode-cmake-tools/runs/4933169033?check_suite_focus=true
```
2022-01-25T07:38:39.8034093Z �[0m  1) CMake FileAPI driver tests
2022-01-25T07:38:39.8035702Z        All target for Visual Studio Community 2019 - amd64:
2022-01-25T07:38:39.8038322Z �[0m�[31m     Error: Timeout of 60000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (d:\a\vscode-cmake-tools\vscode-cmake-tools\out\test\unit-tests\driver\cmfileapi-driver.test.js)�[0m�[90m
2022-01-25T07:38:39.8040365Z   	at listOnTimeout (internal/timers.js:554:17)
2022-01-25T07:38:39.8041995Z   	at processTimers (internal/timers.js:497:7)
2022-01-25T07:38:39.8043420Z �[0m
2022-01-25T07:38:39.8044865Z �[0m  2) Kits scan test
2022-01-25T07:38:39.8058046Z        Detect system kits never throws:
2022-01-25T07:38:39.8061281Z �[0m�[31m     Error: Timeout of 120000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (d:\a\vscode-cmake-tools\vscode-cmake-tools\out\test\unit-tests\kit-scan.test.js)�[0m�[90m
2022-01-25T07:38:39.8063425Z   	at listOnTimeout (internal/timers.js:554:17)
2022-01-25T07:38:39.8065171Z   	at processTimers (internal/timers.js:497:7)
```

Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
@lygstate
Format kit.ts
9683c6d 
Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
@lygstate
Refactoring environmentVariables.ts out for set get environment varia…
420c266 
…bles on win32 properly

Fixes microsoft#1592

Fixes debug launch when user didn't configure the debug environment

Fixes microsoft#2237

Getting Backend tests no need to tsc -p

And testing environment variables properly

runCompileCommand use getCMakeBuildCommandEnvironment

Getting getCMakeBuildCommandEnvironment to be consistence with getCTestCommandEnvironment and getConfigureEnvironment

Rename HardEnv to Environment

Convert expandString to template.

Add tests for nodejs util.inspect

Calling to fixPaths properly

Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
@bobbrow
Copy link
Copy Markdown
Member

bobbrow commented Jan 25, 2022

@andreeis was almost done with the validation and then had to take some sick leave. This will go in soon. Thank you for your patience.

@bobbrow bobbrow merged commit 56db1ba into microsoft:main Jan 28, 2022
@bobbrow
Copy link
Copy Markdown
Member

bobbrow commented Jan 28, 2022

I merged it. We will continue to test as we have time, but I believe it will be good if future development already has this change in as it will add additional coverage.

@github-actions github-actions Bot locked and limited conversation to collaborators Mar 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bobbrow bobbrow bobbrow approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.10.0

2 participants

@lygstate @bobbrow