diff --git a/vscode-dotnet-runtime-extension/src/test/functional/DotnetCoreAcquisitionExtension.test.ts b/vscode-dotnet-runtime-extension/src/test/functional/DotnetCoreAcquisitionExtension.test.ts index 49ed4d8365..569ac8cb05 100644 --- a/vscode-dotnet-runtime-extension/src/test/functional/DotnetCoreAcquisitionExtension.test.ts +++ b/vscode-dotnet-runtime-extension/src/test/functional/DotnetCoreAcquisitionExtension.test.ts @@ -138,7 +138,7 @@ suite('DotnetCoreAcquisitionExtension End to End', function () context.architecture = arch; } const result = await vscode.commands.executeCommand('dotnet.acquire', context); - assert.exists(result, 'Command results a result'); + assert.exists(result, 'Acquire Command results a result'); assert.exists(result!.dotnetPath, 'The return type of the local runtime install command has a .dotnetPath property'); assert.isTrue(fs.existsSync(result!.dotnetPath), 'The returned path of .net does exist'); assert.include(result!.dotnetPath, '.dotnet', '.dotnet is in the path of the local runtime install'); diff --git a/vscode-dotnet-runtime-library/src/Acquisition/DotnetConditionValidator.ts b/vscode-dotnet-runtime-library/src/Acquisition/DotnetConditionValidator.ts index b2bd9fd0ef..6364183f39 100644 --- a/vscode-dotnet-runtime-library/src/Acquisition/DotnetConditionValidator.ts +++ b/vscode-dotnet-runtime-library/src/Acquisition/DotnetConditionValidator.ts @@ -107,7 +107,7 @@ export class DotnetConditionValidator implements IDotnetConditionValidator return ''; } - const infoCommand = CommandExecutor.makeCommand(`"${hostPath}"`, ['--info']); + const infoCommand = CommandExecutor.makeCommand(`${hostPath}`, ['--info']); const envWithForceEnglish = process.env; envWithForceEnglish.DOTNET_CLI_UI_LANGUAGE = 'en-US'; // System may not have english installed, but CDK already calls this without issue -- the .NET SDK language invocation is also wrapped by a runtime library and natively includes english assets @@ -143,7 +143,7 @@ Please set the PATH to a dotnet host that matches the architecture ${requirement return []; } - const findSDKsCommand = CommandExecutor.makeCommand(`"${existingPath}"`, ['--list-sdks', '--arch', requestedArchitecture]); + const findSDKsCommand = CommandExecutor.makeCommand(`${existingPath}`, ['--list-sdks', '--arch', requestedArchitecture]); const sdkInfo = await (this.executor!).execute(findSDKsCommand, { dotnetInstallToolCacheTtlMs: DOTNET_INFORMATION_CACHE_DURATION_MS }, false).then(async (result) => { @@ -182,7 +182,7 @@ Please set the PATH to a dotnet host that matches the architecture ${requirement // We don't check that the version is 10.0 or later after 2026 when .NET 11 starts rolling out, as It will be slower to check all of the numbers in the output for versions >= 10. const hostMaySupportArchFlag = listDotnetInstallsStdout.includes("10.0") || listDotnetInstallsStdout.includes("11.0") || Date.now() >= new Date('2026-03-01').getTime(); // Use runtimes instead of sdks, as sdks will always have a runtime, and runtime search can be cached across both mode calls. - const findInvalidCommand = CommandExecutor.makeCommand(`"${dotnetExecutablePath}"`, ['--list-runtimes', '--arch', 'invalid-arch']); + const findInvalidCommand = CommandExecutor.makeCommand(`${dotnetExecutablePath}`, ['--list-runtimes', '--arch', 'invalid-arch']); const hostSupportsArchFlag = hostMaySupportArchFlag ? (await (this.executor!).execute(findInvalidCommand, { dotnetInstallToolCacheTtlMs: DOTNET_INFORMATION_CACHE_DURATION_MS }, false)).status !== '0' : false; return hostSupportsArchFlag; } @@ -325,7 +325,7 @@ Please set the PATH to a dotnet host that matches the architecture ${requirement private getRuntimesCommand(existingPath: string, requestedArchitecture: string): CommandExecutorCommand { - return CommandExecutor.makeCommand(`"${existingPath}"`, ['--list-runtimes', '--arch', requestedArchitecture]); + return CommandExecutor.makeCommand(`${existingPath}`, ['--list-runtimes', '--arch', requestedArchitecture]); } public async getRuntimes(existingPath: string, requestedArchitecture: string | null, knownArchitecture: boolean): Promise diff --git a/vscode-dotnet-runtime-library/src/Acquisition/DotnetCoreAcquisitionWorker.ts b/vscode-dotnet-runtime-library/src/Acquisition/DotnetCoreAcquisitionWorker.ts index 26017dba0d..ae1d30465c 100644 --- a/vscode-dotnet-runtime-library/src/Acquisition/DotnetCoreAcquisitionWorker.ts +++ b/vscode-dotnet-runtime-library/src/Acquisition/DotnetCoreAcquisitionWorker.ts @@ -35,8 +35,7 @@ import DotnetWSLSecurityError, EventBasedError, EventCancellationError, - SuppressedAcquisitionError, - UtilizingExistingInstallPromise + SuppressedAcquisitionError } from '../EventStream/EventStreamEvents'; import * as versionUtils from './VersionUtilities'; @@ -360,7 +359,7 @@ To keep your .NET version up to date, please reconnect to the internet at your s private async sdkIsFound(context: IAcquisitionWorkerContext, version: string): Promise { const executor = new CommandExecutor(context, this.utilityContext); - const listSDKsCommand = CommandExecutor.makeCommand('dotnet', ['--list-sdks', '--arch']); + const listSDKsCommand = CommandExecutor.makeCommand('dotnet', ['--list-sdks', '--arch', context.acquisitionContext.architecture ?? DotnetCoreAcquisitionWorker.defaultArchitecture()], false); const result = await executor.execute(listSDKsCommand, { dotnetInstallToolCacheTtlMs: DOTNET_INFORMATION_CACHE_DURATION_MS }, false); if (result.status !== '0') diff --git a/vscode-dotnet-runtime-library/src/Acquisition/DotnetPathFinder.ts b/vscode-dotnet-runtime-library/src/Acquisition/DotnetPathFinder.ts index fba1577e75..619207ebdd 100644 --- a/vscode-dotnet-runtime-library/src/Acquisition/DotnetPathFinder.ts +++ b/vscode-dotnet-runtime-library/src/Acquisition/DotnetPathFinder.ts @@ -411,7 +411,7 @@ export class DotnetPathFinder implements IDotnetPathFinder // /usr/local/bin/dotnet becomes /snap/dotnet-sdk/current/dotnet in reality, may have different behavior in shells. if (os.platform() === 'win32') { - LocalMemoryCacheSingleton.getInstance().aliasCommandAsAnotherCommandRoot(`"${truePath}"`, `"${tentativePath}"`, this.workerContext.eventStream); + LocalMemoryCacheSingleton.getInstance().aliasCommandAsAnotherCommandRoot(`${truePath}`, `${tentativePath}`, this.workerContext.eventStream); } } else diff --git a/vscode-dotnet-runtime-library/src/Acquisition/WinMacGlobalInstaller.ts b/vscode-dotnet-runtime-library/src/Acquisition/WinMacGlobalInstaller.ts index fdf6dd413b..0c0de3adbd 100644 --- a/vscode-dotnet-runtime-library/src/Acquisition/WinMacGlobalInstaller.ts +++ b/vscode-dotnet-runtime-library/src/Acquisition/WinMacGlobalInstaller.ts @@ -262,7 +262,7 @@ This report should be made at https://github.com/dotnet/vscode-dotnet-runtime/is { if (os.platform() === 'win32') // Windows does not have chmod +x ability with nodejs. { - const permissionsCommand = CommandExecutor.makeCommand('icacls', [`"${installerPath}"`, '/grant:r', `"%username%":F`, '/t', '/c']); + const permissionsCommand = CommandExecutor.makeCommand('icacls', [`${installerPath}`, '/grant:r', `%username%:F`, '/t', '/c']); const commandRes = await this.commandRunner.execute(permissionsCommand, {}, false); if (commandRes.stderr !== '') { @@ -334,7 +334,7 @@ Please try again, or download the .NET Installer file yourself. You may also rep else if (error?.message?.includes('EPERM')) { this.acquisitionContext.eventStream.post(new DotnetFileIntegrityFailureEvent(`The file ${installerFile} did not have the correct permissions scope to be assessed. -Permissions: ${JSON.stringify(await this.commandRunner.execute(CommandExecutor.makeCommand('icacls', [`"${installerFile}"`]), { dotnetInstallToolCacheTtlMs: SYSTEM_INFORMATION_CACHE_DURATION_MS }, false))}`)); +Permissions: ${JSON.stringify(await this.commandRunner.execute(CommandExecutor.makeCommand('icacls', [`${installerFile}`]), { dotnetInstallToolCacheTtlMs: SYSTEM_INFORMATION_CACHE_DURATION_MS }, false))}`)); } return ask ? this.userChoosesToContinueWithInvalidHash() : false; } @@ -436,7 +436,7 @@ Please correct your PATH variable or make sure the 'open' utility is installed s } else if (workingCommand.commandRoot === 'command') { - workingCommand = CommandExecutor.makeCommand(`open`, [`-W`, `"${path.resolve(installerPath)}"`]); + workingCommand = CommandExecutor.makeCommand(`open`, [`-W`, `${path.resolve(installerPath)}`]); } this.acquisitionContext.eventStream.post(new NetInstallerBeginExecutionEvent(`The OS X .NET Installer has been launched.`)); @@ -450,15 +450,15 @@ Please correct your PATH variable or make sure the 'open' utility is installed s } else { - const command = `"${path.resolve(installerPath)}"`; + const command = `${path.resolve(installerPath)}`; let commandOptions: string[] = []; if (await this.file.isElevated(this.acquisitionContext, this.utilityContext)) { - commandOptions = [`/quiet`, `/install`, `/norestart`]; + commandOptions = [`/install`, `/quiet`, `/norestart`]; } else { - commandOptions = [`/passive`, `/install`, `/norestart`] + commandOptions = [`/install`, `/passive`, `/norestart`] } this.acquisitionContext.eventStream.post(new NetInstallerBeginExecutionEvent(`The Windows .NET Installer has been launched.`)); @@ -478,7 +478,7 @@ Please correct your PATH variable or make sure the 'open' utility is installed s // Remove this when https://github.com/typescript-eslint/typescript-eslint/issues/2728 is done // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access error.message = `The installer does not have permission to execute. Please try running as an administrator. ${error?.message}. -Permissions: ${JSON.stringify(await this.commandRunner.execute(CommandExecutor.makeCommand('icacls', [`"${installerPath}"`]), { dotnetInstallToolCacheTtlMs: SYSTEM_INFORMATION_CACHE_DURATION_MS }, false))}`; +Permissions: ${JSON.stringify(await this.commandRunner.execute(CommandExecutor.makeCommand('icacls', [`${installerPath}`]), { dotnetInstallToolCacheTtlMs: SYSTEM_INFORMATION_CACHE_DURATION_MS }, false))}`; } // Remove this when https://github.com/typescript-eslint/typescript-eslint/issues/2728 is done // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access diff --git a/vscode-dotnet-runtime-library/src/Utils/CommandExecutor.ts b/vscode-dotnet-runtime-library/src/Utils/CommandExecutor.ts index 4f78fb0176..d7a855d99e 100644 --- a/vscode-dotnet-runtime-library/src/Utils/CommandExecutor.ts +++ b/vscode-dotnet-runtime-library/src/Utils/CommandExecutor.ts @@ -6,7 +6,6 @@ import * as proc from 'child_process'; import * as fs from 'fs'; import * as os from 'os'; -import { promisify } from 'util'; import open = require('open'); import path = require('path'); @@ -440,7 +439,11 @@ ${stderr}`)); options.cwd ??= path.resolve(__dirname); // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - options.shell ??= true; + options.shell ??= os.platform() === 'win32' ? false : true; + // ^ Windows systemcalls (node.js process library uses process_wrap which uses process.cc which makes system calls) + // Windows seems to resolve the PATH by default in a system call. Unix system calls do not. + // We could further improve Unix performance in the future by re-implementing our own PATH resolution. + // And turning SHELL off by default on Unix as well. // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access options.encoding = 'utf8'; @@ -455,7 +458,7 @@ ${stderr}`)); else { options = { - cwd: path.resolve(__dirname), shell: true, encoding: 'utf8', env: + cwd: path.resolve(__dirname), shell: os.platform() === 'win32' ? false : true, encoding: 'utf8', env: { ...process.env, DOTNET_CLI_UI_LANGUAGE: 'en-US', DOTNET_NOLOGO: 'true' } }; } @@ -515,39 +518,79 @@ ${stderr}`)); } const commandStartTime = process.hrtime.bigint(); - const commandResult: CommandExecutorResult = await promisify(proc.exec)(fullCommandString, options).then( - fulfilled => + const commandResult: CommandExecutorResult = await this.asyncSpawn(command, options, terminalFailure); + this.logCommandResult(commandResult, fullCommandString, commandStartTime, command.commandRoot); + + if (useCache) + { + LocalMemoryCacheSingleton.getInstance().putCommand({ command, options }, commandResult, this.context); + } + return commandResult; + } + } + + private asyncSpawn(commandToExecute: CommandExecutorCommand, options: any, terminalFailure: boolean): Promise + { + return new Promise((resolve, reject) => + { + const child = proc.spawn(commandToExecute.commandRoot, commandToExecute.commandParts, { + ...options, + stdio: ['ignore', 'pipe', 'pipe'], // Ignore stdin (we won't send any input), Capture stdout and stderr + }); + + let stdout = ''; + let stderr = ''; + + child.stdout.on('data', (data: any) => + { + stdout += data.toString(); + }); + + child.stderr.on('data', (data: any) => + { + stderr += data.toString(); + }); + + child.on('close', (code: any) => + { + if (code === 0) + { + return resolve({ stdout, stderr, status: '0' }); + } + else { - // If any status besides 0 is returned, an error is thrown by nodejs - return { stdout: fulfilled.stdout?.toString() ?? '', stderr: fulfilled.stderr?.toString() ?? '', status: '0' }; - }, - rejected => // Rejected object: error type with stderr : Buffer, stdout : Buffer ... with .code (number) or .signal (string)} - { // see https://nodejs.org/api/child_process.html#child_processexeccommand-options-callback - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - const result = { stdout: rejected?.stdout?.toString() ?? '', stderr: rejected?.stderr?.toString() ?? '', status: rejected?.code?.toString() ?? rejected?.signal?.toString() ?? '' }; + const result = { stdout, stderr, status: code?.toString() ?? '1' }; if (terminalFailure) { - this.logCommandResult(result, fullCommandString, commandStartTime, command.commandRoot); - throw rejected ?? new Error(`Spawning ${fullCommandString} failed with an unspecified error.`); // according to nodejs spec, this should never be possible + this.logCommandResult(result, CommandExecutor.prettifyCommandExecutorCommand(commandToExecute, false), process.hrtime.bigint(), commandToExecute.commandRoot); + return reject(new CommandExecutionNonZeroExitFailure(new EventBasedError('CommandExecutionNonZeroExitFailure', ''), null)); } else { // signal is a string or obj, code is a number - return result; + return resolve(result); } } - ); + }); // We don't need to handle exit, close is when all exits have been called. (stderr, stdout) - this.logCommandResult(commandResult, fullCommandString, commandStartTime, command.commandRoot); - - if (useCache) + child.on('error', (error) => { - LocalMemoryCacheSingleton.getInstance().putCommand({ command, options }, commandResult, this.context); - } - return commandResult; - } + const result = { stdout, stderr, status: error.name?.toString() ?? '' }; + if (terminalFailure) + { + this.logCommandResult(result, CommandExecutor.prettifyCommandExecutorCommand(commandToExecute, false), process.hrtime.bigint(), commandToExecute.commandRoot); + return reject(new CommandExecutionNonZeroExitFailure(new EventBasedError('CommandExecutionNonZeroExitFailure', ''), null)); + } + else + { + // signal is a string or obj, code is a number + return resolve(result); + } + }); + }); } + private logCommandResult(commandResult: CommandExecutorResult, fullCommandStringForTelemetryOnly: string, commandStartTime: bigint, commandRoot: string) { const durationMs = (Number(process.hrtime.bigint() - commandStartTime) / 1000000).toFixed(2); @@ -646,10 +689,10 @@ Please report this at https://github.com/dotnet/vscode-dotnet-runtime/issues.`), if (os.platform() === 'win32') { const setShellVariable = CommandExecutor.makeCommand(`set`, [`${variable}=${value}`]); - const setSystemVariable = CommandExecutor.makeCommand(`setx`, [`${variable}`, `"${value}"`]); + const setSystemVariable = CommandExecutor.makeCommand(`setx`, [`${variable}`, `${value}`]); try { - const shellEditResponse = (await this.execute(setShellVariable, null, false)).status; + const shellEditResponse = (await this.execute(setShellVariable, { shell: true }, false)).status; environmentEditExitCode += Number(shellEditResponse[0]); const systemEditResponse = (await this.execute(setSystemVariable, null, false)).status environmentEditExitCode += Number(systemEditResponse[0]); diff --git a/vscode-dotnet-runtime-library/src/Utils/FileUtilities.ts b/vscode-dotnet-runtime-library/src/Utils/FileUtilities.ts index 91bef85c37..71bec8dd9f 100644 --- a/vscode-dotnet-runtime-library/src/Utils/FileUtilities.ts +++ b/vscode-dotnet-runtime-library/src/Utils/FileUtilities.ts @@ -318,7 +318,7 @@ export class FileUtilities extends IFileUtilities { try { - const commandResult = await executor.execute(CommandExecutor.makeCommand('id', ['-u']), { dotnetInstallToolCacheTtlMs: SYSTEM_INFORMATION_CACHE_DURATION_MS }, false); + const commandResult = await executor.execute(CommandExecutor.makeCommand('id', ['-u']), { shell: true, dotnetInstallToolCacheTtlMs: SYSTEM_INFORMATION_CACHE_DURATION_MS }, false); return commandResult.status === '0'; } catch (error: any) diff --git a/vscode-dotnet-runtime-library/src/Utils/TypescriptUtilities.ts b/vscode-dotnet-runtime-library/src/Utils/TypescriptUtilities.ts index ce33c41006..2ac4fcbae1 100644 --- a/vscode-dotnet-runtime-library/src/Utils/TypescriptUtilities.ts +++ b/vscode-dotnet-runtime-library/src/Utils/TypescriptUtilities.ts @@ -49,7 +49,7 @@ export async function isRunningUnderWSL(acquisitionContext: IAcquisitionWorkerCo const command = CommandExecutor.makeCommand('grep', ['-i', 'Microsoft', '/proc/version']); executor ??= new CommandExecutor(acquisitionContext, utilityContext); - const commandResult = await executor.execute(command, {}, false); + const commandResult = await executor.execute(command, { shell: true }, false); if (!commandResult || !commandResult.stdout) { diff --git a/vscode-dotnet-runtime-library/src/test/unit/WinMacGlobalInstaller.test.ts b/vscode-dotnet-runtime-library/src/test/unit/WinMacGlobalInstaller.test.ts index 76c26efc9e..6696f3018a 100644 --- a/vscode-dotnet-runtime-library/src/test/unit/WinMacGlobalInstaller.test.ts +++ b/vscode-dotnet-runtime-library/src/test/unit/WinMacGlobalInstaller.test.ts @@ -112,17 +112,19 @@ suite('Windows & Mac Global Installer Tests', function () { assert.isTrue(mockExecutor.attemptedCommand.startsWith('open'), `It ran the right mac command, open. Command found: ${mockExecutor.attemptedCommand}`); assert.isTrue(mockExecutor.attemptedCommand.includes('-W'), 'It used the -W flag'); - assert.isTrue(mockExecutor.attemptedCommand.includes('"'), 'It put the installer in quotes for username with space in it'); + const executablePath = mockExecutor.attemptedCommand.split('-W')[1].trim(); + assert.isTrue(fs.existsSync(executablePath), 'It used a full path of an executable that exists'); } else if (os.platform() === 'win32') { - const returnedPath = mockExecutor.attemptedCommand.split(' ')[0].slice(1, -1); + const returnedPath = mockExecutor.attemptedCommand.split('/install')[0].trim(); assert.isTrue(fs.existsSync(returnedPath), `It ran a command to an executable that exists: ${returnedPath}`); - assert.isTrue(mockExecutor.attemptedCommand.includes('"'), 'It put the installer in quotes for username with space in it'); + assert.isTrue(mockExecutor.attemptedCommand.includes('.exe'), 'It has the .exe in the path of the installer, and did not skip / split incorrectly via a path space'); if (await new FileUtilities().isElevated(mockSdkContext, utilContext)) { assert.include(mockExecutor.attemptedCommand, ' /quiet /install /norestart', 'It ran under the hood if it had privileges already'); } + } mockSdkContext // Rerun install to clean it up.