Using consistence function for environment set and get to fix the gap between windows and unix. #1763

lygstate · 2021-04-13T17:48:50Z

Signed-off-by: Yonggang Luo [email protected]

bobbrow · 2021-04-13T18:38:40Z

Thanks @lygstate! We'll take a look at this for 1.8.

elahehrashedi · 2021-07-22T19:54:17Z

src/drivers/driver.ts

-    );
-    return env;
-  }
+  private _kitEnvironmentVariables: proc.EnvironmentVariables = {};


UPDATE: We just updated the coding guidelines. You don't have to follow the new guideline for this PR. We will start applying it for the upcoming PRs :)
....
the plan is not to use _ anymore, I know you didn't name this property, but as you are touching it, that would be good to rename it at the end.
That would be great to apply the new naming guidelines for the rest of the PR as well: https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines

Sorry, I touched too much variable with leading _, I think it's better to apply the naming guidelines with a single short, maybe by the lint rules, otherwise it's would get the code hard to review

You are right, it's easier if we start applying new guidelines in future PRs.

elahehrashedi · 2021-07-22T21:42:21Z

src/drivers/driver.ts

  }

  /**
   * Get the environment variables that should be set at CMake-build time.
   */
  async getCMakeBuildCommandEnvironment(in_env: proc.EnvironmentVariables): Promise<proc.EnvironmentVariables> {
-    let envs: proc.EnvironmentVariables;
+    const envs_to_merge: (proc.EnvironmentVariables | undefined)[] = [in_env];


Thank you for your contribution.
Would you please provide some explanation on why mergeEnvironmentWithExpand is needed here?
There is a mergeEnvironment and splitEnvironmentVars and computeExpandedEnvironment, and it is a little confusing having another function. We can add more comments/samples like what you did for mergeEnvironmentWithExpand to add some clarity, or choose some more clear names.

/** * mergeEnvironmentWithExpand will merge new env variable and * expand it one by one, so that the environment like this * will expand properly: * config.environment * { PATH:`${env.PATH};C:\\MinGW\\bin` } * config.configureEnvironment * { PATH:`${env.PATH};C:\\OtherPath\\bin` } * * @param intermediateExpand If this is the intermediate expanding * procedure or the final expanding procedure, if it's the * intermediate expanding procedure, the variable that not found * won't not expaned, if it's the final expanding procedure, the variable * that not found would expand to ''. * @param envs The list of environment variables to expand * @param opts Environment expand options * * NOTE: By mergeEnvironment one by one to enable expanding self * containd variable such as PATH properly. If configureEnvironment * and environment both configured different PATH, doing this will * preserve them all */ export async function mergeEnvironmentWithExpand( intermediateExpand: boolean, envs: (EnvironmentVariables | undefined)[], opts?: ExpansionOptions)

I've document it in mergeEnvironmentWithExpand, don't know if the function name is proper, looking for better name

I appreciate if you could add some comments/samples to mergeEnvironment and splitEnvironmentVars and computeExpandedEnvironment, similar to what you did for mergeEnvironmentWithExpand to add some clarity.

I didn't touch and use of splitEnvironmentVars, other functions are documented

src/proc.ts

gap between windows and unix. Directly use configureEnv, as now getConfigureEnvironment would not return environment with undefined/null value Also expand with environment variable when get effectiveKitEnvironment Fixes microsoft#1592 preset using the same environment variable merge strategy like kits does Using GeneralEnvironmentType Signed-off-by: Yonggang Luo <[email protected]>

lygstate · 2021-07-30T04:25:11Z

Hope this can be merged in 1.8, as it fixes a lot of issues

bobbrow

The names of these new functions and the way we access the environment seem a little weird now. I wouldn't think to go to util to get environment values. I think it would be better if we had an environment or env module that managed all of this.

Accessing the process.env directly should be ok. process.env looks up environment variables in a case-insensitive way on Windows which should be faster than the new function. The real problem is with the proc.EnvironmentVariables type which does not do the same thing. There is also new expansion behavior added that seems it should be in a separate PR. I would prefer for this PR to scale back the changes and focus on the problem with accessing variables in proc.EnvironmentVariables. I worry that the changes proposed are risky for 1.8 given that we want to release it soon.

bobbrow · 2021-07-30T16:20:38Z

src/util.ts

+ * @param expectKey The environment key
+ * @return [key, value] pair of the found environment item or undefined when not found
+ */
+export function envGet(


General comment for function signatures is to put all the arguments and return type on the same line if they fit comfortably. It looks inconsistent to put each argument and return type on its own line here.

bobbrow · 2021-07-30T16:22:18Z

src/util.ts

+export function envGet(
+  env: GeneralEnvironmentType,
+  expectKey: string
+): [string, string] | undefined {


I would prefer that you define an interface for this instead of using a pair. Interfaces let you name the fields which makes reading the code easier later on. Anyplace where you use [0] I have to go back to the comment for this function and figure out what that means. Alternately, if it's .key, then there is no confusion.

bobbrow · 2021-07-30T16:29:06Z

src/util.ts

+ * @param expectKey The environment key
+ * @return [key, value] pair of the found environment item or undefined when not found
+ */
+export function envGet(


Based on what I see, this function does not need to be exported. It might also make sense to rename it envGetKeyValue to disambiguate it better.

bobbrow · 2021-07-30T16:46:19Z

src/util.ts

+    if (env instanceof Map) {
+      return find(env.entries(), ([key, _val]) => envKeyEqualWin32(key, expectKey));
+    } else {
+      for (const key of Object.keys(env)) {


It feels really inefficient to loop through all keys anytime you want to access a value. process.env should accept the key in a case-insensitive way -- the problem seems to be with the EnvironmentVariables type which is just an object. I think the better route is to make EnvironmentVariables a class and normalize how variables are inserted so that lookup is trivial. That might be a more involved change though.

bobbrow · 2021-07-30T17:04:21Z

src/expand.ts

+export async function computeExpandedEnvironment(
+  in_env: EnvironmentVariables,
+  expanded_env: EnvironmentVariables,
+  intermediateExpand: boolean,


Can you rename intermediateExpand to ignoreUnknownVars? I think that better conveys the meaning.

good name, thanks

lygstate · 2021-07-30T17:35:36Z

The names of these new functions and the way we access the environment seem a little weird now. I wouldn't think to go to util to get environment values. I think it would be better if we had an environment or env module that managed all of this.

Accessing the process.env directly should be ok. process.env looks up environment variables in a case-insensitive way on Windows which should be faster than the new function. The real problem is with the proc.EnvironmentVariables type which does not do the same thing. There is also new expansion behavior added that seems it should be in a separate PR. I would prefer for this PR to scale back the changes and focus on the problem with accessing variables in proc.EnvironmentVariables. I worry that the changes proposed are risky for 1.8 given that we want to release it soon.

It's OK to merge this in 1.9, I'll apply your suggestions

bobbrow · 2021-07-30T22:56:10Z

I didn't finish this prototype -- it doesn't compile yet. But it's kind of what I was thinking for more consistent access to the environment.

Draft PR: #2025

lygstate · 2021-07-31T07:59:27Z

I didn't finish this prototype -- it doesn't compile yet. But it's kind of what I was thinking for more consistent access to the environment.

Draft PR: #2025

OK, would you implement this or I can reference your draft and re-implement this PR

bobbrow · 2021-08-02T18:38:24Z

Because I manage a lot of projects, I don't always have time to work on it. If you would like to complete this, that would be the fastest path.

lygstate · 2021-08-02T21:11:29Z

Because I manage a lot of projects, I don't always have time to work on it. If you would like to complete this, that would be the fastest path.

Sorry, it's a accident close

bobbrow · 2021-10-29T22:23:49Z

This appears to have been closed automatically because we deleted the 'develop' branch in favor of working directly in 'main'. Draft PR: #2025 has been retargeted to 'main'.

lygstate force-pushed the handle-env-set-get branch 2 times, most recently from e42c865 to b96cbe0 Compare April 13, 2021 17:53

bobbrow added this to the 1.8.0 milestone Apr 13, 2021

lygstate force-pushed the handle-env-set-get branch 25 times, most recently from 21aada3 to ffcfe24 Compare April 16, 2021 07:24

bobbrow added this to the 1.9.0 milestone Jul 9, 2021

lygstate force-pushed the handle-env-set-get branch from dd568f4 to 4cafa84 Compare July 14, 2021 09:23

elahehrashedi reviewed Jul 22, 2021

View reviewed changes

src/proc.ts Outdated Show resolved Hide resolved

lygstate force-pushed the handle-env-set-get branch 2 times, most recently from c3eda93 to f318283 Compare July 25, 2021 16:18

lygstate added 3 commits July 28, 2021 02:28

Convert all src process.env access to util.envGetValue

ad06918

Convert collectDevBatVars to use util.envGetValue

189ff99

lygstate force-pushed the handle-env-set-get branch from ec0fe5e to a4dec6a Compare July 27, 2021 18:29

lygstate mentioned this pull request Jul 28, 2021

Patch Windows SDK bin path in a more reliable way. #2021

Merged

lygstate requested a review from elahehrashedi July 28, 2021 19:31

bobbrow requested changes Jul 30, 2021

View reviewed changes

lygstate closed this Jul 31, 2021

lygstate reopened this Aug 2, 2021

bobbrow removed this from the 1.9.0 milestone Oct 5, 2021

bobbrow deleted the branch microsoft:develop October 29, 2021 22:05

bobbrow closed this Oct 29, 2021

bobbrow mentioned this pull request Nov 8, 2021

Turn preset to use mergeEnvironmentWithExpand #2234

Closed

github-actions bot locked and limited conversation to collaborators Jan 29, 2022

lygstate deleted the handle-env-set-get branch February 8, 2022 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using consistence function for environment set and get to fix the gap between windows and unix. #1763

Using consistence function for environment set and get to fix the gap between windows and unix. #1763

lygstate commented Apr 13, 2021

bobbrow commented Apr 13, 2021

elahehrashedi Jul 22, 2021 •

edited

Loading

lygstate Jul 25, 2021 •

edited

Loading

elahehrashedi Jul 26, 2021

elahehrashedi Jul 22, 2021

lygstate Jul 25, 2021 •

edited

Loading

elahehrashedi Jul 26, 2021

lygstate Jul 27, 2021 •

edited

Loading

lygstate commented Jul 30, 2021

bobbrow left a comment

bobbrow Jul 30, 2021

bobbrow Jul 30, 2021

bobbrow Jul 30, 2021

bobbrow Jul 30, 2021

bobbrow Jul 30, 2021

lygstate Jul 30, 2021

lygstate commented Jul 30, 2021

bobbrow commented Jul 30, 2021

lygstate commented Jul 31, 2021

bobbrow commented Aug 2, 2021

lygstate commented Aug 2, 2021

bobbrow commented Oct 29, 2021

Using consistence function for environment set and get to fix the gap between windows and unix. #1763

Using consistence function for environment set and get to fix the gap between windows and unix. #1763

Conversation

lygstate commented Apr 13, 2021

bobbrow commented Apr 13, 2021

elahehrashedi Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

lygstate Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lygstate Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lygstate Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

lygstate commented Jul 30, 2021

bobbrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lygstate commented Jul 30, 2021

bobbrow commented Jul 30, 2021

lygstate commented Jul 31, 2021

bobbrow commented Aug 2, 2021

lygstate commented Aug 2, 2021

bobbrow commented Oct 29, 2021

elahehrashedi Jul 22, 2021 •

edited

Loading

lygstate Jul 25, 2021 •

edited

Loading

lygstate Jul 25, 2021 •

edited

Loading

lygstate Jul 27, 2021 •

edited

Loading