Skip to content

Enhance additionalPowerShellExes setting #4686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 8, 2023
Merged

Enhance additionalPowerShellExes setting #4686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d7b0ef
Enhance `additionalPowerShellExes` setting
andyleejordan Aug 4, 2023
d32d1bb
Also strip quote pairs from `cwd` setting
andyleejordan Aug 4, 2023
350eaff
Add `powerShellAdditionalExePaths` unit tests
andyleejordan Aug 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,11 @@
"markdownDescription": "**Deprecated:** Specifies whether you should be prompted to update your version of `PackageManagement` if it's under 1.4.6.",
"markdownDeprecationMessage": "**Deprecated:** This prompt has been removed as it's no longer strictly necessary to upgrade the `PackageManagement` module."
},
"powershell.suppressAdditionalExeNotFoundWarning": {
"type": "boolean",
"default": false,
"markdownDescription": "Suppresses the warning message when any of `#powershell.powerShellAdditionalExePaths#` is not found."
},
"powershell.startAsLoginShell.osx": {
"type": "boolean",
"default": true,
Expand Down
49 changes: 42 additions & 7 deletions src/platform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@
import * as os from "os";
import * as path from "path";
import * as process from "process";
import vscode = require("vscode");
import { integer } from "vscode-languageserver-protocol";
import { ILogger } from "./logging";
import { PowerShellAdditionalExePathSettings } from "./settings";
import { changeSetting, getSettings, PowerShellAdditionalExePathSettings } from "./settings";

// This uses require so we can rewire it in unit tests!
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-var-requires
Expand Down Expand Up @@ -149,8 +150,16 @@ export class PowerShellExeFinder {
for (const additionalPwsh of this.enumerateAdditionalPowerShellInstallations()) {
if (await additionalPwsh.exists()) {
yield additionalPwsh;
} else {
void this.logger.writeAndShowWarning(`Additional PowerShell '${additionalPwsh.displayName}' not found at '${additionalPwsh.exePath}'!`);
} else if (!additionalPwsh.suppressWarning) {
const message = `Additional PowerShell '${additionalPwsh.displayName}' not found at '${additionalPwsh.exePath}'!`;
this.logger.writeWarning(message);

if (!getSettings().suppressAdditionalExeNotFoundWarning) {
const selection = await vscode.window.showWarningMessage(message, "Don't Show Again");
if (selection !== undefined) {
await changeSetting("suppressAdditionalExeNotFoundWarning", true, true, this.logger);
}
}
}
}
}
Expand Down Expand Up @@ -223,9 +232,33 @@ export class PowerShellExeFinder {
private *enumerateAdditionalPowerShellInstallations(): Iterable<IPossiblePowerShellExe> {
for (const versionName in this.additionalPowerShellExes) {
if (Object.prototype.hasOwnProperty.call(this.additionalPowerShellExes, versionName)) {
const exePath = this.additionalPowerShellExes[versionName];
if (exePath) {
yield new PossiblePowerShellExe(exePath, versionName);
const exePath = utils.stripQuotePair(this.additionalPowerShellExes[versionName]);
if (!exePath) {
continue;
}

// Always search for what the user gave us first
yield new PossiblePowerShellExe(exePath, versionName);

// Also search for `pwsh[.exe]` and `powershell[.exe]` if missing
const args: [string, undefined, boolean, boolean]
// Must be a tuple type and is suppressing the warning
= [versionName, undefined, true, true];

// Handle Windows where '.exe' and 'powershell' are things
if (this.platformDetails.operatingSystem === OperatingSystem.Windows) {
if (!exePath.endsWith("pwsh.exe") && !exePath.endsWith("powershell.exe")) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree probably need some tests to validate all these potential permutations, looks good to me though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized last night I should also be able to handle the common case of ~/, and share that and the quote-stripping with the cwd setting, and then do a preview and then work on saving the cwd in a cache instead of a setting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so I decided that since the Python extension uses untildify I'm fine bringing it too, which means I'll get that taken care of...in the next PR. @JustinGrote could you give this a review? It's got tests now!

if (exePath.endsWith("pwsh") || exePath.endsWith("powershell")) {
// Add extension if that was missing
yield new PossiblePowerShellExe(exePath + ".exe", ...args);
}
// Also add full exe names (this isn't an else just in case
// the folder was named "pwsh" or "powershell")
yield new PossiblePowerShellExe(path.join(exePath, "pwsh.exe"), ...args);
yield new PossiblePowerShellExe(path.join(exePath, "powershell.exe"), ...args);
}
} else if (!exePath.endsWith("pwsh")) { // Always just 'pwsh' on non-Windows
yield new PossiblePowerShellExe(path.join(exePath, "pwsh"), ...args);
}
}
}
Expand Down Expand Up @@ -529,14 +562,16 @@ export function getWindowsSystemPowerShellPath(systemFolderName: string): string

interface IPossiblePowerShellExe extends IPowerShellExeDetails {
exists(): Promise<boolean>;
readonly suppressWarning: boolean;
}

class PossiblePowerShellExe implements IPossiblePowerShellExe {
constructor(
public readonly exePath: string,
public readonly displayName: string,
private knownToExist?: boolean,
public readonly supportsProperArguments = true) { }
public readonly supportsProperArguments = true,
public readonly suppressWarning = false) { }

public async exists(): Promise<boolean> {
if (this.knownToExist === undefined) {
Expand Down
4 changes: 3 additions & 1 deletion src/settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export class Settings extends PartialSettings {
// This setting is no longer used but is here to assist in cleaning up the users settings.
powerShellExePath = "";
promptToUpdatePowerShell = true;
suppressAdditionalExeNotFoundWarning = false;
startAsLoginShell = new StartAsLoginShellSettings();
startAutomatically = true;
enableProfileLoading = true;
Expand Down Expand Up @@ -216,7 +217,8 @@ let hasPrompted = false;
export let chosenWorkspace: vscode.WorkspaceFolder | undefined = undefined;

export async function validateCwdSetting(logger: ILogger): Promise<string> {
let cwd: string | undefined = vscode.workspace.getConfiguration(utils.PowerShellLanguageId).get<string>("cwd");
let cwd: string | undefined = utils.stripQuotePair(
vscode.workspace.getConfiguration(utils.PowerShellLanguageId).get<string>("cwd"));

// Only use the cwd setting if it exists.
if (cwd !== undefined && await utils.checkIfDirectoryExists(cwd)) {
Expand Down
14 changes: 14 additions & 0 deletions src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,20 @@ export function escapeSingleQuotes(p: string): string {
return p.replace(new RegExp("'", "g"), "''");
}

export function stripQuotePair(p: string | undefined): string | undefined {
if (p === undefined) {
return p;
}

// Remove matching surrounding quotes from p (without regex)
if (p.startsWith("'") && p.endsWith("'")
|| p.startsWith("\"") && p.endsWith("\"")) {
return p.slice(1, -1);
}

return p;
}

export function getPipePath(pipeName: string): string {
if (os.platform() === "win32") {
return "\\\\.\\pipe\\" + pipeName;
Expand Down
188 changes: 187 additions & 1 deletion test/core/platform.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import * as sinon from "sinon";
import * as platform from "../../src/platform";
import * as fs from "fs"; // NOTE: Necessary for mock-fs.
import * as vscode from "vscode";
import { stripQuotePair } from "../../src/utils";

// We have to rewire the platform module so that mock-fs can be used, as it
// overrides the fs module but not the vscode.workspace.fs module.
Expand All @@ -34,7 +35,8 @@ async function fakeReadDirectory(targetPath: string | vscode.Uri): Promise<strin
const utilsMock = {
checkIfFileExists: fakeCheckIfFileOrDirectoryExists,
checkIfDirectoryExists: fakeCheckIfFileOrDirectoryExists,
readDirectory: fakeReadDirectory
readDirectory: fakeReadDirectory,
stripQuotePair: stripQuotePair
};

platformMock.__set__("utils", utilsMock);
Expand Down Expand Up @@ -62,6 +64,9 @@ interface ITestPlatformSuccessCase extends ITestPlatform {
// Platform configurations where we expect to find a set of PowerShells
let successTestCases: ITestPlatformSuccessCase[];

// Platform configurations for testing the powerShellAdditionalExePaths setting
let additionalPowerShellExes: Record<string, string>;
let successAdditionalTestCases: ITestPlatformSuccessCase[];

if (process.platform === "win32") {
const msixAppDir = path.join(process.env.LOCALAPPDATA!, "Microsoft", "WindowsApps");
Expand Down Expand Up @@ -443,8 +448,105 @@ if (process.platform === "win32") {
},
},
];

additionalPowerShellExes = {
"pwsh": "C:\\Users\\test\\pwsh\\pwsh.exe",
"pwsh-no-exe": "C:\\Users\\test\\pwsh\\pwsh",
"pwsh-folder": "C:\\Users\\test\\pwsh\\",
"pwsh-folder-no-slash": "C:\\Users\\test\\pwsh",
"pwsh-single-quotes": "'C:\\Users\\test\\pwsh\\pwsh.exe'",
"pwsh-double-quotes": "\"C:\\Users\\test\\pwsh\\pwsh.exe\"",
};

successAdditionalTestCases = [
{
name: "Windows (Additional PowerShell Executables)",
platformDetails: {
operatingSystem: platform.OperatingSystem.Windows,
isOS64Bit: true,
isProcess64Bit: true,
},
environmentVars: {
"USERPROFILE": "C:\\Users\\test",
},
expectedPowerShellSequence: [
{
exePath: "C:\\Users\\test\\pwsh\\pwsh.exe",
displayName: "pwsh",
supportsProperArguments: true
},
{
exePath: "C:\\Users\\test\\pwsh\\pwsh",
displayName: "pwsh-no-exe",
supportsProperArguments: true
},
{
exePath: "C:\\Users\\test\\pwsh\\pwsh.exe",
displayName: "pwsh-no-exe",
supportsProperArguments: true
},
{
exePath: "C:\\Users\\test\\pwsh\\pwsh\\pwsh.exe",
displayName: "pwsh-no-exe",
supportsProperArguments: true
},
{
exePath: "C:\\Users\\test\\pwsh\\pwsh\\powershell.exe",
displayName: "pwsh-no-exe",
supportsProperArguments: true
},
{
exePath: "C:\\Users\\test\\pwsh\\",
displayName: "pwsh-folder",
supportsProperArguments: true
},
{
exePath: "C:\\Users\\test\\pwsh\\pwsh.exe",
displayName: "pwsh-folder",
supportsProperArguments: true
},
{
exePath: "C:\\Users\\test\\pwsh\\powershell.exe",
displayName: "pwsh-folder",
supportsProperArguments: true
},
{
exePath: "C:\\Users\\test\\pwsh",
displayName: "pwsh-folder-no-slash",
supportsProperArguments: true
},
{
exePath: "C:\\Users\\test\\pwsh.exe",
displayName: "pwsh-folder-no-slash",
supportsProperArguments: true
},
{
exePath: "C:\\Users\\test\\pwsh\\pwsh.exe",
displayName: "pwsh-folder-no-slash",
supportsProperArguments: true
},
{
exePath: "C:\\Users\\test\\pwsh\\powershell.exe",
displayName: "pwsh-folder-no-slash",
supportsProperArguments: true
},
{
exePath: "C:\\Users\\test\\pwsh\\pwsh.exe",
displayName: "pwsh-single-quotes",
supportsProperArguments: true
},
{
exePath: "C:\\Users\\test\\pwsh\\pwsh.exe",
displayName: "pwsh-double-quotes",
supportsProperArguments: true
},
],
filesystem: {},
}
];
} else {
const pwshDailyDir = path.join(os.homedir(), ".powershell-daily");

successTestCases = [
{
name: "Linux (all installations)",
Expand Down Expand Up @@ -642,6 +744,66 @@ if (process.platform === "win32") {
},
},
];

additionalPowerShellExes = {
"pwsh": "/home/bin/pwsh",
"pwsh-folder": "/home/bin/",
"pwsh-folder-no-slash": "/home/bin",
"pwsh-single-quotes": "'/home/bin/pwsh'",
"pwsh-double-quotes": "\"/home/bin/pwsh\"",
};

successAdditionalTestCases = [
{ // Also sufficient for macOS as the behavior is the same
name: "Linux (Additional PowerShell Executables)",
platformDetails: {
operatingSystem: platform.OperatingSystem.Linux,
isOS64Bit: true,
isProcess64Bit: true,
},
environmentVars: {
"HOME": "/home/test",
},
expectedPowerShellSequence: [
{
exePath: "/home/bin/pwsh",
displayName: "pwsh",
supportsProperArguments: true
},
{
exePath: "/home/bin/",
displayName: "pwsh-folder",
supportsProperArguments: true
},
{
exePath: "/home/bin/pwsh",
displayName: "pwsh-folder",
supportsProperArguments: true
},
{
exePath: "/home/bin",
displayName: "pwsh-folder-no-slash",
supportsProperArguments: true
},
{
exePath: "/home/bin/pwsh",
displayName: "pwsh-folder-no-slash",
supportsProperArguments: true
},
{
exePath: "/home/bin/pwsh",
displayName: "pwsh-single-quotes",
supportsProperArguments: true
},
{
exePath: "/home/bin/pwsh",
displayName: "pwsh-double-quotes",
supportsProperArguments: true
},
],
filesystem: {},
}
];
}

const errorTestCases: ITestPlatform[] = [
Expand Down Expand Up @@ -846,4 +1008,28 @@ describe("Platform module", function () {
});
}
});

describe("PowerShell executables from 'powerShellAdditionalExePaths' are found", function () {
afterEach(function () {
sinon.restore();
mockFS.restore();
});

for (const testPlatform of successAdditionalTestCases) {
it(`Guesses for ${testPlatform.name}`, function () {
setupTestEnvironment(testPlatform);

const powerShellExeFinder = new platformMock.PowerShellExeFinder(testPlatform.platformDetails, additionalPowerShellExes);

let i = 0;
for (const additionalPwsh of powerShellExeFinder.enumerateAdditionalPowerShellInstallations()) {
const expectedPowerShell = testPlatform.expectedPowerShellSequence[i];
i++;

assert.strictEqual(additionalPwsh.exePath, expectedPowerShell.exePath);
assert.strictEqual(additionalPwsh.displayName, expectedPowerShell.displayName);
}
});
}
});
});