Skip to content

Commit 48f358e

Browse files
committed
Use bundled environment activation script file (#3083)
### 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.
1 parent 65a8193 commit 48f358e

20 files changed

+416
-178
lines changed

project-words

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ dont
2626
eglot
2727
Eglot
2828
eruby
29+
EUTF
2930
exitstatus
3031
EXTGLOB
3132
fakehome

vscode/activation.rb

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
env = ENV.map { |k, v| "#{k}RUBY_LSP_VS#{v}" }
2+
env.unshift(RUBY_VERSION, Gem.path.join(","), !!defined?(RubyVM::YJIT))
3+
STDERR.print("RUBY_LSP_ACTIVATION_SEPARATOR#{env.join("RUBY_LSP_FS")}RUBY_LSP_ACTIVATION_SEPARATOR")

vscode/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
"name": "ruby-lsp",
33
"displayName": "Ruby LSP",
44
"description": "VS Code plugin for connecting with the Ruby LSP",
5-
"version": "0.8.20",
5+
"version": "0.9.1",
66
"publisher": "Shopify",
77
"repository": {
88
"type": "git",

vscode/src/debugger.ts

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -258,9 +258,6 @@ export class Debugger
258258

259259
this.logDebuggerMessage(`Spawning debugger in directory ${cwd}`);
260260
this.logDebuggerMessage(` Command bundle ${args.join(" ")}`);
261-
this.logDebuggerMessage(
262-
` Environment ${JSON.stringify(configuration.env)}`,
263-
);
264261

265262
this.debugProcess = spawn("bundle", args, {
266263
shell: true,

vscode/src/ruby.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,7 @@ export class Ruby implements RubyInterface {
135135
new None(
136136
this.workspaceFolder,
137137
this.outputChannel,
138+
this.context,
138139
this.manuallySelectRuby.bind(this),
139140
workspaceRubyPath,
140141
),
@@ -170,6 +171,7 @@ export class Ruby implements RubyInterface {
170171
new None(
171172
this.workspaceFolder,
172173
this.outputChannel,
174+
this.context,
173175
this.manuallySelectRuby.bind(this),
174176
globalRubyPath,
175177
),
@@ -323,6 +325,7 @@ export class Ruby implements RubyInterface {
323325
new Asdf(
324326
this.workspaceFolder,
325327
this.outputChannel,
328+
this.context,
326329
this.manuallySelectRuby.bind(this),
327330
),
328331
);
@@ -332,6 +335,7 @@ export class Ruby implements RubyInterface {
332335
new Chruby(
333336
this.workspaceFolder,
334337
this.outputChannel,
338+
this.context,
335339
this.manuallySelectRuby.bind(this),
336340
),
337341
);
@@ -341,6 +345,7 @@ export class Ruby implements RubyInterface {
341345
new Rbenv(
342346
this.workspaceFolder,
343347
this.outputChannel,
348+
this.context,
344349
this.manuallySelectRuby.bind(this),
345350
),
346351
);
@@ -350,6 +355,7 @@ export class Ruby implements RubyInterface {
350355
new Rvm(
351356
this.workspaceFolder,
352357
this.outputChannel,
358+
this.context,
353359
this.manuallySelectRuby.bind(this),
354360
),
355361
);
@@ -359,6 +365,7 @@ export class Ruby implements RubyInterface {
359365
new Mise(
360366
this.workspaceFolder,
361367
this.outputChannel,
368+
this.context,
362369
this.manuallySelectRuby.bind(this),
363370
),
364371
);
@@ -368,6 +375,7 @@ export class Ruby implements RubyInterface {
368375
new RubyInstaller(
369376
this.workspaceFolder,
370377
this.outputChannel,
378+
this.context,
371379
this.manuallySelectRuby.bind(this),
372380
),
373381
);
@@ -377,6 +385,7 @@ export class Ruby implements RubyInterface {
377385
new Custom(
378386
this.workspaceFolder,
379387
this.outputChannel,
388+
this.context,
380389
this.manuallySelectRuby.bind(this),
381390
),
382391
);
@@ -386,6 +395,7 @@ export class Ruby implements RubyInterface {
386395
new None(
387396
this.workspaceFolder,
388397
this.outputChannel,
398+
this.context,
389399
this.manuallySelectRuby.bind(this),
390400
),
391401
);
@@ -395,6 +405,7 @@ export class Ruby implements RubyInterface {
395405
new Shadowenv(
396406
this.workspaceFolder,
397407
this.outputChannel,
408+
this.context,
398409
this.manuallySelectRuby.bind(this),
399410
),
400411
);

vscode/src/ruby/chruby.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,10 @@ export class Chruby extends VersionManager {
3131
constructor(
3232
workspaceFolder: vscode.WorkspaceFolder,
3333
outputChannel: WorkspaceChannel,
34+
context: vscode.ExtensionContext,
3435
manuallySelectRuby: () => Promise<void>,
3536
) {
36-
super(workspaceFolder, outputChannel, manuallySelectRuby);
37+
super(workspaceFolder, outputChannel, context, manuallySelectRuby);
3738

3839
const configuredRubies = vscode.workspace
3940
.getConfiguration("rubyLsp")
@@ -199,7 +200,7 @@ export class Chruby extends VersionManager {
199200
].join(";");
200201

201202
const result = await this.runScript(
202-
`${rubyExecutableUri.fsPath} -W0 -e '${script}'`,
203+
`${rubyExecutableUri.fsPath} -EUTF-8:UTF-8 -e '${script}'`,
203204
);
204205

205206
const [defaultGems, gemHome, yjit, version] =

vscode/src/ruby/none.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,11 @@ export class None extends VersionManager {
1919
constructor(
2020
workspaceFolder: vscode.WorkspaceFolder,
2121
outputChannel: WorkspaceChannel,
22+
context: vscode.ExtensionContext,
2223
manuallySelectRuby: () => Promise<void>,
2324
rubyPath?: string,
2425
) {
25-
super(workspaceFolder, outputChannel, manuallySelectRuby);
26+
super(workspaceFolder, outputChannel, context, manuallySelectRuby);
2627
this.rubyPath = rubyPath ?? "ruby";
2728
}
2829

vscode/src/ruby/versionManager.ts

Lines changed: 28 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -14,29 +14,29 @@ export interface ActivationResult {
1414
gemPath: string[];
1515
}
1616

17+
// Changes to either one of these values have to be synchronized with a corresponding update in `activation.rb`
1718
export const ACTIVATION_SEPARATOR = "RUBY_LSP_ACTIVATION_SEPARATOR";
19+
export const VALUE_SEPARATOR = "RUBY_LSP_VS";
20+
export const FIELD_SEPARATOR = "RUBY_LSP_FS";
1821

1922
export abstract class VersionManager {
20-
public activationScript = [
21-
`STDERR.print("${ACTIVATION_SEPARATOR}" + `,
22-
"{ env: ENV.to_h, yjit: !!defined?(RubyVM::YJIT), version: RUBY_VERSION, gemPath: Gem.path }.to_json + ",
23-
`"${ACTIVATION_SEPARATOR}")`,
24-
].join("");
25-
2623
protected readonly outputChannel: WorkspaceChannel;
2724
protected readonly workspaceFolder: vscode.WorkspaceFolder;
2825
protected readonly bundleUri: vscode.Uri;
2926
protected readonly manuallySelectRuby: () => Promise<void>;
3027

28+
private readonly context: vscode.ExtensionContext;
3129
private readonly customBundleGemfile?: string;
3230

3331
constructor(
3432
workspaceFolder: vscode.WorkspaceFolder,
3533
outputChannel: WorkspaceChannel,
34+
context: vscode.ExtensionContext,
3635
manuallySelectRuby: () => Promise<void>,
3736
) {
3837
this.workspaceFolder = workspaceFolder;
3938
this.outputChannel = outputChannel;
39+
this.context = context;
4040
this.manuallySelectRuby = manuallySelectRuby;
4141
const customBundleGemfile: string = vscode.workspace
4242
.getConfiguration("rubyLsp")
@@ -59,28 +59,33 @@ export abstract class VersionManager {
5959
// language server
6060
abstract activate(): Promise<ActivationResult>;
6161

62-
protected async runEnvActivationScript(activatedRuby: string) {
62+
protected async runEnvActivationScript(
63+
activatedRuby: string,
64+
): Promise<ActivationResult> {
65+
const activationUri = vscode.Uri.joinPath(
66+
this.context.extensionUri,
67+
"activation.rb",
68+
);
69+
6370
const result = await this.runScript(
64-
`${activatedRuby} -W0 -rjson -e '${this.activationScript}'`,
71+
`${activatedRuby} -EUTF-8:UTF-8 '${activationUri.fsPath}'`,
6572
);
6673

6774
const activationContent = new RegExp(
68-
`${ACTIVATION_SEPARATOR}(.*)${ACTIVATION_SEPARATOR}`,
75+
`${ACTIVATION_SEPARATOR}([^]*)${ACTIVATION_SEPARATOR}`,
6976
).exec(result.stderr);
7077

71-
return this.parseWithErrorHandling(activationContent![1]);
72-
}
73-
74-
protected parseWithErrorHandling(json: string) {
75-
try {
76-
return JSON.parse(json);
77-
} catch (error: any) {
78-
this.outputChannel.error(
79-
`Tried parsing invalid JSON environment: ${json}`,
80-
);
81-
82-
throw error;
83-
}
78+
const [version, gemPath, yjit, ...envEntries] =
79+
activationContent![1].split(FIELD_SEPARATOR);
80+
81+
return {
82+
version,
83+
gemPath: gemPath.split(","),
84+
yjit: yjit === "true",
85+
env: Object.fromEntries(
86+
envEntries.map((entry) => entry.split(VALUE_SEPARATOR)),
87+
),
88+
};
8489
}
8590

8691
// Runs the given command in the directory for the Bundle, using the user's preferred shell and inheriting the current
@@ -98,14 +103,12 @@ export abstract class VersionManager {
98103
this.outputChannel.info(
99104
`Running command: \`${command}\` in ${this.bundleUri.fsPath} using shell: ${shell}`,
100105
);
101-
this.outputChannel.debug(
102-
`Environment used for command: ${JSON.stringify(process.env)}`,
103-
);
104106

105107
return asyncExec(command, {
106108
cwd: this.bundleUri.fsPath,
107109
shell,
108110
env: process.env,
111+
encoding: "utf-8",
109112
});
110113
}
111114

vscode/src/test/suite/client.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ async function launchClient(workspaceUri: vscode.Uri) {
5050
get: (_name: string) => undefined,
5151
update: (_name: string, _value: any) => Promise.resolve(),
5252
},
53+
extensionUri: vscode.Uri.file(path.join(workspaceUri.fsPath, "vscode")),
5354
} as unknown as vscode.ExtensionContext;
5455
const fakeLogger = new FakeLogger();
5556
const outputChannel = new WorkspaceChannel("fake", fakeLogger as any);

vscode/src/test/suite/debugger.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,12 +198,14 @@ suite("Debugger", () => {
198198
'source "https://rubygems.org"\ngem "debug"',
199199
);
200200

201+
const extensionPath = path.dirname(path.dirname(path.dirname(__dirname)));
201202
const context = {
202203
subscriptions: [],
203204
workspaceState: {
204205
get: () => undefined,
205206
update: () => undefined,
206207
},
208+
extensionUri: vscode.Uri.file(extensionPath),
207209
} as unknown as vscode.ExtensionContext;
208210
const outputChannel = new WorkspaceChannel("fake", LOG_CHANNEL);
209211
const workspaceFolder: vscode.WorkspaceFolder = {

0 commit comments

Comments
 (0)