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

fix the remove unused installations cleanup #385

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

Techatrix
Copy link
Collaborator

The VS Code documentation says that the deactivate function can be async but it doesn't seem to work.

The sorting was also wrong.

@Vexu
Copy link
Member

Vexu commented Jan 22, 2025

The VS Code documentation says that the deactivate function can be async but it doesn't seem to work.

Is it not a problem for deactivating ZLS too? Can you describe the issue a bit more? Was the function not being run?

@Techatrix
Copy link
Collaborator Author

Can you describe the issue a bit more? Was the function not being run?

Changing the deactivate function to the following will show the problem:

function delay(timeout: number): Promise<void> {
    return new Promise((resolve) => {
        setTimeout(() => {
            resolve();
        }, timeout);
    });
}

export async function deactivate() {
    appendFileSync("/home/techatrix/repos/vscode-zig/output.log", "Start\n");
    await delay(2000);
    appendFileSync("/home/techatrix/repos/vscode-zig/output.log", "End\n");
}

After restarting the extension, the output.log file will only contain Start which indicates that vscode did not await the returned promise. Reducing the delay to a very low value like 10 works as expected. I tried different values and I got inconsistent result around 300ms. The API docs do state that it is possible to return a promise and microsoft/vscode#47881 says that the extension has 5 seconds to shut down.

Also, the deactivate is only called when restarting the extension but not when closing vscode making it useless to cleanup unused Zig/ZLS installations.

Is it not a problem for deactivating ZLS too?

Good point. I didn't check this before but the time limit is enough to shutdown ZLS. The stopClient finishes successfully but any calls to removeUnusedInstallations don't. It struggled to get past the vscode.workspace.fs.readDirectory call.


After looking back at this PR, I think it better to call removeUnusedInstallations after installing a new Zig or ZLS version instead of calling it in the activate function.

The deactivate function does not run when closing VS Code and it has a
time limit that is to small to finish running the version cleanup logic.

Instead, unused versions should be uninstalled every time a new version
has been installed.

The sorting was also wrong.
@Techatrix Techatrix force-pushed the techatrix/fix-unused-version-cleanup branch from 5dc6a0d to 7d6ffa1 Compare January 23, 2025 16:59
@Vexu
Copy link
Member

Vexu commented Jan 23, 2025

Thanks for the detailed response!

@Vexu Vexu merged commit 4d575c7 into master Jan 23, 2025
2 checks passed
@Techatrix Techatrix deleted the techatrix/fix-unused-version-cleanup branch January 24, 2025 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants