From 3e4b9c07d28c5a5347d297fe4da669a0b0fa4d5c Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Tue, 26 Feb 2019 14:01:03 -0800 Subject: [PATCH 1/2] Revert "Do not wrap npm path with quotes" This reverts commit 1ed5e1c63b71e0a4e7fd0493a37e43a7ef518ebb. --- src/typingsInstaller/nodeTypingsInstaller.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/typingsInstaller/nodeTypingsInstaller.ts b/src/typingsInstaller/nodeTypingsInstaller.ts index 2facb1223d01c..1d75218c88323 100644 --- a/src/typingsInstaller/nodeTypingsInstaller.ts +++ b/src/typingsInstaller/nodeTypingsInstaller.ts @@ -89,6 +89,10 @@ namespace ts.server.typingsInstaller { log); this.npmPath = npmLocation !== undefined ? npmLocation : getDefaultNPMLocation(process.argv[0]); + // If the NPM path contains spaces and isn't wrapped in quotes, do so. + if (stringContains(this.npmPath, " ") && this.npmPath[0] !== `"`) { + this.npmPath = `"${this.npmPath}"`; + } if (this.log.isEnabled()) { this.log.writeLine(`Process id: ${process.pid}`); this.log.writeLine(`NPM location: ${this.npmPath} (explicit '${Arguments.NpmLocation}' ${npmLocation === undefined ? "not " : ""} provided)`); From fd10c12116b7caf5fe8766812c3c26fac4f43c9c Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Tue, 26 Feb 2019 14:01:42 -0800 Subject: [PATCH 2/2] Revert "Use execFileSync in typing installer" This reverts commit bc386c11fd3f026ca84ec556b1b8fb4a2eee0038. --- .../unittests/tsserver/typingsInstaller.ts | 12 ++++++------ src/typingsInstaller/nodeTypingsInstaller.ts | 16 ++++++++-------- src/typingsInstallerCore/typingsInstaller.ts | 17 +++++------------ 3 files changed, 19 insertions(+), 26 deletions(-) diff --git a/src/testRunner/unittests/tsserver/typingsInstaller.ts b/src/testRunner/unittests/tsserver/typingsInstaller.ts index 5d648ba23a20a..76df9934682fd 100644 --- a/src/testRunner/unittests/tsserver/typingsInstaller.ts +++ b/src/testRunner/unittests/tsserver/typingsInstaller.ts @@ -1684,9 +1684,9 @@ namespace ts.projectSystem { TI.getNpmCommandForInstallation(npmPath, tsVersion, packageNames, packageNames.length - Math.ceil(packageNames.length / 2)).command ]; it("works when the command is too long to install all packages at once", () => { - const commands: [string, string[]][] = []; - const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, (file, args) => { - commands.push([file, args]); + const commands: string[] = []; + const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, command => { + commands.push(command); return false; }); assert.isFalse(hasError); @@ -1694,9 +1694,9 @@ namespace ts.projectSystem { }); it("installs remaining packages when one of the partial command fails", () => { - const commands: [string, string[]][] = []; - const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, (file, args) => { - commands.push([file, args]); + const commands: string[] = []; + const hasError = TI.installNpmPackages(npmPath, tsVersion, packageNames, command => { + commands.push(command); return commands.length === 1; }); assert.isTrue(hasError); diff --git a/src/typingsInstaller/nodeTypingsInstaller.ts b/src/typingsInstaller/nodeTypingsInstaller.ts index 1d75218c88323..62bdcfce2603b 100644 --- a/src/typingsInstaller/nodeTypingsInstaller.ts +++ b/src/typingsInstaller/nodeTypingsInstaller.ts @@ -70,10 +70,10 @@ namespace ts.server.typingsInstaller { cwd: string; encoding: "utf-8"; } - type ExecFileSync = (file: string, args: string[], options: ExecSyncOptions) => string; + type ExecSync = (command: string, options: ExecSyncOptions) => string; export class NodeTypingsInstaller extends TypingsInstaller { - private readonly nodeExecFileSync: ExecFileSync; + private readonly nodeExecSync: ExecSync; private readonly npmPath: string; readonly typesRegistry: Map>; @@ -97,7 +97,7 @@ namespace ts.server.typingsInstaller { this.log.writeLine(`Process id: ${process.pid}`); this.log.writeLine(`NPM location: ${this.npmPath} (explicit '${Arguments.NpmLocation}' ${npmLocation === undefined ? "not " : ""} provided)`); } - ({ execFileSync: this.nodeExecFileSync } = require("child_process")); + ({ execSync: this.nodeExecSync } = require("child_process")); this.ensurePackageDirectoryExists(globalTypingsCacheLocation); @@ -105,7 +105,7 @@ namespace ts.server.typingsInstaller { if (this.log.isEnabled()) { this.log.writeLine(`Updating ${typesRegistryPackageName} npm package...`); } - this.execFileSyncAndLog(this.npmPath, ["install", "--ignore-scripts", `${typesRegistryPackageName}@${this.latestDistTag}`], { cwd: globalTypingsCacheLocation }); + this.execSyncAndLog(`${this.npmPath} install --ignore-scripts ${typesRegistryPackageName}@${this.latestDistTag}`, { cwd: globalTypingsCacheLocation }); if (this.log.isEnabled()) { this.log.writeLine(`Updated ${typesRegistryPackageName} npm package`); } @@ -189,7 +189,7 @@ namespace ts.server.typingsInstaller { this.log.writeLine(`#${requestId} with arguments'${JSON.stringify(packageNames)}'.`); } const start = Date.now(); - const hasError = installNpmPackages(this.npmPath, version, packageNames, (file, args) => this.execFileSyncAndLog(file, args, { cwd })); + const hasError = installNpmPackages(this.npmPath, version, packageNames, command => this.execSyncAndLog(command, { cwd })); if (this.log.isEnabled()) { this.log.writeLine(`npm install #${requestId} took: ${Date.now() - start} ms`); } @@ -197,12 +197,12 @@ namespace ts.server.typingsInstaller { } /** Returns 'true' in case of error. */ - private execFileSyncAndLog(file: string, args: string[], options: Pick): boolean { + private execSyncAndLog(command: string, options: Pick): boolean { if (this.log.isEnabled()) { - this.log.writeLine(`Exec: ${file} ${args.join(" ")}`); + this.log.writeLine(`Exec: ${command}`); } try { - const stdout = this.nodeExecFileSync(file, args, { ...options, encoding: "utf-8" }); + const stdout = this.nodeExecSync(command, { ...options, encoding: "utf-8" }); if (this.log.isEnabled()) { this.log.writeLine(` Succeeded. stdout:${indent(sys.newLine, stdout)}`); } diff --git a/src/typingsInstallerCore/typingsInstaller.ts b/src/typingsInstallerCore/typingsInstaller.ts index 3d0858d7dfe34..df83f1a677c39 100644 --- a/src/typingsInstallerCore/typingsInstaller.ts +++ b/src/typingsInstallerCore/typingsInstaller.ts @@ -31,35 +31,28 @@ namespace ts.server.typingsInstaller { } /*@internal*/ - export function installNpmPackages(npmPath: string, tsVersion: string, packageNames: string[], install: (file: string, args: string[]) => boolean) { + export function installNpmPackages(npmPath: string, tsVersion: string, packageNames: string[], install: (command: string) => boolean) { let hasError = false; for (let remaining = packageNames.length; remaining > 0;) { const result = getNpmCommandForInstallation(npmPath, tsVersion, packageNames, remaining); remaining = result.remaining; - hasError = install(result.command[0], result.command[1]) || hasError; + hasError = install(result.command) || hasError; } return hasError; } - function getUserAgent(tsVersion: string) { - return `--user-agent="typesInstaller/${tsVersion}"`; - } - const npmInstall = "install", ignoreScripts = "--ignore-scripts", saveDev = "--save-dev"; - const commandBaseLength = npmInstall.length + ignoreScripts.length + saveDev.length + getUserAgent("").length + 5; /*@internal*/ export function getNpmCommandForInstallation(npmPath: string, tsVersion: string, packageNames: string[], remaining: number) { const sliceStart = packageNames.length - remaining; - let packages: string[], toSlice = remaining; + let command: string, toSlice = remaining; while (true) { - packages = toSlice === packageNames.length ? packageNames : packageNames.slice(sliceStart, sliceStart + toSlice); - const commandLength = npmPath.length + commandBaseLength + packages.join(" ").length + tsVersion.length; - if (commandLength < 8000) { + command = `${npmPath} install --ignore-scripts ${(toSlice === packageNames.length ? packageNames : packageNames.slice(sliceStart, sliceStart + toSlice)).join(" ")} --save-dev --user-agent="typesInstaller/${tsVersion}"`; + if (command.length < 8000) { break; } toSlice = toSlice - Math.floor(toSlice / 2); } - const command: [string, string[]] = [npmPath, [npmInstall, ignoreScripts, ...packages, saveDev, getUserAgent(tsVersion)]]; return { command, remaining: remaining - toSlice }; }