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

Use serverDataFolderName in check-requirements-linux.sh #208074

Merged
merged 2 commits into from
Mar 23, 2024

Conversation

aeschli
Copy link
Contributor

@aeschli aeschli commented Mar 19, 2024

Instead of serverApplicationName it should be serverDataFolderName

  • serverDataFolderName matches with the data folder that is already there
  • serverDataFolderName starts with a . which is IMO what we want.

@aeschli aeschli self-assigned this Mar 19, 2024
@aeschli aeschli enabled auto-merge (squash) March 19, 2024 08:38
@aeschli aeschli requested a review from deepak1556 March 19, 2024 08:39
@vscodenpa vscodenpa added this to the March 2024 milestone Mar 19, 2024
@deepak1556
Copy link
Collaborator

Any reason we want the file to be hidden by default ? I went with serverApplicationName to avoid the file from being hidden.

@aeschli
Copy link
Contributor Author

aeschli commented Mar 19, 2024

I think it should be hidden as it's an VS Code internal file. No point for a user to see or manipulate.
I was thinking that this to be used as the marker file. Maybe I misunderstood.Thinking more about it, the file won't work to remember that the OS passes the compatibility check. Maybe we need a file that has the error code as content?

@deepak1556
Copy link
Collaborator

I think it should be hidden as it's an VS Code internal file. No point for a user to see or manipulate.

No actually it is the opposite, this file is meant for users to force the legacy server. If the detection script is not returning the right result on a setup for whatever reason we need a way for the users to override the result.

Thinking more about it, the file won't work to remember that the OS passes the compatibility check. Maybe we need a file that has the error code as content?

I was thinking the extensions would setup that file depending on the exit code from this script.

@deepak1556
Copy link
Collaborator

If the detection script is not returning the right result on a setup for whatever reason we need a way for the users to override the result.

Thinking again, should this rather be a setting on the client side to force the legacy server and avoid introducing a separate file ? That way it works for both classic and exec server cases, also the extension controls the behavior.

remote.useLegacyServer: "true" | "false"

@aeschli
Copy link
Contributor Author

aeschli commented Mar 22, 2024

@deepak1556 Now that check-requirements is no longer part of the server start script, I suggest we remove the

if [ -f "$HOME/@@SERVER_DATA_FOLDER_NAME@@-use-legacy" ]; then

check and leave it to the extensions to have a marker file or a setting.

Copy link
Collaborator

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

sounds good, thanks!

@aeschli aeschli merged commit 57b0c14 into main Mar 23, 2024
6 checks passed
@aeschli aeschli deleted the aeschli/late-pinniped-675 branch March 23, 2024 01:10
Gobinath-B pushed a commit to Gobinath-B/vscode that referenced this pull request Mar 24, 2024
…8074)

* Use serverDataFolderName in check-requirements-linux.sh

* remove the (new) file check
@microsoft microsoft locked and limited conversation to collaborators Jun 10, 2024
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.

4 participants