From c25dd42808b291d3d0d301502555bf6192249684 Mon Sep 17 00:00:00 2001 From: Huajie Zhang Date: Fri, 24 Nov 2023 15:27:22 +0800 Subject: [PATCH] perf: improve script driver (#10436) * perf: improve script driver * test: ut * fix: encoding --- .../yaml-schema/v1.3/yaml.schema.json | 5 +- .../component/driver/script/scriptDriver.ts | 77 +++++++++++++------ .../driver/script/scriptDriver.test.ts | 61 +++++++++++++++ 3 files changed, 115 insertions(+), 28 deletions(-) diff --git a/packages/fx-core/resource/yaml-schema/v1.3/yaml.schema.json b/packages/fx-core/resource/yaml-schema/v1.3/yaml.schema.json index 5a21a0a7b9..993b86c8f8 100644 --- a/packages/fx-core/resource/yaml-schema/v1.3/yaml.schema.json +++ b/packages/fx-core/resource/yaml-schema/v1.3/yaml.schema.json @@ -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", diff --git a/packages/fx-core/src/component/driver/script/scriptDriver.ts b/packages/fx-core/src/component/driver/script/scriptDriver.ts index 0a20c803a2..3c2c01b164 100644 --- a/packages/fx-core/src/component/driver/script/scriptDriver.ts +++ b/packages/fx-core/src/component/driver/script/scriptDriver.ts @@ -4,22 +4,22 @@ /** * @author huajiezhang */ -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"; @@ -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 { + 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( @@ -82,26 +107,28 @@ export async function executeCommand( redirectTo?: string ): Promise> { 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, })}.` @@ -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 }, @@ -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(""); @@ -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); @@ -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); }; diff --git a/packages/fx-core/tests/component/driver/script/scriptDriver.test.ts b/packages/fx-core/tests/component/driver/script/scriptDriver.test.ts index edf34d74e2..f696339fbd 100644 --- a/packages/fx-core/tests/component/driver/script/scriptDriver.test.ts +++ b/packages/fx-core/tests/component/driver/script/scriptDriver.test.ts @@ -11,6 +11,7 @@ import * as sinon from "sinon"; import * as tools from "../../../../src/common/tools"; import { convertScriptErrorToFxError, + defaultShell, getStderrHandler, parseSetOutputCommand, scriptDriver, @@ -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(); @@ -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); + }); +});