-
Notifications
You must be signed in to change notification settings - Fork 177
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
base: main
Are you sure you want to change the base?
Use bundled environment activation script file #3083
Conversation
How to use the Graphite Merge QueueAdd 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. |
f832caa
to
01351d7
Compare
3f3c787
to
57ae166
Compare
57ae166
to
02f86f3
Compare
504d98e
to
f3c0718
Compare
b003c17
to
fe8b2f8
Compare
f3c0718
to
0ee2ba7
Compare
0ee2ba7
to
7f629ac
Compare
fe8b2f8
to
60e40a8
Compare
7f629ac
to
72e94e8
Compare
60e40a8
to
5372f78
Compare
72e94e8
to
e584ec7
Compare
5372f78
to
06130f6
Compare
e584ec7
to
50af30c
Compare
84a157d
to
d7cfcbd
Compare
50af30c
to
64aaa57
Compare
d7cfcbd
to
11599be
Compare
64aaa57
to
ed15491
Compare
3a3d4b3
to
cb96da0
Compare
cb96da0
to
01c33ea
Compare
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:
json
was compiled for a different Ruby version, we will fail to require it and thus fail to activateThis 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 asASCII-8BIT
. To fix that, I started settingLANG
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.