Skip to content

Commit

Permalink
parse arguments in tasks.json (#7895)
Browse files Browse the repository at this point in the history
  • Loading branch information
@elahehrashedi
elahehrashedi committed Sep 9, 2021
1 parent 1db3e20 commit 7516eef
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 45 deletions.
103 changes: 59 additions & 44 deletions Extension/src/LanguageServer/cppBuildTaskProvider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* Copyright (c) Microsoft Corporation. All Rights Reserved.
* See 'LICENSE' in the project root for license information.
* ------------------------------------------------------------------------------------------ */
/* eslint-disable no-unused-expressions */
import * as path from 'path';
import {
TaskDefinition, Task, TaskGroup, ShellExecution, Uri, workspace,
Expand All @@ -25,7 +26,7 @@ export interface CppBuildTaskDefinition extends TaskDefinition {
label: string; // The label appears in tasks.json file.
command: string;
args: string[];
options: cp.ExecOptions | undefined;
options: cp.ExecOptions | cp.SpawnOptions | undefined;
}

export class CppBuildTask extends Task {
Expand Down Expand Up @@ -170,7 +171,7 @@ export class CppBuildTaskProvider implements TaskProvider {
args = args.concat(compilerArgs);
}
const cwd: string = isWindows && !isCl && !process.env.PATH?.includes(path.dirname(compilerPath)) ? path.dirname(compilerPath) : "${fileDirname}";
const options: cp.ExecOptions | undefined = { cwd: cwd };
const options: cp.ExecOptions | cp.SpawnOptions | undefined = { cwd: cwd };
definition = {
type: CppBuildTaskProvider.CppBuildScriptType,
label: taskLabel,
Expand Down Expand Up @@ -341,7 +342,7 @@ class CustomBuildTaskTerminal implements Pseudoterminal {
public get onDidClose(): Event<number> { return this.closeEmitter.event; }
private endOfLine: string = "\r\n";

constructor(private command: string, private args: string[], private options: cp.ExecOptions | undefined) {
constructor(private command: string, private args: string[], private options: cp.ExecOptions | cp.SpawnOptions | undefined) {
}

async open(_initialDimensions: TerminalDimensions | undefined): Promise<void> {
Expand All @@ -364,14 +365,17 @@ class CustomBuildTaskTerminal implements Pseudoterminal {
private async doBuild(): Promise<any> {
// Do build.
let activeCommand: string = util.resolveVariables(this.command);
this.args.forEach(value => {
let temp: string = util.resolveVariables(value);
if (temp && temp.includes(" ")) {
temp = "\"" + temp + "\"";
}
activeCommand = activeCommand + " " + temp;
this.args.forEach((value, index) => {
value = util.normalizeArg(util.resolveVariables(value));
activeCommand = activeCommand + " " + value;
this.args[index] = value;
});
if (this.options?.cwd) {
if (this.options) {
this.options.shell = true;
} else {
this.options = { "shell": true };
}
if (this.options.cwd) {
this.options.cwd = util.resolveVariables(this.options.cwd);
}

Expand All @@ -380,48 +384,59 @@ class CustomBuildTaskTerminal implements Pseudoterminal {
this.writeEmitter.fire(line + this.endOfLine);
}
};

this.writeEmitter.fire(activeCommand + this.endOfLine);
let child: cp.ChildProcess | undefined;
try {
const result: number = await new Promise<number>((resolve, reject) => {
cp.exec(activeCommand, this.options, (_error, stdout, _stderr) => {
const dot: string = ".";
const is_cl: boolean = (_stderr || _stderr === '') && stdout ? true : false;
if (is_cl) {
// cl.exe, header info may not appear if /nologo is used.
if (_stderr) {
splitWriteEmitter(_stderr); // compiler header info and command line D warnings (e.g. when /MTd and /MDd are both used)
}
splitWriteEmitter(stdout); // linker header info and potentially compiler C warnings
}
if (_error) {
if (stdout) {
// cl.exe
} else if (_stderr) {
splitWriteEmitter(_stderr); // gcc/clang
} else {
splitWriteEmitter(_error.message); // e.g. command executable not found
}
telemetry.logLanguageServerEvent("cppBuildTaskError");
this.writeEmitter.fire(localize("build_finished_with_error", "Build finished with error(s)") + dot + this.endOfLine);
child = cp.spawn(this.command, this.args, this.options ? this.options : {});
let error: string = "";
let stdout: string = "";
let stderr: string = "";
const result: number = await new Promise<number>(resolve => {
if (child) {
child.on('error', err => {
splitWriteEmitter(err.message);
error = err.message;
resolve(-1);
} else if (_stderr && !stdout) { // gcc/clang
splitWriteEmitter(_stderr);
telemetry.logLanguageServerEvent("cppBuildTaskWarnings");
this.writeEmitter.fire(localize("build_finished_with_warnings", "Build finished with warning(s)") + dot + this.endOfLine);
resolve(0);
} else if (stdout && stdout.includes("warning C")) { // cl.exe, compiler warnings
telemetry.logLanguageServerEvent("cppBuildTaskWarnings");
this.writeEmitter.fire(localize("build_finished_with_warnings", "Build finished with warning(s)") + dot + this.endOfLine);
resolve(0);
} else {
this.writeEmitter.fire(localize("build finished successfully", "Build finished successfully.") + this.endOfLine);
});
child.stdout?.on('data', data => {
const str: string = data.toString("utf8");
splitWriteEmitter(str);
stdout += str;
});
child.stderr?.on('data', data => {
const str: string = data.toString("utf8");
splitWriteEmitter(str);
stderr += str;
});
child.on('close', result => {
if (result === null) {
this.writeEmitter.fire(localize("build.run.terminated", "Build run was terminated.") + this.endOfLine);
resolve(-1);
}
resolve(0);
}
});
});
}
});
this.printBuildSummary(error, stdout, stderr);
this.closeEmitter.fire(result);
} catch {
this.closeEmitter.fire(-1);
}
}

private printBuildSummary(error: string, stdout: string, stderr: string): void {
if (error || (!stdout && stderr && stderr.includes("error"))) {
telemetry.logLanguageServerEvent("cppBuildTaskError");
this.writeEmitter.fire(localize("build.finished.with.error", "Build finished with error(s).") + this.endOfLine);
} else if (!stdout && stderr) { // gcc/clang
telemetry.logLanguageServerEvent("cppBuildTaskWarnings");
this.writeEmitter.fire(localize("build.finished.with.warnings", "Build finished with warning(s).") + this.endOfLine);
} else if (stdout && stdout.includes("warning C")) { // cl.exe, compiler warnings
telemetry.logLanguageServerEvent("cppBuildTaskWarnings");
this.writeEmitter.fire(localize("build.finished.with.warnings", "Build finished with warning(s).") + this.endOfLine);
} else {
this.writeEmitter.fire(localize("build.finished.successfully", "Build finished successfully.") + this.endOfLine);
}
}
}
23 changes: 23 additions & 0 deletions Extension/src/common.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1304,3 +1304,26 @@ export function sequentialResolve<T>(items: T[], promiseBuilder: (item: T) => Pr
return promiseBuilder(nextItem);
}, Promise.resolve());
}

export function normalizeArg(arg: string): string {
arg = arg.trimLeft().trimRight();
// Check if the arg is enclosed in backtick,
// or includes unscaped double-quotes (or single-quotes on windows),

This comment has been minimized.

Sign in to view

Copy link
@jogo-

jogo- Sep 10, 2021

Contributor

Typo: unscaped -> unescaped

This comment has been minimized.

Sign in to view

Copy link
@sean-mcmanus

sean-mcmanus Sep 10, 2021

Collaborator

Thanks, I can fix that as part of #8129

This comment has been minimized.

Sign in to view

Copy link
@sean-mcmanus

sean-mcmanus Sep 10, 2021

Collaborator

89a764d
// or includes unscaped single-quotes on mac and linux.
if (/^`.*`$/g.test(arg) || /.*[^\\]".*/g.test(arg) ||
(process.platform.includes("win") && /.*[^\\]'.*/g.test(arg)) ||
(!process.platform.includes("win") && /.*[^\\]'.*/g.test(arg))) {
return arg;
}
// The special character double-quote is already scaped in the arg.

This comment has been minimized.

Sign in to view

Copy link
@jogo-

jogo- Sep 10, 2021

Contributor

Typo: scaped -> escaped

This comment has been minimized.

Sign in to view

Copy link
@sean-mcmanus

sean-mcmanus Sep 10, 2021

Collaborator

Thanks, I can fix that as part of #8129

This comment has been minimized.

Sign in to view

Copy link
@sean-mcmanus

sean-mcmanus Sep 10, 2021

Collaborator

89a764d
const unescapedSpaces: string | undefined = arg.split('').find((char, index) => index > 0 && char === " " && arg[index - 1] !== "\\");
if (!unescapedSpaces && !process.platform.includes("win")) {
return arg;
} else if (arg.includes(" ")) {
arg = arg.replace(/\\\s/g, " ");
return "\"" + arg + "\"";
} else {
return arg;
}
}

33 changes: 32 additions & 1 deletion Extension/test/unitTests/common.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* ------------------------------------------------------------------------------------------ */
import * as assert from "assert";
import * as os from "os";
import { envDelimiter, resolveVariables, escapeForSquiggles } from "../../src/common";
import { envDelimiter, resolveVariables, escapeForSquiggles, normalizeArg } from "../../src/common";

suite("Common Utility validation", () => {
suite("resolveVariables", () => {
Expand Down Expand Up @@ -217,6 +217,37 @@ suite("Common Utility validation", () => {
testEscapeForSquigglesScenario("\"\\\\\"", "\"\\\\\\\\\""); // quoted string containing escaped backslash
});

test("normalizeArgs:", () => {
const testNormalizeArgsScenario: any = (input: string, expectedOutput: string) => {
const result: string = normalizeArg(input);
if (result !== expectedOutput) {
throw new Error(`normalizeArgs failure: for \"${input}\", \"${result}\" !== \"${expectedOutput}\"`);
}
};
/*
this is how the args from tasks.json will be sent to the chilprocess.spawn:
"args":[
"-DTEST1=TEST1 TEST1", // "-DTEST1=TEST1 TEST1"
"-DTEST2=\"TEST2 TEST2\"", // -DTEST2="TEST2 TEST2"
"-DTEST3=\\\"TEST3 TEST3\\\"", // "-DTEST3=\"TEST3 TEST3\""
"-DTEST4=TEST4\\ TEST4", // "-DTEST4=TEST4 TEST4"
"-DTEST5='TEST5 TEST5'", // -DTEST5='TEST5 TEST5'
"-DTEST6=TEST6\\ TEST6 Test6", // "-DTEST6=TEST6 TEST6 Test6"
]
*/
testNormalizeArgsScenario("-DTEST1=TEST1 TEST1", "\"-DTEST1=TEST1 TEST1\"");
testNormalizeArgsScenario("-DTEST2=\"TEST2 TEST2\"", "-DTEST2=\"TEST2 TEST2\"");
testNormalizeArgsScenario("-DTEST3=\\\"TEST3 TEST3\\\"", "\"-DTEST3=\\\"TEST3 TEST3\\\"\"");
if (process.platform.includes("win")) {
testNormalizeArgsScenario("-DTEST4=TEST4\\ TEST4", "\"-DTEST4=TEST4 TEST4\"");
testNormalizeArgsScenario("-DTEST5=\'TEST5 TEST5\'", "-DTEST5=\'TEST5 TEST5\'");
} else {
testNormalizeArgsScenario("-DTEST4=TEST4\\ TEST4", "-DTEST4=TEST4\\ TEST4");
testNormalizeArgsScenario("-DTEST5='TEST5 TEST5'", "-DTEST5='TEST5 TEST5'");
}
testNormalizeArgsScenario("-DTEST6=TEST6\\ TEST6 Test6", "\"-DTEST6=TEST6 TEST6 Test6\"");
});

interface ResolveTestFlowEnvironment {
withEnvironment(additionalEnvironment: {[key: string]: string | string[]}): ResolveTestFlowAssert;
shouldLookupSymbol(key: string): void;
Expand Down

0 comments on commit 7516eef

Please sign in to comment.