Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: add linux library dependency check for remote server #202210

Merged
merged 7 commits into from
Jan 15, 2024

Conversation

deepak1556
Copy link
Collaborator

Fixes #201129

Originally based on https://github.com/microsoft/vscode-remote-ssh/blob/84f9f5a0668aa37d9d8d73e508660679fc7db88f/open-ssh-remote/src/install-script/server-installer.ts#L166-L277 but script has been changed to be aware of multi-arch libraries and cases where ldd might not return accurate result. Please check comments above specific sections for details.

@deepak1556 deepak1556 added this to the December / January 2024 milestone Jan 11, 2024
@deepak1556 deepak1556 self-assigned this Jan 11, 2024
@deepak1556
Copy link
Collaborator Author

deepak1556 commented Jan 11, 2024

Testing with custom insiders

Build running at https://monacotools.visualstudio.com/Monaco/_build/results?buildId=251108&view=results

❌ - marks error flow
✅ - marks successful connection

aarch64 and armhf connections were tested using remote containers
x86_64 connections was tested with remote wsl

Distros x86_64 aarch64 armhf
Ubuntu 18.04
Ubuntu 20.04
Debian stretch -
Debian buster -
Debian bookworm - -
Alpine - N/A
CentOS 7 -
OracleLinux 7.9 - -
openSUSE tumbleweed - -

@deepak1556 deepak1556 force-pushed the robo/add_remote_dependency_check branch from 10653c1 to 8e6cf94 Compare January 11, 2024 07:25
@deepak1556
Copy link
Collaborator Author

Screenshot 2024-01-12 at 1 50 51

@connor4312
Copy link
Member

Currently the requirements check is done in the CLI, which is what remotes are normalizing on as a way to install things. In the shell script this duplicates logic and surfaces warnings in a way that's harder for remote extensions to handle if we moved to using this script--we'd need to add logic to look for specific output on the vscode server. Is there a reason to prefer the shell script over allowing the CLI to guard it?

@deepak1556
Copy link
Collaborator Author

As far as what I have heard from @aeschli and @chrmarti the cli is not yet the default in all remote scenarios, correct me here. We want these prerequisites to be checked for the upcoming 1.86 release for all cases. I am happy to rely on the CLI if that does the work today for all scenarios.

we'd need to add logic to look for specific output on the vscode server

The script executes before the server starts and aborts server setup if conditions are not met, can you clarify the need to parse the output ?

@deepak1556
Copy link
Collaborator Author

To clarify, the shell code is not meant to be permanent, it is temporary way to reliably check the requirements on all remote cases as the server executable will not run on the affected platforms starting with 1.86 and we want to show a nice error in the log pointing to the FAQ for these users. I was told that code-server-linux.sh is the entry point for server on all remote cases so ended up adding the check here.

@deepak1556 deepak1556 marked this pull request as ready for review January 11, 2024 18:07
@connor4312
Copy link
Member

The script executes before the server starts and aborts server setup if conditions are not met, can you clarify the need to parse the output ?

I'm not sure (haven't tested) whether the remotes show that nicely. Of course, showing just the raw output is probably fine, but e.g. I think remote SSH has some logic that shows failed prerequisite checks in a cleaner notification.

To clarify, the shell code is not meant to be permanent, it is temporary way to reliably check the requirements on all remote cases as the server executable will not run on the affected platforms starting with 1.86 and we want to show a nice error in the log pointing to the FAQ for these users. I was told that code-server-linux.sh is the entry point for server on all remote cases so ended up adding the check here.

Sounds reasonable then 👍

@deepak1556
Copy link
Collaborator Author

deepak1556 commented Jan 12, 2024

I think remote SSH has some logic that shows failed prerequisite checks in a cleaner notification.

Yeah this would be a nice follow-up, I have added a custom exit code for this case specifically. Extensions need not parse the output but rather listen for the exit code and show a notification with the same FAQ link.

@deepak1556 deepak1556 merged commit 1339f07 into main Jan 15, 2024
34 checks passed
@deepak1556 deepak1556 deleted the robo/add_remote_dependency_check branch January 15, 2024 08:43
@microsoft microsoft locked and limited conversation to collaborators Jun 10, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify platform requirements for 1.86 release
3 participants