From abb5ef65f045265a4e223031198960f22eda1399 Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Fri, 4 Aug 2023 19:03:18 -0700 Subject: [PATCH 1/8] Use untildify for ~ in `cwd` and `additionalPowerShellExes` This makes them way less annoying. I'm ok taking a dependency on untildify as it's a very simple package, and the Python extension for VS Code also uses it. However, it must remain at v4.0.0, as the latest version, v5.0.0, is pure ESM and therefore cannot be loaded by VS Code. https://github.com/sindresorhus/untildify/releases/tag/v5.0.0 --- .github/dependabot.yml | 3 +++ package-lock.json | 14 ++++++++++++++ package.json | 1 + src/platform.ts | 5 ++++- src/settings.ts | 8 ++++++-- test/core/platform.test.ts | 20 ++++++++++++++------ 6 files changed, 42 insertions(+), 9 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index cd5194b60f..d66963a303 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -11,6 +11,9 @@ updates: - "esbuild" - "eslint" - "@typescript-eslint/*" + ignore: + - dependency-name: "untildify" + versions: ["5.x"] - package-ecosystem: github-actions directory: "/" schedule: diff --git a/package-lock.json b/package-lock.json index e42425ec1f..db832984b7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -12,6 +12,7 @@ "@vscode/extension-telemetry": "0.8.2", "node-fetch": "2.6.12", "semver": "7.5.4", + "untildify": "4.0.0", "uuid": "9.0.0", "vscode-languageclient": "8.1.0", "vscode-languageserver-protocol": "3.17.3" @@ -5318,6 +5319,14 @@ "integrity": "sha512-+A5Sja4HP1M08MaXya7p5LvjuM7K6q/2EaC0+iovj/wOcMsTzMvDFbasi/oSapiwOlt252IqsKqPjCl7huKS0A==", "dev": true }, + "node_modules/untildify": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/untildify/-/untildify-4.0.0.tgz", + "integrity": "sha512-KK8xQ1mkzZeg9inewmFVDNkg3l5LUhoq9kN6iWYB/CC9YMG8HA+c1Q8HwDe6dEX7kErrEVNVBO3fWsVq5iDgtw==", + "engines": { + "node": ">=8" + } + }, "node_modules/uri-js": { "version": "4.4.1", "resolved": "https://registry.npmjs.org/uri-js/-/uri-js-4.4.1.tgz", @@ -9469,6 +9478,11 @@ "integrity": "sha512-+A5Sja4HP1M08MaXya7p5LvjuM7K6q/2EaC0+iovj/wOcMsTzMvDFbasi/oSapiwOlt252IqsKqPjCl7huKS0A==", "dev": true }, + "untildify": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/untildify/-/untildify-4.0.0.tgz", + "integrity": "sha512-KK8xQ1mkzZeg9inewmFVDNkg3l5LUhoq9kN6iWYB/CC9YMG8HA+c1Q8HwDe6dEX7kErrEVNVBO3fWsVq5iDgtw==" + }, "uri-js": { "version": "4.4.1", "resolved": "https://registry.npmjs.org/uri-js/-/uri-js-4.4.1.tgz", diff --git a/package.json b/package.json index e568f476eb..4ab97c8055 100644 --- a/package.json +++ b/package.json @@ -77,6 +77,7 @@ "@vscode/extension-telemetry": "0.8.2", "node-fetch": "2.6.12", "semver": "7.5.4", + "untildify": "4.0.0", "uuid": "9.0.0", "vscode-languageclient": "8.1.0", "vscode-languageserver-protocol": "3.17.3" diff --git a/src/platform.ts b/src/platform.ts index b5a790c2e3..415c777d2d 100644 --- a/src/platform.ts +++ b/src/platform.ts @@ -8,6 +8,7 @@ import vscode = require("vscode"); import { integer } from "vscode-languageserver-protocol"; import { ILogger } from "./logging"; import { changeSetting, getSettings, PowerShellAdditionalExePathSettings } from "./settings"; +import untildify from "untildify"; // 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 @@ -232,11 +233,13 @@ export class PowerShellExeFinder { private *enumerateAdditionalPowerShellInstallations(): Iterable { for (const versionName in this.additionalPowerShellExes) { if (Object.prototype.hasOwnProperty.call(this.additionalPowerShellExes, versionName)) { - const exePath = utils.stripQuotePair(this.additionalPowerShellExes[versionName]); + let exePath = utils.stripQuotePair(this.additionalPowerShellExes[versionName]); if (!exePath) { continue; } + exePath = untildify(exePath); + // Always search for what the user gave us first yield new PossiblePowerShellExe(exePath, versionName); diff --git a/src/settings.ts b/src/settings.ts index 3e200227dd..8687ea997c 100644 --- a/src/settings.ts +++ b/src/settings.ts @@ -5,6 +5,7 @@ import vscode = require("vscode"); import utils = require("./utils"); import os = require("os"); import { ILogger } from "./logging"; +import untildify from "untildify"; // TODO: Quite a few of these settings are unused in the client and instead // exist just for the server. Those settings do not need to be represented in @@ -221,8 +222,11 @@ export async function validateCwdSetting(logger: ILogger): Promise { vscode.workspace.getConfiguration(utils.PowerShellLanguageId).get("cwd")); // Only use the cwd setting if it exists. - if (cwd !== undefined && await utils.checkIfDirectoryExists(cwd)) { - return cwd; + if (cwd !== undefined) { + cwd = untildify(cwd); + if (await utils.checkIfDirectoryExists(cwd)) { + return cwd; + } } // If there is no workspace, or there is but it has no folders, fallback. diff --git a/test/core/platform.test.ts b/test/core/platform.test.ts index b5c082ec57..c8d05bacd6 100644 --- a/test/core/platform.test.ts +++ b/test/core/platform.test.ts @@ -451,6 +451,7 @@ if (process.platform === "win32") { additionalPowerShellExes = { "pwsh": "C:\\Users\\test\\pwsh\\pwsh.exe", + "pwsh-tilde": "~\\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", @@ -466,15 +467,18 @@ if (process.platform === "win32") { isOS64Bit: true, isProcess64Bit: true, }, - environmentVars: { - "USERPROFILE": "C:\\Users\\test", - }, + environmentVars: {}, expectedPowerShellSequence: [ { exePath: "C:\\Users\\test\\pwsh\\pwsh.exe", displayName: "pwsh", supportsProperArguments: true }, + { + exePath: path.join(os.homedir(), "pwsh", "pwsh.exe"), + displayName: "pwsh-tilde", + supportsProperArguments: true + }, { exePath: "C:\\Users\\test\\pwsh\\pwsh", displayName: "pwsh-no-exe", @@ -747,6 +751,7 @@ if (process.platform === "win32") { additionalPowerShellExes = { "pwsh": "/home/bin/pwsh", + "pwsh-tilde": "~/bin/pwsh", "pwsh-folder": "/home/bin/", "pwsh-folder-no-slash": "/home/bin", "pwsh-single-quotes": "'/home/bin/pwsh'", @@ -761,15 +766,18 @@ if (process.platform === "win32") { isOS64Bit: true, isProcess64Bit: true, }, - environmentVars: { - "HOME": "/home/test", - }, + environmentVars: {}, expectedPowerShellSequence: [ { exePath: "/home/bin/pwsh", displayName: "pwsh", supportsProperArguments: true }, + { + exePath: path.join(os.homedir(), "bin", "pwsh"), + displayName: "pwsh-tilde", + supportsProperArguments: true + }, { exePath: "/home/bin/", displayName: "pwsh-folder", From 0094d35b1b4de976fba938738220d8587b8e8a40 Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Fri, 4 Aug 2023 19:39:18 -0700 Subject: [PATCH 2/8] Add tests for `validateCwdSetting` Those were overdue! --- src/settings.ts | 2 +- test/core/settings.test.ts | 81 ++++++++++++++++++++++++++++++-------- 2 files changed, 65 insertions(+), 18 deletions(-) diff --git a/src/settings.ts b/src/settings.ts index 8687ea997c..b718008c08 100644 --- a/src/settings.ts +++ b/src/settings.ts @@ -217,7 +217,7 @@ export async function changeSetting( let hasPrompted = false; export let chosenWorkspace: vscode.WorkspaceFolder | undefined = undefined; -export async function validateCwdSetting(logger: ILogger): Promise { +export async function validateCwdSetting(logger: ILogger | undefined): Promise { let cwd: string | undefined = utils.stripQuotePair( vscode.workspace.getConfiguration(utils.PowerShellLanguageId).get("cwd")); diff --git a/test/core/settings.test.ts b/test/core/settings.test.ts index 082f4cc626..7ad9c26b6e 100644 --- a/test/core/settings.test.ts +++ b/test/core/settings.test.ts @@ -2,32 +2,79 @@ // Licensed under the MIT License. import * as assert from "assert"; +import * as os from "os"; import * as vscode from "vscode"; -import { Settings, getSettings, getEffectiveConfigurationTarget, changeSetting, CommentType } from "../../src/settings"; +import { + Settings, + getSettings, + getEffectiveConfigurationTarget, + changeSetting, + CommentType, + validateCwdSetting +} from "../../src/settings"; +import path from "path"; describe("Settings E2E", function () { - it("Loads without error", function () { - assert.doesNotThrow(getSettings); + describe("The 'getSettings' method loads the 'Settings' class", function () { + it("Loads without error", function () { + assert.doesNotThrow(getSettings); + }); + + it("Loads the correct defaults", function () { + const testSettings = new Settings(); + const actualSettings = getSettings(); + assert.deepStrictEqual(actualSettings, testSettings); + }); }); - it("Loads the correct defaults", function () { - const testSettings = new Settings(); - const actualSettings = getSettings(); - assert.deepStrictEqual(actualSettings, testSettings); + describe("The 'changeSetting' method", function () { + it("Updates correctly", async function () { + await changeSetting("helpCompletion", CommentType.LineComment, false, undefined); + assert.strictEqual(getSettings().helpCompletion, CommentType.LineComment); + }); }); - it("Updates correctly", async function () { - await changeSetting("helpCompletion", CommentType.LineComment, false, undefined); - assert.strictEqual(getSettings().helpCompletion, CommentType.LineComment); + describe("The 'getEffectiveConfigurationTarget' method'", function () { + it("Works for 'Workspace' target", async function () { + await changeSetting("helpCompletion", CommentType.LineComment, false, undefined); + const target = getEffectiveConfigurationTarget("helpCompletion"); + assert.strictEqual(target, vscode.ConfigurationTarget.Workspace); + }); + + it("Works for 'undefined' target", async function () { + await changeSetting("helpCompletion", undefined, false, undefined); + const target = getEffectiveConfigurationTarget("helpCompletion"); + assert.strictEqual(target, undefined); + }); }); - it("Gets the effective configuration target", async function () { - await changeSetting("helpCompletion", CommentType.LineComment, false, undefined); - let target = getEffectiveConfigurationTarget("helpCompletion"); - assert.strictEqual(target, vscode.ConfigurationTarget.Workspace); + describe("The CWD setting", function () { + beforeEach(async function () { + await changeSetting("cwd", undefined, undefined, undefined); + }); + + const workspace = vscode.workspace.workspaceFolders![0].uri.fsPath; + + it("Defaults to the 'mocks' workspace folder", async function () { + assert.strictEqual(await validateCwdSetting(undefined), workspace); + }); + + it("Uses the default when given a non-existent folder", async function () { + await changeSetting("cwd", "/a/totally/fake/folder", undefined, undefined); + assert.strictEqual(await validateCwdSetting(undefined), workspace); + }); + + it("Uses the given folder when it exists", async function () { + // A different than default folder that definitely exists + const cwd = path.resolve(path.join(process.cwd(), "..")); + await changeSetting("cwd", cwd, undefined, undefined); + assert.strictEqual(await validateCwdSetting(undefined), cwd); + }); - await changeSetting("helpCompletion", undefined, false, undefined); - target = getEffectiveConfigurationTarget("helpCompletion"); - assert.strictEqual(target, undefined); + it("Uses the home folder for ~ (tilde)", async function () { + // A different than default folder that definitely exists + await changeSetting("cwd", "~", undefined, undefined); + assert.strictEqual(await validateCwdSetting(undefined), os.homedir()); + }); }); }); From aa6dd8ea98147c689e8ddddbe5191c1c3e37c6a8 Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Tue, 8 Aug 2023 15:01:58 -0700 Subject: [PATCH 3/8] Stop using `this.sessionSettings.cwd` directly So we can remove weirdly placed `validateCwdSetting` calls and instead use it exactly when required. --- src/main.ts | 4 +--- src/process.ts | 6 +++--- src/session.ts | 7 +++---- src/settings.ts | 2 +- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/main.ts b/src/main.ts index b0ce924884..5b0fee117e 100644 --- a/src/main.ts +++ b/src/main.ts @@ -26,7 +26,7 @@ import { ShowHelpFeature } from "./features/ShowHelp"; import { SpecifyScriptArgsFeature } from "./features/DebugSession"; import { Logger } from "./logging"; import { SessionManager } from "./session"; -import { LogLevel, getSettings, validateCwdSetting } from "./settings"; +import { LogLevel, getSettings } from "./settings"; import { PowerShellLanguageId } from "./utils"; import { LanguageClientConsumer } from "./languageClientConsumer"; @@ -57,8 +57,6 @@ export async function activate(context: vscode.ExtensionContext): Promise("enabled"), }, errorHandler: { diff --git a/src/settings.ts b/src/settings.ts index b718008c08..96e38bf622 100644 --- a/src/settings.ts +++ b/src/settings.ts @@ -37,7 +37,7 @@ export class Settings extends PartialSettings { sideBar = new SideBarSettings(); pester = new PesterSettings(); buttons = new ButtonSettings(); - cwd = ""; + cwd = ""; // NOTE: use validateCwdSetting() instead of this directly! enableReferencesCodeLens = true; analyzeOpenDocumentsOnly = false; // TODO: Add (deprecated) useX86Host (for testing) From e3743c5bebd5da50e09fb9043e7610a8b36079a6 Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Tue, 8 Aug 2023 15:05:42 -0700 Subject: [PATCH 4/8] Support relative paths in `cwd` and add `getChosenWorkspace` This cleans up how we remember which workspace the user chose, and sets us up to save that information not in the `cwd` setting. Refactors and fixes `validateCwdSetting` to treat the empty string as undefined for `cwd`. --- src/features/PesterTests.ts | 5 ++- src/features/RunCode.ts | 6 ++-- src/settings.ts | 66 ++++++++++++++++++++++++------------- 3 files changed, 47 insertions(+), 30 deletions(-) diff --git a/src/features/PesterTests.ts b/src/features/PesterTests.ts index 7a49df2cf2..f5bd839c94 100644 --- a/src/features/PesterTests.ts +++ b/src/features/PesterTests.ts @@ -5,7 +5,7 @@ import * as path from "path"; import vscode = require("vscode"); import { ILogger } from "../logging"; import { SessionManager } from "../session"; -import { getSettings, chosenWorkspace, validateCwdSetting } from "../settings"; +import { getSettings, getChosenWorkspace } from "../settings"; import utils = require("../utils"); enum LaunchType { @@ -132,8 +132,7 @@ export class PesterTestsFeature implements vscode.Disposable { // Ensure the necessary script exists (for testing). The debugger will // start regardless, but we also pass its success along. - await validateCwdSetting(this.logger); return await utils.checkIfFileExists(this.invokePesterStubScriptPath) - && vscode.debug.startDebugging(chosenWorkspace, launchConfig); + && vscode.debug.startDebugging(await getChosenWorkspace(this.logger), launchConfig); } } diff --git a/src/features/RunCode.ts b/src/features/RunCode.ts index 896ae7dd0f..652cf55daf 100644 --- a/src/features/RunCode.ts +++ b/src/features/RunCode.ts @@ -4,7 +4,7 @@ import vscode = require("vscode"); import { SessionManager } from "../session"; import { ILogger } from "../logging"; -import { getSettings, chosenWorkspace, validateCwdSetting } from "../settings"; +import { getSettings, getChosenWorkspace } from "../settings"; enum LaunchType { Debug, @@ -40,9 +40,7 @@ export class RunCodeFeature implements vscode.Disposable { // Create or show the interactive console // TODO: #367: Check if "newSession" mode is configured this.sessionManager.showDebugTerminal(true); - - await validateCwdSetting(this.logger); - await vscode.debug.startDebugging(chosenWorkspace, launchConfig); + await vscode.debug.startDebugging(await getChosenWorkspace(this.logger), launchConfig); } } diff --git a/src/settings.ts b/src/settings.ts index 96e38bf622..0f03514273 100644 --- a/src/settings.ts +++ b/src/settings.ts @@ -6,6 +6,7 @@ import utils = require("./utils"); import os = require("os"); import { ILogger } from "./logging"; import untildify from "untildify"; +import path = require("path"); // TODO: Quite a few of these settings are unused in the client and instead // exist just for the server. Those settings do not need to be represented in @@ -214,47 +215,66 @@ export async function changeSetting( } // We don't want to query the user more than once, so this is idempotent. -let hasPrompted = false; -export let chosenWorkspace: vscode.WorkspaceFolder | undefined = undefined; - -export async function validateCwdSetting(logger: ILogger | undefined): Promise { - let cwd: string | undefined = utils.stripQuotePair( - vscode.workspace.getConfiguration(utils.PowerShellLanguageId).get("cwd")); - - // Only use the cwd setting if it exists. - if (cwd !== undefined) { - cwd = untildify(cwd); - if (await utils.checkIfDirectoryExists(cwd)) { - return cwd; - } +let hasChosen = false; +let chosenWorkspace: vscode.WorkspaceFolder | undefined = undefined; +export async function getChosenWorkspace(logger: ILogger | undefined): Promise { + if (hasChosen) { + return chosenWorkspace; } // If there is no workspace, or there is but it has no folders, fallback. if (vscode.workspace.workspaceFolders === undefined || vscode.workspace.workspaceFolders.length === 0) { - cwd = undefined; + chosenWorkspace = undefined; // If there is exactly one workspace folder, use that. } else if (vscode.workspace.workspaceFolders.length === 1) { - cwd = vscode.workspace.workspaceFolders[0].uri.fsPath; + chosenWorkspace = vscode.workspace.workspaceFolders[0]; // If there is more than one workspace folder, prompt the user once. - } else if (vscode.workspace.workspaceFolders.length > 1 && !hasPrompted) { - hasPrompted = true; + } else if (vscode.workspace.workspaceFolders.length > 1) { const options: vscode.WorkspaceFolderPickOptions = { placeHolder: "Select a workspace folder to use for the PowerShell Extension.", }; + chosenWorkspace = await vscode.window.showWorkspaceFolderPick(options); - cwd = chosenWorkspace?.uri.fsPath; - // Save the picked 'cwd' to the workspace settings. - // We have to check again because the user may not have picked. - if (cwd !== undefined && await utils.checkIfDirectoryExists(cwd)) { - await changeSetting("cwd", cwd, undefined, logger); + logger?.writeVerbose(`User selected workspace: '${chosenWorkspace?.name}'`); + } + + // NOTE: We don't rely on checking if `chosenWorkspace` is undefined because + // that may be the case if the user dismissed the prompt, and we don't want + // to show it again this session. + hasChosen = true; + return chosenWorkspace; +} + +export async function validateCwdSetting(logger: ILogger | undefined): Promise { + let cwd: string | undefined = utils.stripQuotePair( + vscode.workspace.getConfiguration(utils.PowerShellLanguageId).get("cwd")); + + // Use the resolved cwd setting if it exists. We're checking truthiness + // because it could be an empty string, in which case we ignore it. + if (cwd) { + cwd = path.resolve(untildify(cwd)); + if (await utils.checkIfDirectoryExists(cwd)) { + return cwd; } } + // Otherwise get a cwd from the workspace, if possible. + const workspace = await getChosenWorkspace(logger); + cwd = workspace?.uri.fsPath; + + // Save the picked 'cwd' to the workspace settings. + if (cwd && await utils.checkIfDirectoryExists(cwd)) { + // TODO: Stop saving this to settings! Instead, save the picked + // workspace (or save this, but in a cache). + await changeSetting("cwd", cwd, undefined, logger); + } + // If there were no workspace folders, or somehow they don't exist, use // the home directory. - if (cwd === undefined || !await utils.checkIfDirectoryExists(cwd)) { + if (!cwd || !await utils.checkIfDirectoryExists(cwd)) { return os.homedir(); } + return cwd; } From b98320edb02aa7375ff9fcb997e22b63cdf7bb6e Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Tue, 8 Aug 2023 15:07:52 -0700 Subject: [PATCH 5/8] Fix up settings tests and add test for `cwd` relative path support --- test/TestEnvironment.code-workspace | 2 +- test/core/settings.test.ts | 38 +++++++++++++++++++++-------- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/test/TestEnvironment.code-workspace b/test/TestEnvironment.code-workspace index b4035e33c5..c204beb6d1 100644 --- a/test/TestEnvironment.code-workspace +++ b/test/TestEnvironment.code-workspace @@ -8,6 +8,6 @@ "settings": { "git.openRepositoryInParentFolders": "never", "csharp.suppressDotnetRestoreNotification": true, - "extensions.ignoreRecommendations": true + "extensions.ignoreRecommendations": true, } } diff --git a/test/core/settings.test.ts b/test/core/settings.test.ts index 7ad9c26b6e..9e7d925be0 100644 --- a/test/core/settings.test.ts +++ b/test/core/settings.test.ts @@ -13,9 +13,20 @@ import { validateCwdSetting } from "../../src/settings"; import path from "path"; +import { ensureEditorServicesIsConnected } from "../utils"; describe("Settings E2E", function () { + async function changeCwdSetting(cwd: string | undefined): Promise { + await changeSetting("cwd", cwd, vscode.ConfigurationTarget.Workspace, undefined); + } + + async function resetCwdSetting(): Promise { + await changeCwdSetting(undefined); + } + describe("The 'getSettings' method loads the 'Settings' class", function () { + before(resetCwdSetting); + it("Loads without error", function () { assert.doesNotThrow(getSettings); }); @@ -29,29 +40,30 @@ describe("Settings E2E", function () { describe("The 'changeSetting' method", function () { it("Updates correctly", async function () { - await changeSetting("helpCompletion", CommentType.LineComment, false, undefined); + await changeSetting("helpCompletion", CommentType.LineComment, vscode.ConfigurationTarget.Workspace, undefined); assert.strictEqual(getSettings().helpCompletion, CommentType.LineComment); }); }); describe("The 'getEffectiveConfigurationTarget' method'", function () { it("Works for 'Workspace' target", async function () { - await changeSetting("helpCompletion", CommentType.LineComment, false, undefined); + await changeSetting("helpCompletion", CommentType.LineComment, vscode.ConfigurationTarget.Workspace, undefined); const target = getEffectiveConfigurationTarget("helpCompletion"); assert.strictEqual(target, vscode.ConfigurationTarget.Workspace); }); it("Works for 'undefined' target", async function () { - await changeSetting("helpCompletion", undefined, false, undefined); + await changeSetting("helpCompletion", undefined, vscode.ConfigurationTarget.Workspace, undefined); const target = getEffectiveConfigurationTarget("helpCompletion"); assert.strictEqual(target, undefined); }); }); describe("The CWD setting", function () { - beforeEach(async function () { - await changeSetting("cwd", undefined, undefined, undefined); - }); + // We're relying on this to be sure that the workspace is loaded. + before(ensureEditorServicesIsConnected); + before(resetCwdSetting); + afterEach(resetCwdSetting); const workspace = vscode.workspace.workspaceFolders![0].uri.fsPath; @@ -60,20 +72,26 @@ describe("Settings E2E", function () { }); it("Uses the default when given a non-existent folder", async function () { - await changeSetting("cwd", "/a/totally/fake/folder", undefined, undefined); + await changeCwdSetting("/a/totally/fake/folder"); assert.strictEqual(await validateCwdSetting(undefined), workspace); }); it("Uses the given folder when it exists", async function () { // A different than default folder that definitely exists const cwd = path.resolve(path.join(process.cwd(), "..")); - await changeSetting("cwd", cwd, undefined, undefined); + await changeCwdSetting(cwd); assert.strictEqual(await validateCwdSetting(undefined), cwd); }); it("Uses the home folder for ~ (tilde)", async function () { - // A different than default folder that definitely exists - await changeSetting("cwd", "~", undefined, undefined); + await changeCwdSetting("~"); + assert.strictEqual(await validateCwdSetting(undefined), os.homedir()); + }); + + it("Resolves relative paths", async function () { + // A different than default folder that definitely exists and is relative + const cwd = path.join("~", "somewhere", ".."); + await changeCwdSetting(cwd); assert.strictEqual(await validateCwdSetting(undefined), os.homedir()); }); }); From c0594bda48d07db1765c1ef17c833f9a49fa874d Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Tue, 8 Aug 2023 22:04:26 -0700 Subject: [PATCH 6/8] Handle cwd relative to workspace and named folders --- package.json | 2 +- src/settings.ts | 65 ++++++++++++++++++++++++++------------ test/core/settings.test.ts | 11 +++++-- 3 files changed, 55 insertions(+), 23 deletions(-) diff --git a/package.json b/package.json index 4ab97c8055..9485e50d60 100644 --- a/package.json +++ b/package.json @@ -735,7 +735,7 @@ "powershell.cwd": { "type": "string", "default": "", - "markdownDescription": "An explicit start path where the Extension Terminal will be launched. Both the PowerShell process's and the shell's location will be set to this directory. **Path must be fully resolved: variables are not supported!**" + "markdownDescription": "A path where the Extension Terminal will be launched. Both the PowerShell process's and the shell's location will be set to this directory. Does not support variables, but does support the use of '~' and paths relative to a single workspace. **For multi-root workspaces, use the name of the folder you wish to have as the cwd.**" }, "powershell.scriptAnalysis.enable": { "type": "boolean", diff --git a/src/settings.ts b/src/settings.ts index 0f03514273..6103b9d620 100644 --- a/src/settings.ts +++ b/src/settings.ts @@ -236,7 +236,11 @@ export async function getChosenWorkspace(logger: ILogger | undefined): Promise { - let cwd: string | undefined = utils.stripQuotePair( - vscode.workspace.getConfiguration(utils.PowerShellLanguageId).get("cwd")); - - // Use the resolved cwd setting if it exists. We're checking truthiness - // because it could be an empty string, in which case we ignore it. - if (cwd) { - cwd = path.resolve(untildify(cwd)); - if (await utils.checkIfDirectoryExists(cwd)) { - return cwd; + let cwd = utils.stripQuotePair( + vscode.workspace.getConfiguration(utils.PowerShellLanguageId).get("cwd")) + ?? ""; + + // Replace ~ with home directory. + cwd = untildify(cwd); + + // Use the cwd setting if it's absolute and exists. We don't use or resolve + // relative paths here because it'll be relative to the Code process's cwd, + // which is not what the user is expecting. + if (path.isAbsolute(cwd) && await utils.checkIfDirectoryExists(cwd)) { + return cwd; + } + + // If the cwd matches the name of a workspace folder, use it. Essentially + // "choose" a workspace folder based off the cwd path, and so set the state + // appropriately for `getChosenWorkspace`. + if (vscode.workspace.workspaceFolders) { + for (const workspaceFolder of vscode.workspace.workspaceFolders) { + // TODO: With some more work, we could support paths relative to a + // workspace folder name too. + if (cwd === workspaceFolder.name) { + hasChosen = true; + chosenWorkspace = workspaceFolder; + cwd = ""; + } } } // Otherwise get a cwd from the workspace, if possible. const workspace = await getChosenWorkspace(logger); - cwd = workspace?.uri.fsPath; + if (workspace === undefined) { + logger?.writeVerbose("Workspace was undefined, using homedir!"); + return os.homedir(); + } - // Save the picked 'cwd' to the workspace settings. - if (cwd && await utils.checkIfDirectoryExists(cwd)) { - // TODO: Stop saving this to settings! Instead, save the picked - // workspace (or save this, but in a cache). - await changeSetting("cwd", cwd, undefined, logger); + const workspacePath = workspace.uri.fsPath; + + // Use the chosen workspace's root to resolve the cwd. + const relativePath = path.join(workspacePath, cwd); + if (await utils.checkIfDirectoryExists(relativePath)) { + return relativePath; } - // If there were no workspace folders, or somehow they don't exist, use - // the home directory. - if (!cwd || !await utils.checkIfDirectoryExists(cwd)) { - return os.homedir(); + // Just use the workspace path. + if (await utils.checkIfDirectoryExists(workspacePath)) { + return workspacePath; } - return cwd; + // If all else fails, use the home directory. + return os.homedir(); } diff --git a/test/core/settings.test.ts b/test/core/settings.test.ts index 9e7d925be0..306872a2dc 100644 --- a/test/core/settings.test.ts +++ b/test/core/settings.test.ts @@ -88,11 +88,18 @@ describe("Settings E2E", function () { assert.strictEqual(await validateCwdSetting(undefined), os.homedir()); }); - it("Resolves relative paths", async function () { + it("Accepts relative paths", async function () { // A different than default folder that definitely exists and is relative const cwd = path.join("~", "somewhere", ".."); + const expected = path.join(os.homedir(), "somewhere", ".."); await changeCwdSetting(cwd); - assert.strictEqual(await validateCwdSetting(undefined), os.homedir()); + assert.strictEqual(await validateCwdSetting(undefined), expected); + }); + + it("Handles relative paths", async function () { + await changeCwdSetting("./BinaryModule"); + const expected = path.join(workspace, "./BinaryModule"); + assert.strictEqual(await validateCwdSetting(undefined), expected); }); }); }); From fe8cc41aceddbecb2f075ad48f840feaf28c8d08 Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Wed, 9 Aug 2023 11:38:12 -0700 Subject: [PATCH 7/8] Add prompt to save multi-root workspace choice to `cwd` So it's not just automatic, but can be done easily (and is an appropriate value to sync with workspace settings). --- src/logging.ts | 4 ++-- src/session.ts | 2 +- src/settings.ts | 8 ++++++++ 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/logging.ts b/src/logging.ts index 6a8b984f74..846fcfc5b8 100644 --- a/src/logging.ts +++ b/src/logging.ts @@ -100,8 +100,8 @@ export class Logger implements ILogger { public async writeAndShowInformation(message: string, ...additionalMessages: string[]): Promise { this.write(message, ...additionalMessages); - const selection = await vscode.window.showInformationMessage(message, "Show Logs"); - if (selection !== undefined) { + const selection = await vscode.window.showInformationMessage(message, "Show Logs", "Okay"); + if (selection === "Show Logs") { this.showLogPanel(); } } diff --git a/src/session.ts b/src/session.ts index 393325eeaa..0d3cc093d0 100644 --- a/src/session.ts +++ b/src/session.ts @@ -469,7 +469,7 @@ export class SessionManager implements Middleware { this.logger.updateLogLevel(settings.developer.editorServicesLogLevel); // Detect any setting changes that would affect the session. - if (!this.suppressRestartPrompt && + if (!this.suppressRestartPrompt && this.sessionStatus === SessionStatus.Running && (settings.cwd.toLowerCase() !== this.sessionSettings.cwd.toLowerCase() || settings.powerShellDefaultVersion.toLowerCase() !== this.sessionSettings.powerShellDefaultVersion.toLowerCase() || settings.developer.editorServicesLogLevel.toLowerCase() !== this.sessionSettings.developer.editorServicesLogLevel.toLowerCase() diff --git a/src/settings.ts b/src/settings.ts index 6103b9d620..8b838f5a4d 100644 --- a/src/settings.ts +++ b/src/settings.ts @@ -240,6 +240,14 @@ export async function getChosenWorkspace(logger: ILogger | undefined): Promise Date: Wed, 9 Aug 2023 11:39:16 -0700 Subject: [PATCH 8/8] Increase unit test timeout to 10 minutes And Mocha's debugger hook-up timeout to 30 seconds. Now unit tests can be debugged easily, and the timeout is still appropriate enough for CI. --- .mocharc.json | 2 +- extension-dev.code-workspace | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.mocharc.json b/.mocharc.json index 2bd21c2f98..ef0c638ee7 100644 --- a/.mocharc.json +++ b/.mocharc.json @@ -6,7 +6,7 @@ ".jsx" ], "require": "source-map-support/register", - "timeout": 60000, + "timeout": 600000, "slow": 2000, "spec": "out/test/**/*.test.js" } diff --git a/extension-dev.code-workspace b/extension-dev.code-workspace index f698c754ef..b2dd02bc63 100644 --- a/extension-dev.code-workspace +++ b/extension-dev.code-workspace @@ -59,7 +59,7 @@ "mochaExplorer.autoload": false, // The test instance pops up every time discovery or run is done, this could be annoying on startup. "mochaExplorer.debuggerPort": 59229, // Matches the launch config, we dont want to use the default port as we are launching a duplicate instance of vscode and it might conflict. "mochaExplorer.ipcRole": "server", - "mochaExplorer.ipcTimeout": 10000, + "mochaExplorer.ipcTimeout": 30000, // 30 seconds "testExplorer.useNativeTesting": true, "mochaExplorer.env": { "VSCODE_VERSION": "insiders",