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

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad06918
Convert all src process.env access to util.envGetValue
lygstate Apr 14, 2021
189ff99
Convert collectDevBatVars to use util.envGetValue
lygstate Jul 27, 2021
a4dec6a
Using consistence function for environment set and get to fix the
lygstate Apr 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/cmake-tools.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,7 +688,7 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI {
async executeCMakeCommand(args: string[], options?: ExecutionOptions): Promise<ExecutionResult> {
const drv = await this.getCMakeDriverInstance();
if (drv) {
return drv.executeCommand(drv.cmake.path, args, undefined, options).result;
return (await drv.executeCommand(drv.cmake.path, args, undefined, options)).result;
} else {
throw new Error(localize('unable.to.execute.cmake.command', 'Unable to execute cmake command, there is no valid cmake driver instance.'));
}
Expand All @@ -697,7 +697,7 @@ export class CMakeTools implements vscode.Disposable, api.CMakeToolsAPI {
async execute(program: string, args: string[], options?: ExecutionOptions): Promise<ExecutionResult> {
const drv = await this.getCMakeDriverInstance();
if (drv) {
return drv.executeCommand(program, args, undefined, options).result;
return (await drv.executeCommand(program, args, undefined, options)).result;
} else {
throw new Error(localize('unable.to.execute.program', 'Unable to execute program, there is no valid cmake driver instance.'));
}
Expand Down
5 changes: 3 additions & 2 deletions src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,8 +310,9 @@ export class ConfigurationReader implements vscode.Disposable {
get enableTraceLogging(): boolean { return this.configData.enableTraceLogging; }

get loggingLevel(): LogLevelKey {
if (process.env['CMT_LOGGING_LEVEL']) {
return process.env['CMT_LOGGING_LEVEL']! as LogLevelKey;
const cmt_logging_elvel = util.envGetValue(process.env, 'CMT_LOGGING_LEVEL');
if (cmt_logging_elvel) {
return cmt_logging_elvel as LogLevelKey;
}
return this.configData.loggingLevel;
}
Expand Down
17 changes: 12 additions & 5 deletions src/ctest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ export class CTestDriver implements vscode.Disposable {
this.ws.config.ctestDefaultArgs, this.ws.config.ctestArgs);
}

const child = driver.executeCommand(
const child = await driver.executeCommand(
ctestpath,
ctestArgs,
new CTestOutputLogger(),
Expand Down Expand Up @@ -410,10 +410,17 @@ export class CTestDriver implements vscode.Disposable {
} else {
buildConfigArgs.push('-C', driver.currentBuildType);
}
const result
= await driver
.executeCommand(ctestpath, ['-N', ...buildConfigArgs], undefined, {cwd: driver.binaryDir, silent: true})
.result;
const child = await driver.executeCommand(
ctestpath,
["-N", ...buildConfigArgs],
undefined,
{
cwd: driver.binaryDir,
silent: true,
environment: await driver.getCTestCommandEnvironment()
}
);
const result = await child.result;
if (result.retc !== 0) {
// There was an error running CTest. Odd...
log.error(localize('ctest.error', 'There was an error running ctest to determine available test executables'));
Expand Down
3 changes: 2 additions & 1 deletion src/drivers/cmfileapi-driver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,8 @@ export class CMakeFileApiDriver extends CMakeDriver {
log.debug(`Configuring using ${this.useCMakePresets ? 'preset' : 'kit'}`);
log.debug('Invoking CMake', cmake, 'with arguments', JSON.stringify(args));
const env = await this.getConfigureEnvironment();
const res = await this.executeCommand(cmake, args, outputConsumer, {environment: env}).result;
const child = await this.executeCommand(cmake, args, outputConsumer, {environment: env});
const res = await child.result;
log.trace(res.stderr);
log.trace(res.stdout);
if (res.retc === 0) {
Expand Down
99 changes: 50 additions & 49 deletions src/drivers/driver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {CMakeOutputConsumer} from '@cmt/diagnostics/cmake';
import {RawDiagnosticParser} from '@cmt/diagnostics/util';
import {ProgressMessage} from '@cmt/drivers/cms-client';
import * as expand from '@cmt/expand';
import {CMakeGenerator, effectiveKitEnvironment, Kit, kitChangeNeedsClean, KitDetect, getKitDetect, getKitEnvironmentVariablesObject} from '@cmt/kit';
import {CMakeGenerator, effectiveKitEnvironment, Kit, kitChangeNeedsClean, KitDetect, getKitDetect} from '@cmt/kit';
import * as logging from '@cmt/logging';
import paths from '@cmt/paths';
import {fs} from '@cmt/pr';
Expand Down Expand Up @@ -169,69 +169,56 @@ export abstract class CMakeDriver implements vscode.Disposable {

/**
* The environment variables required by the current kit
* The ${dollar} are not expanded and missing environment variable subs are not expanded for this variables
*/
private _kitEnvironmentVariables = new Map<string, string>();

/**
* Compute the environment variables that apply with substitutions by expansionOptions
*/
async computeExpandedEnvironment(in_env: proc.EnvironmentVariables, expanded_env: proc.EnvironmentVariables): Promise<proc.EnvironmentVariables> {
const env = {} as {[key: string]: string};
const opts = this.expansionOptions;

await Promise.resolve(
util.objectPairs(in_env)
.forEach(async ([key, value]) => env[key] = await expand.expandString(value, {...opts, envOverride: expanded_env}))
);
return env;
}
private _kitEnvironmentVariables: proc.EnvironmentVariables = {};
Copy link
Contributor

@elahehrashedi elahehrashedi Jul 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@lygstate lygstate Jul 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


/**
* Get the environment variables that should be set at CMake-configure time.
*/
async getConfigureEnvironment(): Promise<proc.EnvironmentVariables> {
const envs_to_merge: (proc.EnvironmentVariables | undefined)[] = [];
if (this.useCMakePresets) {
return this._configurePreset?.environment as proc.EnvironmentVariables;
envs_to_merge.push(this._configurePreset?.environment as proc.EnvironmentVariables);
} else {
let envs = getKitEnvironmentVariablesObject(this._kitEnvironmentVariables);
/* 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 */
envs = util.mergeEnvironment(envs, await this.computeExpandedEnvironment(this.config.environment, envs));
envs = util.mergeEnvironment(envs, await this.computeExpandedEnvironment(this.config.configureEnvironment, envs));
envs = util.mergeEnvironment(envs, await this.computeExpandedEnvironment(this._variantEnv, envs));
return envs;
envs_to_merge.push(this._kitEnvironmentVariables);
}
envs_to_merge.push(this.config.environment);
envs_to_merge.push(this.config.configureEnvironment);
envs_to_merge.push(this._variantEnv);
return expand.mergeEnvironmentWithExpand(false, envs_to_merge, this.expansionOptions);
}

/**
* 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];
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@lygstate lygstate Jul 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

/**
 * 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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@lygstate lygstate Jul 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

if (this.useCMakePresets) {
envs = util.mergeEnvironment(in_env, this._buildPreset?.environment as proc.EnvironmentVariables);
envs_to_merge.push(this._buildPreset?.environment as proc.EnvironmentVariables);
} else {
envs = util.mergeEnvironment(in_env, getKitEnvironmentVariablesObject(this._kitEnvironmentVariables));
envs_to_merge.push(this._kitEnvironmentVariables);
}
envs = util.mergeEnvironment(envs, await this.computeExpandedEnvironment(this.config.environment, envs));
envs = util.mergeEnvironment(envs, await this.computeExpandedEnvironment(this.config.buildEnvironment, envs));
envs = util.mergeEnvironment(envs, await this.computeExpandedEnvironment(this._variantEnv, envs));
return envs;
envs_to_merge.push(this.config.environment);
envs_to_merge.push(this.config.buildEnvironment);
envs_to_merge.push(this._variantEnv);
return expand.mergeEnvironmentWithExpand(false, envs_to_merge, this.expansionOptions);
}

/**
* Get the environment variables that should be set at CTest and running program time.
*/
async getCTestCommandEnvironment(): Promise<proc.EnvironmentVariables> {
const envs_to_merge: (proc.EnvironmentVariables | undefined)[] = [];
if (this.useCMakePresets) {
return (this._testPreset?.environment ? this._testPreset?.environment : {}) as proc.EnvironmentVariables;
envs_to_merge.push(this._testPreset?.environment as proc.EnvironmentVariables);
} else {
let envs = getKitEnvironmentVariablesObject(this._kitEnvironmentVariables);
envs = util.mergeEnvironment(envs, await this.computeExpandedEnvironment(this.config.environment, envs));
envs = util.mergeEnvironment(envs, await this.computeExpandedEnvironment(this.config.testEnvironment, envs));
envs = util.mergeEnvironment(envs, await this.computeExpandedEnvironment(this._variantEnv, envs));
return envs;
envs_to_merge.push(this._kitEnvironmentVariables);
}
envs_to_merge.push(this.config.environment);
envs_to_merge.push(this.config.testEnvironment);
envs_to_merge.push(this._variantEnv);
return expand.mergeEnvironmentWithExpand(false, envs_to_merge, this.expansionOptions);
}

get onProgress(): vscode.Event<ProgressMessage> {
Expand Down Expand Up @@ -318,15 +305,29 @@ export abstract class CMakeDriver implements vscode.Disposable {
return {vars, variantVars};
}

getEffectiveSubprocessEnvironment(opts?: proc.ExecutionOptions): proc.EnvironmentVariables {
const cur_env = process.env as proc.EnvironmentVariables;
const kit_env = (this.config.ignoreKitEnv) ? {} : getKitEnvironmentVariablesObject(this._kitEnvironmentVariables);
return util.mergeEnvironment(cur_env, kit_env, (opts && opts.environment) ? opts.environment : {});
async getEffectiveSubprocessEnvironment(opts?: proc.ExecutionOptions): Promise<proc.EnvironmentVariables> {
let envs_to_merge: (proc.EnvironmentVariables | undefined)[] = [];
if (this.config.ignoreKitEnv) {
envs_to_merge = [
process.env as proc.EnvironmentVariables,
this.config.environment,
this._variantEnv,
opts?.environment
];
} else {
envs_to_merge = [
this._kitEnvironmentVariables,
this.config.environment,
this._variantEnv,
opts?.environment
];
}
return expand.mergeEnvironmentWithExpand(false, envs_to_merge, this.expansionOptions);
}

executeCommand(command: string, args?: string[], consumer?: proc.OutputConsumer, options?: proc.ExecutionOptions):
proc.Subprocess {
const environment = this.getEffectiveSubprocessEnvironment(options);
async executeCommand(command: string, args?: string[], consumer?: proc.OutputConsumer, options?: proc.ExecutionOptions):
Promise<proc.Subprocess> {
const environment = await this.getEffectiveSubprocessEnvironment(options);
const exec_options = {...options, environment};
return proc.execute(command, args, consumer, exec_options);
}
Expand All @@ -347,13 +348,13 @@ export abstract class CMakeDriver implements vscode.Disposable {
* Launch the given compilation command in an embedded terminal.
* @param cmd The compilation command from a compilation database to run
*/
runCompileCommand(cmd: ArgsCompileCommand): vscode.Terminal {
async runCompileCommand(cmd: ArgsCompileCommand): Promise<vscode.Terminal> {
let env: proc.EnvironmentVariables;
if (this.useCMakePresets) {
// buildpreset.environment at least has process.env after expansion
env = this._buildPreset!.environment as proc.EnvironmentVariables;
} else {
env = this.getEffectiveSubprocessEnvironment();
env = await this.getCMakeBuildCommandEnvironment({});
}
const key = `${cmd.directory}${JSON.stringify(env)}`;
let existing = this._compileTerms.get(key);
Expand Down Expand Up @@ -742,7 +743,7 @@ export abstract class CMakeDriver implements vscode.Disposable {
}

public async testHaveCommand(program: string, args: string[] = ['--version']): Promise<boolean> {
const child = this.executeCommand(program, args, undefined, {silent: true});
const child = await this.executeCommand(program, args, undefined, {silent: true});
try {
const result = await child.result;
log.debug(localize('command.version.test.return.code', 'Command version test return code {0}', nullableValueToString(result.retc)));
Expand Down Expand Up @@ -838,7 +839,7 @@ export abstract class CMakeDriver implements vscode.Disposable {
if (arg) {
args.push(arg);
}
const child = this.executeCommand(program, args, undefined, {silent: true, cwd});
const child = await this.executeCommand(program, args, undefined, {silent: true, cwd});
try {
const result = await child.result;
console.log(localize('command.version.test.return.code', 'Command version test return code {0}', nullableValueToString(result.retc)));
Expand Down Expand Up @@ -1568,7 +1569,7 @@ export abstract class CMakeDriver implements vscode.Disposable {
}
const exeOpt: proc.ExecutionOptions
= {environment: buildcmd.build_env, outputEncoding: outputEnc, useTask: this.config.buildTask};
const child = this.executeCommand(buildcmd.command, buildcmd.args, consumer, exeOpt);
const child = await this.executeCommand(buildcmd.command, buildcmd.args, consumer, exeOpt);
this._currentBuildProcess = child;
await child.result;
this._currentBuildProcess = null;
Expand Down
3 changes: 2 additions & 1 deletion src/drivers/legacy-driver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ export class LegacyCMakeDriver extends CMakeDriver {
const cmake = this.cmake.path;
log.debug(localize('invoking.cmake.with.arguments', 'Invoking CMake {0} with arguments {1}', cmake, JSON.stringify(args)));
const env = await this.getConfigureEnvironment();
const res = await this.executeCommand(cmake, args, outputConsumer, {environment: env}).result;
const child = await this.executeCommand(cmake, args, outputConsumer, {environment: env});
const res = await child.result;
log.trace(res.stderr);
log.trace(res.stdout);
if (res.retc === 0) {
Expand Down
Loading