Support kernelspec argv containing non traditional args for `{connect…

…ion_file}`

news/2 Fixes/7203.md

@@ -0,0 +1 @@
+Support kernelspec argv containing non traditional args for `{connection_file}`.

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<IJupyterKernelSpec>): number {
-    return kernelSpec.argv.indexOf(connectionFilePlaceholder);
+    return kernelSpec.argv.findIndex((arg) => arg.includes(connectionFilePlaceholder));
 }
 
 type ConnectionWithKernelSpec =

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}"`;
                 }

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<IProcessServiceFactory>();
+        const daemonPool = mock<KernelDaemonPool>();
+        const connection = mock<IKernelConnection>();
+        const fs = mock<IFileSystem>();
+        const extensionChecker = mock<IPythonExtensionChecker>();
+        const kernelEnvVarsService = mock<KernelEnvironmentVariablesService>();
+        processService = mock<IProcessService>();
+        const instanceOfExecutionService = instance(processService);
+        (instanceOfExecutionService as any).then = undefined;
+        const observableProc: ObservableExecutionResult<string> = {
+            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();
+    });
+});