Skip to content

Commit

Permalink
perf: improve script driver (#10436)
Browse files Browse the repository at this point in the history 
* perf: improve script driver

* test: ut

* fix: encoding
  • Loading branch information
@jayzhang
jayzhang authored Nov 24, 2023
1 parent 4a50152 commit c25dd42
Show file tree
Hide file tree
Showing 3 changed files with 115 additions and 28 deletions.
5 changes: 2 additions & 3 deletions packages/fx-core/resource/yaml-schema/v1.3/yaml.schema.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1434,12 +1434,11 @@
},
"workingDirectory": {
"type": "string",
"description": "current working directory. Defaults to the directory of this file."
"description": "Current working directory. Defaults to the directory of this file."
},
"shell": {
"type": "string",
"enum": ["bash", "sh", "powershell", "pwsh", "cmd"],
"description": "The shell type. Can be omitted. If omitted, it defaults to bash on Linux/MacOS, defaults to pwsh on windows."
"description": "Shell command. If not specified, use default shell for current platform. The rule is: 1) use the value of the 'SHELL' environment variable if it is set. Otherwise the shell depends on operation system: 2) on macOS, use '/bin/zsh' if it exists, otherwise use '/bin/bash'; 3) On Windows, use the value of the 'ComSpec' environment variable if it exists, otherwise use 'cmd.exe'; 4) On Linux or other OS, use '/bin/sh' if it extis."
},
"timeout": {
"type": "number",
Expand Down
77 changes: 52 additions & 25 deletions packages/fx-core/src/component/driver/script/scriptDriver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,22 @@
/**
* @author huajiezhang <huajiezhang@microsoft.com>
*/
import { err, FxError, ok, Result, LogProvider } from "@microsoft/teamsfx-api";
import { Service } from "typedi";
import { DriverContext } from "../interface/commonArgs";
import { ExecutionResult, StepDriver } from "../interface/stepDriver";
import { hooks } from "@feathersjs/hooks";
import { addStartAndEndTelemetry } from "../middleware/addStartAndEndTelemetry";
import { FxError, LogProvider, Result, err, ok } from "@microsoft/teamsfx-api";
import child_process from "child_process";
import fs from "fs-extra";
import iconv from "iconv-lite";
import os from "os";
import * as path from "path";
import { Service } from "typedi";
import { ScriptExecutionError, ScriptTimeoutError } from "../../../error/script";
import { TelemetryConstant } from "../../constant/commonConstant";
import { ProgressMessages } from "../../messages";
import { DotenvOutput } from "../../utils/envUtil";
import { ScriptExecutionError, ScriptTimeoutError } from "../../../error/script";
import { getSystemEncoding } from "../../utils/charsetUtils";
import * as path from "path";
import os from "os";
import fs from "fs-extra";
import iconv from "iconv-lite";
import child_process from "child_process";
import { DotenvOutput } from "../../utils/envUtil";
import { DriverContext } from "../interface/commonArgs";
import { ExecutionResult, StepDriver } from "../interface/stepDriver";
import { addStartAndEndTelemetry } from "../middleware/addStartAndEndTelemetry";

const ACTION_NAME = "script";

Expand All @@ -31,6 +31,31 @@ interface ScriptDriverArgs {
redirectTo?: string;
}

/**
* Get the default shell for the current platform:
* - If `SHELL` environment variable is set, return its value. otherwise:
* - On macOS, return `/bin/zsh` if it exists, otherwise return `/bin/bash`.
* - On Windows, return the value of the `ComSpec` environment variable if it exists, otherwise return `cmd.exe`.
* - On Linux, return `/bin/sh`.
*/
export async function defaultShell(): Promise<string | undefined> {
if (process.env.SHELL) {
return process.env.SHELL;
}
if (process.platform === "darwin") {
if (await fs.pathExists("/bin/zsh")) return "/bin/zsh";
else if (await fs.pathExists("/bin/bash")) return "/bin/bash";
return undefined;
}
if (process.platform === "win32") {
return process.env.ComSpec || "cmd.exe";
}
if (await fs.pathExists("/bin/sh")) {
return "/bin/sh";
}
return undefined;
}

@Service(ACTION_NAME)
export class ScriptDriver implements StepDriver {
async _run(
Expand Down Expand Up @@ -82,26 +107,28 @@ export async function executeCommand(
redirectTo?: string
): Promise<Result<[string, DotenvOutput], FxError>> {
const systemEncoding = await getSystemEncoding();
return new Promise((resolve, reject) => {
const dshell = await defaultShell();
return new Promise((resolve) => {
const finalShell = shell || dshell;
let finalCmd = command;
if (typeof finalShell === "string" && finalShell.includes("cmd")) {
finalCmd = `%ComSpec% /D /E:ON /V:OFF /S /C "CALL ${command}"`;
}
const platform = os.platform();
let workingDir = workingDirectory || ".";
workingDir = path.isAbsolute(workingDir) ? workingDir : path.join(projectPath, workingDir);
if (platform === "win32") {
workingDir = capitalizeFirstLetter(path.resolve(workingDir ?? ""));
}
let run = command;
let appendFile: string | undefined = undefined;
if (redirectTo) {
appendFile = path.isAbsolute(redirectTo) ? redirectTo : path.join(projectPath, redirectTo);
}
if (shell === "cmd") {
run = `%ComSpec% /D /E:ON /V:OFF /S /C "CALL ${command}"`;
}
logProvider.verbose(
`Start to run command: "${maskSecretValues(command)}" with args: ${JSON.stringify({
shell: shell,
`Start to run command: "${maskSecretValues(finalCmd)}" with args: ${JSON.stringify({
shell: finalShell,
cwd: workingDir,
encoding: "buffer",
encoding: systemEncoding,
env: { ...process.env, ...env },
timeout: timeout,
})}.`
Expand All @@ -110,9 +137,9 @@ export async function executeCommand(
const stderrStrings: string[] = [];
process.env.VSLANG = undefined; // Workaroud to disable VS environment variable to void charset encoding issue for non-English characters
const cp = child_process.exec(
run,
finalCmd,
{
shell: shell,
shell: finalShell,
cwd: workingDir,
encoding: "buffer",
env: { ...process.env, ...env },
Expand All @@ -121,7 +148,7 @@ export async function executeCommand(
(error) => {
if (error) {
error.message = stderrStrings.join("").trim() || error.message;
resolve(err(convertScriptErrorToFxError(error, run)));
resolve(err(convertScriptErrorToFxError(error, finalCmd)));
} else {
// handle '::set-output' or '::set-teamsfx-env' pattern
const outputString = allOutputStrings.join("");
Expand All @@ -142,7 +169,7 @@ export async function executeCommand(
};
cp.stdout?.on("data", (data: Buffer) => {
const str = bufferToString(data, systemEncoding);
logProvider.info(` [script action stdout] ${maskSecretValues(str)}`);
logProvider.info(` [script stdout] ${maskSecretValues(str)}`);
dataHandler(str);
});
const handler = getStderrHandler(logProvider, systemEncoding, stderrStrings, dataHandler);
Expand All @@ -158,7 +185,7 @@ export function getStderrHandler(
): (data: Buffer) => void {
return (data: Buffer) => {
const str = bufferToString(data, systemEncoding);
logProvider.warning(` [script action stderr] ${maskSecretValues(str)}`);
logProvider.warning(` [script stderr] ${maskSecretValues(str)}`);
dataHandler(str);
stderrStrings.push(str);
};
Expand Down
61 changes: 61 additions & 0 deletions packages/fx-core/tests/component/driver/script/scriptDriver.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import * as sinon from "sinon";
import * as tools from "../../../../src/common/tools";
import {
convertScriptErrorToFxError,
defaultShell,
getStderrHandler,
parseSetOutputCommand,
scriptDriver,
Expand All @@ -21,6 +22,7 @@ import { ScriptExecutionError, ScriptTimeoutError } from "../../../../src/error/
import { MockLogProvider, MockUserInteraction } from "../../../core/utils";
import { TestAzureAccountProvider } from "../../util/azureAccountMock";
import { TestLogProvider } from "../../util/logProviderMock";
import mockedEnv, { RestoreFn } from "mocked-env";

describe("Script Driver test", () => {
const sandbox = sinon.createSandbox();
Expand Down Expand Up @@ -182,3 +184,62 @@ describe("getStderrHandler", () => {
assert.deepEqual(stderrStrings, ["test"]);
});
});

describe("defaultShell", () => {
const sandbox = sinon.createSandbox();
let restoreEnv: RestoreFn = () => {};
afterEach(() => {
sandbox.restore();
restoreEnv();
});
it("SHELL", async () => {
restoreEnv = mockedEnv({ SHELL: "/bin/bash" });
const result = await defaultShell();
assert.equal(result, "/bin/bash");
});
it("darwin - /bin/zsh", async () => {
sandbox.stub(process, "platform").value("darwin");
sandbox.stub(fs, "pathExists").resolves(true);
const result = await defaultShell();
assert.equal(result, "/bin/zsh");
});
it("darwin - /bin/bash", async () => {
sandbox.stub(process, "platform").value("darwin");
sandbox.stub(fs, "pathExists").onFirstCall().resolves(false).onSecondCall().resolves(true);
const result = await defaultShell();
assert.equal(result, "/bin/bash");
});
it("darwin - undefined", async () => {
sandbox.stub(process, "platform").value("darwin");
sandbox.stub(fs, "pathExists").resolves(false);
const result = await defaultShell();
assert.isUndefined(result);
});

it("win32 - ComSpec", async () => {
sandbox.stub(process, "platform").value("win32");
restoreEnv = mockedEnv({ ComSpec: "cmd.exe" });
const result = await defaultShell();
assert.equal(result, "cmd.exe");
});
it("win32 - cmd.exe", async () => {
sandbox.stub(process, "platform").value("win32");
restoreEnv = mockedEnv({ ComSpec: undefined });
const result = await defaultShell();
assert.equal(result, "cmd.exe");
});

it("other OS - /bin/sh", async () => {
sandbox.stub(process, "platform").value("other");
sandbox.stub(fs, "pathExists").resolves(true);
const result = await defaultShell();
assert.equal(result, "/bin/sh");
});

it("other OS - undefined", async () => {
sandbox.stub(process, "platform").value("other");
sandbox.stub(fs, "pathExists").resolves(false);
const result = await defaultShell();
assert.isUndefined(result);
});
});

0 comments on commit c25dd42

Please sign in to comment.