From 5a354dae1f14716f37b6a75991d3ef4e2175fdf2 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Tue, 25 Jan 2022 21:15:01 +0100 Subject: [PATCH 1/2] Offer to delete the old clangd dir after installing a new one. We want the user to get a chance to check the new clangd first, so: - stash the old path away before reloading - when we start up, check if a stashed path exists, was autoinstalled, and is not being used - offer to delete, then clear the stash if they answered Part of https://github.com/clangd/vscode-clangd/issues/285 --- src/index.ts | 57 +++++++++++++++++++++++++++++++++++-- test/index.ts | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 2 deletions(-) diff --git a/src/index.ts b/src/index.ts index 554cd99..918bbd8 100644 --- a/src/index.ts +++ b/src/index.ts @@ -30,6 +30,9 @@ type UI = { readonly storagePath: string; // Configured clangd location. clangdPath: string; + // Storage for old clangd location, to be maybe cleaned up later. + // This should persist through promptReload. + cleanupPath: string | undefined; // Show a generic message to the user. info(s: string): void; @@ -44,6 +47,8 @@ type UI = { promptUpdate(oldVersion: string, newVersion: string): void; // Ask the user to run installLatest() to install missing clangd. promptInstall(version: string): void; + // Ask the user whether to delete an old clangd installation. + promptDelete(path: string): Promise; // Ask whether to reuse rather than overwrite an existing clangd installation. // Undefined means no choice was made, so we shouldn't do either. shouldReuse(path: string): Promise; @@ -77,8 +82,11 @@ export async function prepare(ui: UI, // Allow extension to load, asynchronously check for updates. return { clangdPath: absPath, - background: checkUpdate ? checkUpdates(/*requested=*/ false, ui) - : Promise.resolve() + background: (async () => { + if (checkUpdate) + await checkUpdates(/*requested=*/ false, ui); + await checkCleanup(ui); + })(), }; } @@ -89,7 +97,9 @@ export async function installLatest(ui: UI) { try { const release = await Github.latestRelease(); const asset = await Github.chooseAsset(release); + const oldPath = ui.clangdPath; ui.clangdPath = await Install.install(release, asset, abort, ui); + ui.cleanupPath = oldPath; ui.promptReload(`clangd ${release.name} is now installed.`); } catch (e) { if (!abort.signal.aborted) { @@ -127,6 +137,49 @@ export async function checkUpdates(requested: boolean, ui: UI) { ui.promptUpdate(upgrade.old, upgrade.new); } +// Returns relative path of `p` within `parent`, if it is inside `parent`. +function pathWithin(p: string, parent: string): string|undefined { + const result = path.relative(parent, p); // /... + if (result && !result.startsWith('..') && !path.isAbsolute(result)) + return result; + return undefined; +} + +// Offer to delete the installation containing ui.cleanupPath if appropriate. +export async function checkCleanup(ui: UI) { + // First, find the parent directory that we'd delete. + // cleanupPath = /storage/install//clangd-/bin/clangd + // We want to delete /storage/install/ + const cleanupPath = ui.cleanupPath; + if (!cleanupPath) + return; + // install = /storage/install + const install = path.join(ui.storagePath, 'install'); + // inInstall = /clangd-/bin/clangd + const inInstall = pathWithin(cleanupPath, install); // /... + if (!inInstall) { + ui.cleanupPath = undefined; // Not a cleanable path. + return; + } + // cleanupDir = /storage/install/ + const cleanupDir = path.join(install, inInstall.split(path.sep)[0]); + + // Don't clean up if it's in use, or already gone. + if (pathWithin(ui.clangdPath, cleanupDir) || + !await promisify(fs.exists)(cleanupDir)) { + ui.cleanupPath = undefined; + return; + } + + // Deleting looks valid, let's asked the user. + const answer = await ui.promptDelete(cleanupPath); + if (answer === undefined) // dismissed the prompt without yes/no + return; // ask again next time + if (answer) + await promisify(rimraf)(cleanupDir); + ui.cleanupPath = undefined; // don't ask or check again +} + // The extension has detected clangd isn't available. // Inform the user, and if possible offer to install or adjust the path. // Unlike installLatest(), we've had no explicit user request or consent yet. diff --git a/test/index.ts b/test/index.ts index 08041c7..58b3038 100644 --- a/test/index.ts +++ b/test/index.ts @@ -31,6 +31,7 @@ class FakeUI { } clangdPath = oldClangd; + cleanupPath: string|undefined; info(s: string) { this.event('info'); @@ -53,6 +54,11 @@ class FakeUI { this.event('shouldReuse'); return this.shouldReuseValue; } + public promptDeleteValue = true; + async promptDelete() { + this.event('promptDelete'); + return this.promptDeleteValue; + } slow(_title: string, work: Promise) { this.event('slow'); @@ -104,6 +110,7 @@ test('install', async (assert, ui) => { assert.equal(ui.clangdPath, installedClangd); assert.deepEqual( ui.events, [/*download*/ 'progress', /*extract*/ 'slow', 'promptReload']); + assert.equal(ui.cleanupPath, oldClangd); }); test('install: no binary for platform', async (assert, ui) => { @@ -196,6 +203,66 @@ test('update: from 15 to 10', async (assert, ui) => { assert.deepEqual(ui.events, [/*up-to-date*/ 'info']); }); +// Test the cleanup of old installs. + +test('cleanup', async (assert, ui) => { + const bin = path.join(ui.storagePath, 'install', '4.0', 'dir', 'bin'); + await fs.promises.mkdir(bin, {recursive: true}); + ui.cleanupPath = path.join(bin, 'clangd'); + ui.promptDeleteValue = true; + await install.checkCleanup(ui); + + assert.deepEqual(ui.events, ['promptDelete']); + assert.equal(ui.cleanupPath, undefined); + assert.false(fs.existsSync(path.join(ui.storagePath, 'install', '4.0')), + `should delete clangd install dir above: ${bin}`); + assert.true(fs.existsSync(path.join(ui.storagePath, 'install')), + 'should not delete install root'); +}); + +test('cleanup: user declined', async (assert, ui) => { + const bin = path.join(ui.storagePath, 'install', '4.0', 'dir', 'bin'); + await fs.promises.mkdir(bin, {recursive: true}); + ui.cleanupPath = path.join(bin, 'clangd'); + ui.promptDeleteValue = false; + await install.checkCleanup(ui); + + assert.deepEqual(ui.events, ['promptDelete']); + assert.equal(ui.cleanupPath, undefined); + assert.true(fs.existsSync(bin), `should not delete, user declined: ${bin}`); +}); + +test('cleanup: user dismissed', async (assert, ui) => { + const bin = path.join(ui.storagePath, 'install', '4.0', 'dir', 'bin'); + await fs.promises.mkdir(bin, {recursive: true}); + ui.cleanupPath = path.join(bin, 'clangd'); + ui.promptDeleteValue = undefined; + await install.checkCleanup(ui); + + assert.deepEqual(ui.events, ['promptDelete']); + assert.equal(ui.cleanupPath, path.join(bin, 'clangd')); + assert.true(fs.existsSync(bin), `should not delete, user dismissed: ${bin}`); +}); + +test('cleanup: still in use', async (assert, ui) => { + const bin = path.join(ui.storagePath, 'install', '4.0', 'dir', 'bin'); + await fs.promises.mkdir(bin, {recursive: true}); + ui.cleanupPath = path.join(bin, 'clangd'); + ui.clangdPath = path.join(ui.storagePath, 'install', '4.0', 'more', 'clangd'); + await install.checkCleanup(ui); + + assert.deepEqual(ui.events, [], `should not prompt, in use: ${bin}`); + assert.true(fs.existsSync(bin), `should not delete, in use: ${bin}`); +}); + +test('cleanup: already gone', async (assert, ui) => { + const bin = path.join(ui.storagePath, 'install', '4.0', 'dir', 'bin'); + ui.cleanupPath = path.join(bin, 'clangd'); + await install.checkCleanup(ui); + + assert.deepEqual(ui.events, [], `should not prompt, missing: ${bin}`); +}); + // Test the generic on-startup flow which: // - locates configured clangd if available // - suggests installing it if missing, and checks for updates if present @@ -263,3 +330,15 @@ test('prepare: unversioned clangd installed', async (assert, ui) => { // We assume any custom-installed clangd is desired. assert.deepEqual(ui.events, []); }); + +test('prepare: cleanup declined', async (assert, ui) => { + ui.cleanupPath = path.join(ui.storagePath, 'install', '4.0', 'dir', 'clangd'); + await fs.promises.mkdir(ui.cleanupPath, {recursive: true}); + ui.clangdPath = newClangd; + ui.promptDeleteValue = false; + + const status = await install.prepare(ui, true); + await status.background; + assert.deepEqual(ui.events, ['promptDelete']); + assert.equal(ui.cleanupPath, undefined, 'Cleared cleanupPath after prompt'); +}); From 587b3b22902b70c05e05e4577f25b3840df8f410 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Tue, 25 Jan 2022 21:38:48 +0100 Subject: [PATCH 2/2] Also bump major version per semver --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 2e5acf8..0c5a322 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@clangd/install", - "version": "0.1.4", + "version": "1.0.0", "description": "Installing clangd binaries from editor plugins.", "main": "out/src/index.js", "files": [