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 bundled environment activation script file #3083

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Jan 21, 2025

Motivation

We've been seeing 3 types of common issues in our telemetry when running the activation script - in particular for Shadowenv, but this affects all other managers except chruby and RubyInstaller:

  1. Pipe encoding issues. It appears that some users have their shell's encoding configured to ASCII-8BIT, which is incompatible with the JSON we try to return and results in an unknown conversion error
  2. Incompatible library for the JSON gem. Since it's a native extension, if json was compiled for a different Ruby version, we will fail to require it and thus fail to activate
  3. Syntax errors. Depending on the user's shell and its configurations, we sometimes hit weird syntax errors, which happen both when requiring json and when running the script

This PR proposes a few changes to try to address these issues.

Implementation

We figured out that the first issue happens when the user sets LANG=C in their shell and has a prompt that includes certain unicode/glyph characters, which makes Ruby consider the encoding of the PS1 environment variable as ASCII-8BIT. To fix that, I started setting LANG and using the -E switch to force Ruby's encoding.

To address the other issues, my suggestion is to bundle the activation script in the extension, so that we can avoid running it with ruby -e. That helps us avoid escaping and syntax issues for different shells (e.g.: we no longer need to worry about quoting).

Additionally, I'm no longer using JSON to communicate. We essentially only need to pass key value pairs between the script and the extension, so I think we can avoid the issues coming from JSON by using a predefined separator to split them.

In this case, I'm just using a predefined string to separate values and we return/expect them in order. Note that we need a very unique separator, otherwise we risk incorrectly splitting the result.

For example, if we were to use line breaks, that would fail because you can have them in values for environment variables, which would make us incorrectly split the output.

Automated Tests

Adjusted current tests.

Manual Tests

Since I'm making changes to one of the most sensitive parts of activation, I want to cut a prerelease version for this branch to give us an opportunity to catch mistakes before stabilizing the approach.

Copy link
Member Author

vinistock commented Jan 21, 2025


How to use the Graphite Merge Queue

Add the label graphite-merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@vinistock vinistock self-assigned this Jan 21, 2025
@vinistock vinistock added vscode This pull request should be included in the VS Code extension's release notes bugfix This PR will fix an existing bug labels Jan 21, 2025 — with Graphite App
@vinistock vinistock force-pushed the 01-20-use_bundled_environment_activation_script_file branch 3 times, most recently from f832caa to 01351d7 Compare January 21, 2025 22:38
@vinistock vinistock marked this pull request as ready for review January 22, 2025 14:43
@vinistock vinistock requested a review from a team as a code owner January 22, 2025 14:43
@vinistock vinistock requested a review from andyw8 January 22, 2025 14:43
vscode/src/ruby/versionManager.ts Show resolved Hide resolved
vscode/src/ruby/versionManager.ts Show resolved Hide resolved
vscode/src/ruby/versionManager.ts Outdated Show resolved Hide resolved
vscode/src/ruby/versionManager.ts Outdated Show resolved Hide resolved
@vinistock vinistock force-pushed the 01-20-use_bundled_environment_activation_script_file branch 4 times, most recently from 3f3c787 to 57ae166 Compare January 22, 2025 22:27
@vinistock vinistock requested a review from paracycle January 22, 2025 22:28
@vinistock vinistock changed the base branch from main to graphite-base/3083 January 23, 2025 19:39
@vinistock vinistock force-pushed the 01-20-use_bundled_environment_activation_script_file branch from 57ae166 to 02f86f3 Compare January 23, 2025 19:39
@vinistock vinistock changed the base branch from graphite-base/3083 to 01-16-fix_file_paths_for_exec_launcher_on_windows January 23, 2025 19:39
@vinistock vinistock requested a review from paracycle January 23, 2025 19:42
@vinistock vinistock force-pushed the 01-16-fix_file_paths_for_exec_launcher_on_windows branch from 504d98e to f3c0718 Compare January 27, 2025 14:33
@vinistock vinistock force-pushed the 01-20-use_bundled_environment_activation_script_file branch 2 times, most recently from b003c17 to fe8b2f8 Compare January 27, 2025 19:37
@vinistock vinistock force-pushed the 01-16-fix_file_paths_for_exec_launcher_on_windows branch from f3c0718 to 0ee2ba7 Compare January 27, 2025 19:37
@vinistock vinistock changed the base branch from 01-16-fix_file_paths_for_exec_launcher_on_windows to graphite-base/3083 January 27, 2025 19:57
@vinistock vinistock force-pushed the 01-20-use_bundled_environment_activation_script_file branch from fe8b2f8 to 60e40a8 Compare January 27, 2025 21:07
@vinistock vinistock force-pushed the 01-20-use_bundled_environment_activation_script_file branch from 60e40a8 to 5372f78 Compare January 27, 2025 21:11
@vinistock vinistock force-pushed the 01-20-use_bundled_environment_activation_script_file branch from 5372f78 to 06130f6 Compare January 27, 2025 21:30
@vinistock vinistock force-pushed the 01-20-use_bundled_environment_activation_script_file branch 2 times, most recently from 84a157d to d7cfcbd Compare January 28, 2025 15:13
@vinistock vinistock force-pushed the 01-20-use_bundled_environment_activation_script_file branch from d7cfcbd to 11599be Compare January 28, 2025 16:07
@vinistock vinistock changed the base branch from graphite-base/3083 to main January 28, 2025 16:07
@vinistock vinistock force-pushed the 01-20-use_bundled_environment_activation_script_file branch 2 times, most recently from 3a3d4b3 to cb96da0 Compare January 28, 2025 16:12
@vinistock vinistock force-pushed the 01-20-use_bundled_environment_activation_script_file branch from cb96da0 to 01c33ea Compare January 31, 2025 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug vscode This pull request should be included in the VS Code extension's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants