Skip to content

Commit

Permalink
Fix problems with launching non zmq on windows (#10944)
Browse files Browse the repository at this point in the history
* Fix jupyter launch on windows

* Add news entry
  • Loading branch information
rchiodo authored Jul 27, 2022
1 parent 087ea90 commit 69fc357
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 10 deletions.
1 change: 1 addition & 0 deletions news/2 Fixes/10940.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixes problem with starting a kernel when ZMQ wasn't supported on Windows.
11 changes: 10 additions & 1 deletion src/kernels/raw/launcher/kernelEnvVarsService.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,16 @@ export class KernelEnvironmentVariablesService {
}
// Merge the env variables with that of the kernel env.
interpreterEnv = interpreterEnv || {};
const mergedVars = { ...process.env };
let mergedVars = { ...process.env };

// On windows (see https://github.com/microsoft/vscode-jupyter/issues/10940)
// upper case all of the keys
if (process.platform === 'win32') {
mergedVars = {};
Object.keys(process.env).forEach((k) => {
mergedVars[k.toUpperCase()] = process.env[k];
});
}
kernelEnv = kernelEnv || {};
customEnvVars = customEnvVars || {};
this.envVarsService.mergeVariables(interpreterEnv, mergedVars); // interpreter vars win over proc.
Expand Down
44 changes: 37 additions & 7 deletions src/test/common/variables/kernelEnvVarsService.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@ suite('Kernel Environment Variables Service', () => {
sysPrefix: '0'
};
let kernelSpec: IJupyterKernelSpec;
let processEnv: NodeJS.ProcessEnv;
const originalEnvVars = Object.assign({}, process.env);
const processPath = Object.keys(process.env).find((k) => k.toLowerCase() == 'path');
let processPath: string | undefined;
setup(() => {
kernelSpec = {
name: 'kernel',
Expand All @@ -64,6 +65,17 @@ suite('Kernel Environment Variables Service', () => {
instance(customVariablesService),
instance(configService)
);
if (process.platform === 'win32') {
// Win32 will generate upper case all the time
const entries = Object.entries(process.env);
processEnv = {};
for (const [key, value] of entries) {
processEnv[key.toUpperCase()] = value;
}
} else {
processEnv = process.env;
}
processPath = Object.keys(processEnv).find((k) => k.toLowerCase() == 'path');
});
teardown(() => Object.assign(process.env, originalEnvVars));

Expand Down Expand Up @@ -91,7 +103,7 @@ suite('Kernel Environment Variables Service', () => {
// Compare ignoring the PATH variable.
assert.deepEqual(
Object.assign(vars, { PATH: '', Path: '' }),
Object.assign({}, process.env, { HELLO_VAR: 'new' }, { PATH: '', Path: '' })
Object.assign({}, processEnv, { HELLO_VAR: 'new' }, { PATH: '', Path: '' })
);
});

Expand All @@ -110,7 +122,7 @@ suite('Kernel Environment Variables Service', () => {
// Compare ignoring the PATH variable.
assert.deepEqual(
Object.assign(vars, { PATH: '', Path: '' }),
Object.assign({}, process.env, { HELLO_VAR: 'new' }, { PATH: '', Path: '' })
Object.assign({}, processEnv, { HELLO_VAR: 'new' }, { PATH: '', Path: '' })
);
});

Expand All @@ -128,7 +140,7 @@ suite('Kernel Environment Variables Service', () => {
// Compare ignoring the PATH variable.
assert.deepEqual(
Object.assign(vars, { PATH: '', Path: '' }),
Object.assign({}, process.env, { HELLO_VAR: 'new' }, { PATH: '', Path: '' })
Object.assign({}, processEnv, { HELLO_VAR: 'new' }, { PATH: '', Path: '' })
);
});

Expand All @@ -138,7 +150,7 @@ suite('Kernel Environment Variables Service', () => {

const vars = await kernelVariablesService.getEnvironmentVariables(undefined, undefined, kernelSpec);

assert.deepEqual(vars, process.env);
assert.deepEqual(vars, processEnv);
});

test('Returns process.env vars if unable to get activated vars for interpreter and no kernelspec.env', async () => {
Expand All @@ -150,11 +162,11 @@ suite('Kernel Environment Variables Service', () => {
// Compare ignoring the PATH variable.
assert.deepEqual(
Object.assign({}, vars, { PATH: '', Path: '' }),
Object.assign({}, process.env, { PATH: '', Path: '' })
Object.assign({}, processEnv, { PATH: '', Path: '' })
);
assert.strictEqual(
vars![processPath!],
`${path.dirname(interpreter.uri.fsPath)}${path.delimiter}${process.env[processPath!]}`
`${path.dirname(interpreter.uri.fsPath)}${path.delimiter}${processEnv[processPath!]}`
);
});

Expand All @@ -174,6 +186,24 @@ suite('Kernel Environment Variables Service', () => {
);
});

test('Upper case is used on windows', async function () {
// See this issue as to what happens if it isn't. https://github.com/microsoft/vscode-jupyter/issues/10940
if (process.platform !== 'win32') {
this.skip();
}
when(envActivation.getActivatedEnvironmentVariables(anything(), anything(), anything())).thenResolve({
path: 'foobar'
});
when(customVariablesService.getCustomEnvironmentVariables(anything(), anything())).thenResolve({
path: 'foobaz'
});

const vars = await kernelVariablesService.getEnvironmentVariables(undefined, interpreter, kernelSpec);
const keys = Object.keys(vars);
const upperCaseKeys = keys.map((key) => key.toUpperCase());
assert.deepEqual(keys, upperCaseKeys);
});

test('KernelSpec interpreterPath used if interpreter is undefined', async () => {
when(interpreterService.getInterpreterDetails(anything())).thenResolve({
envType: EnvironmentType.Conda,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ suite('DataScience - JupyterKernelService', () => {
let settings: IWatchableJupyterSettings;
let fs: IFileSystemNode;
let testWorkspaceFolder: Uri;
// eslint-disable-next-line local-rules/dont-use-process
const pathVariable = Object.keys(process.env).find((k) => k.toLowerCase() == 'path')!;
// PATH variable is forced upper case on Windows
const pathVariable =
// eslint-disable-next-line local-rules/dont-use-process
process.platform === 'win32' ? 'PATH' : Object.keys(process.env).find((k) => k.toLowerCase() == 'path')!;

// Set of kernels. Generated this by running the localKernelFinder unit test and stringifying
// the results returned.
Expand Down

0 comments on commit 69fc357

Please sign in to comment.