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.

Sign up for GitHub

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

Jump to bottom

Enforcing consistent access to environment variables #2236

Merged
merged 4 commits into from
Jan 28, 2022
Merged

Enforcing consistent access to environment variables #2236

merged 4 commits into from
Jan 28, 2022

Conversation

lygstate
Copy link
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

@btgoodwin btgoodwin mentioned this pull request Nov 10, 2021
Closed
@bobbrow
Copy link
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
Copy link
Contributor Author

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

@bobbrow
Copy link
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
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.

package.json Outdated Show resolved Hide resolved
src/cmake-tools.ts Outdated Show resolved Hide resolved
src/cmake-tools.ts Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/drivers/driver.ts Outdated Show resolved Hide resolved
src/preset.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
test/backend-unit-tests/cmake/environment.test.ts Outdated Show resolved Hide resolved
test/helpers/vscodefake/extensioncontext.ts Outdated Show resolved Hide resolved
bobbrow
bobbrow approved these changes Dec 20, 2021
View reviewed changes
Copy link
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 removed this from the On Deck milestone Dec 20, 2021
@bobbrow bobbrow added this to the 1.10.0 milestone Dec 20, 2021
@bobbrow
Copy link
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
Contributor Author

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
Contributor Author

Can this be merged now?

@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 <[email protected]>
@lygstate
Format kit.ts
9683c6d 
Signed-off-by: Yonggang Luo <[email protected]>
@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 <[email protected]>
@bobbrow
Copy link
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
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