Skip to content
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.

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

Offer to delete the old clangd dir after installing a new one. #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -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": [
Expand Down
57 changes: 55 additions & 2 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<boolean|undefined>;
// 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<boolean|undefined>;
Expand Down Expand Up @@ -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);
})(),
};
}

Expand All @@ -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) {
Expand Down Expand Up @@ -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); // <version>/...
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/<version>/clangd-<version>/bin/clangd
// We want to delete /storage/install/<version>
const cleanupPath = ui.cleanupPath;
if (!cleanupPath)
return;
// install = /storage/install
const install = path.join(ui.storagePath, 'install');
// inInstall = <version>/clangd-<version>/bin/clangd
const inInstall = pathWithin(cleanupPath, install); // <version>/...
if (!inInstall) {
ui.cleanupPath = undefined; // Not a cleanable path.
return;
}
// cleanupDir = /storage/install/<version>
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.
Expand Down
79 changes: 79 additions & 0 deletions test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class FakeUI {
}

clangdPath = oldClangd;
cleanupPath: string|undefined;

info(s: string) {
this.event('info');
Expand All @@ -53,6 +54,11 @@ class FakeUI {
this.event('shouldReuse');
return this.shouldReuseValue;
}
public promptDeleteValue = true;
async promptDelete() {
this.event('promptDelete');
return this.promptDeleteValue;
}

slow<T>(_title: string, work: Promise<T>) {
this.event('slow');
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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');
});