diff --git a/news/2 Fixes/7203.md b/news/2 Fixes/7203.md new file mode 100644 index 00000000000..ec20ee60469 --- /dev/null +++ b/news/2 Fixes/7203.md @@ -0,0 +1 @@ +Support kernelspec argv containing non traditional args for `{connection_file}`. diff --git a/src/client/datascience/jupyter/kernels/helpers.ts b/src/client/datascience/jupyter/kernels/helpers.ts index 679e063fbed..edcddcbb0ee 100644 --- a/src/client/datascience/jupyter/kernels/helpers.ts +++ b/src/client/datascience/jupyter/kernels/helpers.ts @@ -41,11 +41,11 @@ import { isDefaultPythonKernelSpecName } from '../../kernel-launcher/localPython // Helper functions for dealing with kernels and kernelspecs // https://jupyter-client.readthedocs.io/en/stable/kernels.html -const connectionFilePlaceholder = '{connection_file}'; +export const connectionFilePlaceholder = '{connection_file}'; // Find the index of the connection file placeholder in a kernelspec export function findIndexOfConnectionFile(kernelSpec: Readonly): number { - return kernelSpec.argv.indexOf(connectionFilePlaceholder); + return kernelSpec.argv.findIndex((arg) => arg.includes(connectionFilePlaceholder)); } type ConnectionWithKernelSpec = diff --git a/src/client/datascience/kernel-launcher/kernelProcess.ts b/src/client/datascience/kernel-launcher/kernelProcess.ts index abd6a8f3cd5..e76f3682d3f 100644 --- a/src/client/datascience/kernel-launcher/kernelProcess.ts +++ b/src/client/datascience/kernel-launcher/kernelProcess.ts @@ -4,7 +4,6 @@ import { ChildProcess } from 'child_process'; import * as fs from 'fs-extra'; -import * as tcpPortUsed from 'tcp-port-used'; import * as tmp from 'tmp'; import { CancellationToken, Event, EventEmitter } from 'vscode'; import { IPythonExtensionChecker } from '../../api/types'; @@ -19,7 +18,11 @@ import * as localize from '../../common/utils/localize'; import { noop, swallowExceptions } from '../../common/utils/misc'; import { captureTelemetry } from '../../telemetry'; import { Commands, Telemetry } from '../constants'; -import { findIndexOfConnectionFile, isPythonKernelConnection } from '../jupyter/kernels/helpers'; +import { + connectionFilePlaceholder, + findIndexOfConnectionFile, + isPythonKernelConnection +} from '../jupyter/kernels/helpers'; import { KernelSpecConnectionMetadata, PythonKernelConnectionMetadata } from '../jupyter/kernels/types'; import { IJupyterKernelSpec } from '../types'; import { KernelDaemonPool } from './kernelDaemonPool'; @@ -166,6 +169,7 @@ export class KernelProcess implements IKernelProcess { // Don't return until our heartbeat channel is open for connections or the kernel died or we timed out try { + const tcpPortUsed = require('tcp-port-used') as typeof import('tcp-port-used'); await Promise.race([ tcpPortUsed.waitUntilUsed(this.connection.hb_port, 200, timeout), deferred.promise, @@ -282,10 +286,26 @@ export class KernelProcess implements IKernelProcess { const tempFile = await this.fileSystem.createTemporaryLocalFile('.json'); this.connectionFile = tempFile.filePath; await tempFile.dispose(); - await fs.writeFile(this.connectionFile, JSON.stringify(this._connection)); + await this.fileSystem.writeLocalFile(this.connectionFile, JSON.stringify(this._connection)); // Then replace the connection file argument with this file - this.launchKernelSpec.argv[indexOfConnectionFile] = this.connectionFile; + // Remmeber, non-python kernels can have argv as `--connection-file={connection_file}`, + // hence we should not replace the entire entry, but just replace the text `{connection_file}` + // See https://github.com/microsoft/vscode-jupyter/issues/7203 + if (this.launchKernelSpec.argv[indexOfConnectionFile].includes('--connection-file')) { + const connectionFile = this.connectionFile.includes(' ') + ? `"${this.connectionFile}"` // Quoted for spaces in file paths. + : this.connectionFile; + this.launchKernelSpec.argv[indexOfConnectionFile] = this.launchKernelSpec.argv[ + indexOfConnectionFile + ].replace(connectionFilePlaceholder, connectionFile); + } else { + // Even though we don't have `--connection-file` don't assume it won't be `--config-file` for other kernels. + // E.g. in Python the name of the argument is `-f` and in. + this.launchKernelSpec.argv[indexOfConnectionFile] = this.launchKernelSpec.argv[ + indexOfConnectionFile + ].replace(connectionFilePlaceholder, this.connectionFile); + } } } @@ -352,6 +372,11 @@ export class KernelProcess implements IKernelProcess { // This will mainly quote paths so that they can run, other arguments shouldn't be quoted or it may cause errors. // The first argument is sliced because it is the executable command. const args = this.launchKernelSpec.argv.slice(1).map((a) => { + // Some kernel specs (non-python) can have argv as `--connection-file={connection_file}` + // The `connection-file` will be quoted when we update it with the real path. + if (a.includes('--connection-file')) { + return a; + } if (a.includes(' ')) { return `"${a}"`; } diff --git a/src/test/datascience/kernelProcess.vscode.test.ts b/src/test/datascience/kernelProcess.vscode.test.ts new file mode 100644 index 00000000000..2fef9e8054c --- /dev/null +++ b/src/test/datascience/kernelProcess.vscode.test.ts @@ -0,0 +1,235 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +'use strict'; + +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import { IKernelConnection } from '../../client/datascience/kernel-launcher/types'; +import { IS_REMOTE_NATIVE_TEST } from '../constants'; +import { IDisposable } from '../../client/common/types'; +import rewiremock from 'rewiremock'; +import { IProcessService, IProcessServiceFactory, ObservableExecutionResult } from '../../client/common/process/types'; +import { anything, capture, instance, mock, when } from 'ts-mockito'; +import { KernelDaemonPool } from '../../client/datascience/kernel-launcher/kernelDaemonPool'; +import { KernelSpecConnectionMetadata } from '../../client/datascience/jupyter/kernels/types'; +import { IFileSystem } from '../../client/common/platform/types'; +import { KernelEnvironmentVariablesService } from '../../client/datascience/kernel-launcher/kernelEnvVarsService'; +import { KernelProcess } from '../../client/datascience/kernel-launcher/kernelProcess'; +import { IPythonExtensionChecker } from '../../client/api/types'; +import { noop } from '../core'; +import { PythonKernelLauncherDaemon } from '../../client/datascience/kernel-launcher/kernelLauncherDaemon'; +import { EventEmitter } from 'events'; +import { disposeAllDisposables } from '../../client/common/helpers'; +import { traceInfo } from '../../client/common/logger'; + +suite('DataScience - Kernel Process', () => { + let processService: IProcessService; + const disposables: IDisposable[] = []; + suiteSetup(async function () { + // These are slow tests, hence lets run only on linux on CI. + if (IS_REMOTE_NATIVE_TEST) { + return this.skip(); + } + rewiremock.disable(); + sinon.restore(); + }); + suiteTeardown(async function () { + rewiremock.disable(); + sinon.restore(); + }); + + // setup(async function () { + // traceInfo(`Start Test ${this.currentTest?.title}`); + // }); + teardown(function () { + rewiremock.disable(); + sinon.restore(); + traceInfo(`End Test Complete ${this.currentTest?.title}`); + disposeAllDisposables(disposables); + }); + + function launchKernel(metadata: KernelSpecConnectionMetadata, connectionFile: string) { + const processExecutionFactory = mock(); + const daemonPool = mock(); + const connection = mock(); + const fs = mock(); + const extensionChecker = mock(); + const kernelEnvVarsService = mock(); + processService = mock(); + const instanceOfExecutionService = instance(processService); + (instanceOfExecutionService as any).then = undefined; + const observableProc: ObservableExecutionResult = { + dispose: noop, + out: { subscribe: noop } as any, + proc: { + stdout: new EventEmitter(), + stderr: new EventEmitter(), + subscribe: noop, + on: noop + } as any + }; + when(processExecutionFactory.create(anything())).thenResolve(instanceOfExecutionService); + when(fs.createTemporaryLocalFile(anything())).thenResolve({ dispose: noop, filePath: connectionFile }); + when(fs.writeFile(anything(), anything())).thenResolve(); + when(kernelEnvVarsService.getEnvironmentVariables(anything(), anything(), anything())).thenResolve(process.env); + when(processService.execObservable(anything(), anything(), anything())).thenReturn(observableProc); + sinon.stub(PythonKernelLauncherDaemon.prototype, 'launch'); + rewiremock.enable(); + rewiremock('tcp-port-used').with({ waitUntilUsed: () => Promise.resolve() }); + return new KernelProcess( + instance(processExecutionFactory), + instance(daemonPool), + instance(connection), + metadata, + instance(fs), + undefined, + instance(extensionChecker), + instance(kernelEnvVarsService) + ); + } + test('Launch from kernelspec (linux)', async function () { + const metadata: KernelSpecConnectionMetadata = { + id: '1', + kernelSpec: { + argv: [ + '/Library/Java/JavaVirtualMachines/adoptopenjdk-11.jdk/Contents/Home/bin/java', + '--add-opens', + 'java.base/jdk.internal.misc=ALL-UNNAMED', + '--illegal-access=permit', + '-Djava.awt.headless=true', + '-Djdk.disableLastUsageTracking=true', + '-Dmaven.repo.local=/Users/jdoe/Notebooks/.venv/share/jupyter/repository', + '-jar', + '/Users/jdoe/.m2/repository/ganymede/ganymede/2.0.0-SNAPSHOT/ganymede-2.0.0-SNAPSHOT.jar', + '--connection-file={connection_file}' + ], + language: 'java', + interrupt_mode: 'message', + display_name: '', + name: '', + path: '' + }, + kind: 'startUsingKernelSpec' + }; + const kernelProcess = launchKernel(metadata, 'wow/connection_config.json'); + await kernelProcess.launch('', 10_000); + const args = capture(processService.execObservable).first(); + + assert.strictEqual(args[0], metadata.kernelSpec.argv[0]); + assert.deepStrictEqual( + args[1], + metadata.kernelSpec.argv + .slice(1, metadata.kernelSpec.argv.length - 1) + .concat('--connection-file=wow/connection_config.json') + ); + await kernelProcess.dispose(); + }); + test('Launch from kernelspec (linux with space in file name)', async function () { + const metadata: KernelSpecConnectionMetadata = { + id: '1', + kernelSpec: { + argv: [ + '/Library/Java/JavaVirtualMachines/adoptopenjdk-11.jdk/Contents/Home/bin/java', + '--add-opens', + 'java.base/jdk.internal.misc=ALL-UNNAMED', + '--illegal-access=permit', + '-Djava.awt.headless=true', + '-Djdk.disableLastUsageTracking=true', + '-Dmaven.repo.local=/Users/jdoe/Notebooks/.venv/share/jupyter/repository', + '-jar', + '/Users/jdoe/.m2/repository/ganymede/ganymede/2.0.0-SNAPSHOT/ganymede-2.0.0-SNAPSHOT.jar', + '--connection-file={connection_file}' + ], + language: 'java', + interrupt_mode: 'message', + display_name: '', + name: '', + path: '' + }, + kind: 'startUsingKernelSpec' + }; + const kernelProcess = launchKernel(metadata, 'wow/connection config.json'); + await kernelProcess.launch('', 10_000); + const args = capture(processService.execObservable).first(); + + assert.strictEqual(args[0], metadata.kernelSpec.argv[0]); + assert.deepStrictEqual( + args[1], + metadata.kernelSpec.argv + .slice(1, metadata.kernelSpec.argv.length - 1) + .concat('--connection-file="wow/connection config.json"') + ); + await kernelProcess.dispose(); + }); + test('Launch from kernelspec (windows)', async function () { + const metadata: KernelSpecConnectionMetadata = { + id: '1', + kernelSpec: { + argv: [ + 'C:\\Program Files\\AdoptOpenJDK\\jdk-16.0.1.9-hotspot\\bin\\java.exe', + '--illegal-access=permit', + '--add-opens', + 'java.base/jdk.internal.misc=ALL-UNNAMED', + '-jar', + 'C:\\Users\\abc\\AppData\\Roaming\\jupyter\\kernels\\ganymede-1.1.0.20210614-java-16kernel.jar', + '--runtime-dir=C:\\Users\\abc\\AppData\\Roaming\\jupyter\\runtime', + '--connection-file={connection_file}' + ], + language: 'java', + interrupt_mode: 'message', + display_name: '', + name: '', + path: '' + }, + kind: 'startUsingKernelSpec' + }; + const kernelProcess = launchKernel(metadata, 'connection_config.json'); + await kernelProcess.launch('', 10_000); + const args = capture(processService.execObservable).first(); + + assert.strictEqual(args[0], metadata.kernelSpec.argv[0]); + assert.deepStrictEqual( + args[1], + metadata.kernelSpec.argv + .slice(1, metadata.kernelSpec.argv.length - 1) + .concat('--connection-file=connection_config.json') + ); + await kernelProcess.dispose(); + }); + test('Launch from kernelspec (windows with space in file name)', async function () { + const metadata: KernelSpecConnectionMetadata = { + id: '1', + kernelSpec: { + argv: [ + 'C:\\Program Files\\AdoptOpenJDK\\jdk-16.0.1.9-hotspot\\bin\\java.exe', + '--illegal-access=permit', + '--add-opens', + 'java.base/jdk.internal.misc=ALL-UNNAMED', + '-jar', + 'C:\\Users\\abc\\AppData\\Roaming\\jupyter\\kernels\\ganymede-1.1.0.20210614-java-16kernel.jar', + '--runtime-dir=C:\\Users\\abc\\AppData\\Roaming\\jupyter\\runtime', + '--connection-file={connection_file}' + ], + language: 'java', + interrupt_mode: 'message', + display_name: '', + name: '', + path: '' + }, + kind: 'startUsingKernelSpec' + }; + const kernelProcess = launchKernel(metadata, 'D:\\hello\\connection config.json'); + await kernelProcess.launch('', 10_000); + const args = capture(processService.execObservable).first(); + + assert.strictEqual(args[0], metadata.kernelSpec.argv[0]); + assert.deepStrictEqual( + args[1], + metadata.kernelSpec.argv + .slice(1, metadata.kernelSpec.argv.length - 1) + .concat('--connection-file="D:\\hello\\connection config.json"') + ); + await kernelProcess.dispose(); + }); +});