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

Swallow copy zsh script error to allow multi-user machines #157900

Merged
merged 1 commit into from Aug 11, 2022
Merged

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Aug 11, 2022

Fixes #157611

@Tyriar Tyriar added this to the August 2022 milestone Aug 11, 2022
@Tyriar Tyriar requested a review from meganrogge August 11, 2022 13:27
@Tyriar Tyriar self-assigned this Aug 11, 2022
@Tyriar Tyriar merged commit 485e6c9 into main Aug 11, 2022
@Tyriar Tyriar deleted the tyriar/157611 branch August 11, 2022 18:02
} catch {
// Swallow error, this should only happen when multiple users are on the same
// machine. Since the shell integration scripts rarely change, plus the other user
// should be using the same version of the server in this case, assume the script is
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you can assume that two users are on the same client version of vscode, especially around the time of a release. And if the name is as generic as /tmp/vscode-zsh/.zshrc couldn't you have an issue with a single user upgrading too? And insiders vs stable?

Copy link
Member Author

@Tyriar Tyriar Aug 25, 2022

Choose a reason for hiding this comment

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

@roblourens I think you're right about multiple versions of the server running at once, though maybe we should notify if we don't when a newer version is installed on the same machine? I didn't want to get into the business of leaving a bunch of per version files around. The impact of not writing this file is quite small as the script doesn't change often and even if it did, it likely wouldn't cause problems and they're easily resolved (update to latest).

We should include the product name in the folder, this was an oversight #159182

Copy link
Member

Choose a reason for hiding this comment

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

they're easily resolved (update to latest).

When you update, how does it end up actually using a newer version of this file? Seems like the oldest will stick around forever?

Copy link
Member Author

Choose a reason for hiding this comment

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

@roblourens oh, I thought this was related to a lock on the file but it's because a user cannot modify another user's file? I'll reopen the issue then

Copy link
Member

Choose a reason for hiding this comment

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

That was how I was reading it but I haven't actually tried that scenario

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The terminal cannot be opened when multiple users use remote-ssh
3 participants