Skip to content

Commit

Permalink
Use bundled environment activation script file (#3083)
Browse files Browse the repository at this point in the history
### 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.
  • Loading branch information
vinistock committed Feb 5, 2025
1 parent 2be659e commit b11d323
Show file tree
Hide file tree
Showing 20 changed files with 416 additions and 178 deletions.
1 change: 1 addition & 0 deletions project-words
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ dont
eglot
Eglot
eruby
EUTF
exitstatus
EXTGLOB
fakehome
Expand Down
3 changes: 3 additions & 0 deletions vscode/activation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
env = ENV.map { |k, v| "#{k}RUBY_LSP_VS#{v}" }
env.unshift(RUBY_VERSION, Gem.path.join(","), !!defined?(RubyVM::YJIT))
STDERR.print("RUBY_LSP_ACTIVATION_SEPARATOR#{env.join("RUBY_LSP_FS")}RUBY_LSP_ACTIVATION_SEPARATOR")
2 changes: 1 addition & 1 deletion vscode/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"name": "ruby-lsp",
"displayName": "Ruby LSP",
"description": "VS Code plugin for connecting with the Ruby LSP",
"version": "0.8.20",
"version": "0.9.1",
"publisher": "Shopify",
"repository": {
"type": "git",
Expand Down
3 changes: 0 additions & 3 deletions vscode/src/debugger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,9 +258,6 @@ export class Debugger

this.logDebuggerMessage(`Spawning debugger in directory ${cwd}`);
this.logDebuggerMessage(` Command bundle ${args.join(" ")}`);
this.logDebuggerMessage(
` Environment ${JSON.stringify(configuration.env)}`,
);

this.debugProcess = spawn("bundle", args, {
shell: true,
Expand Down
11 changes: 11 additions & 0 deletions vscode/src/ruby.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ export class Ruby implements RubyInterface {
new None(
this.workspaceFolder,
this.outputChannel,
this.context,
this.manuallySelectRuby.bind(this),
workspaceRubyPath,
),
Expand Down Expand Up @@ -170,6 +171,7 @@ export class Ruby implements RubyInterface {
new None(
this.workspaceFolder,
this.outputChannel,
this.context,
this.manuallySelectRuby.bind(this),
globalRubyPath,
),
Expand Down Expand Up @@ -323,6 +325,7 @@ export class Ruby implements RubyInterface {
new Asdf(
this.workspaceFolder,
this.outputChannel,
this.context,
this.manuallySelectRuby.bind(this),
),
);
Expand All @@ -332,6 +335,7 @@ export class Ruby implements RubyInterface {
new Chruby(
this.workspaceFolder,
this.outputChannel,
this.context,
this.manuallySelectRuby.bind(this),
),
);
Expand All @@ -341,6 +345,7 @@ export class Ruby implements RubyInterface {
new Rbenv(
this.workspaceFolder,
this.outputChannel,
this.context,
this.manuallySelectRuby.bind(this),
),
);
Expand All @@ -350,6 +355,7 @@ export class Ruby implements RubyInterface {
new Rvm(
this.workspaceFolder,
this.outputChannel,
this.context,
this.manuallySelectRuby.bind(this),
),
);
Expand All @@ -359,6 +365,7 @@ export class Ruby implements RubyInterface {
new Mise(
this.workspaceFolder,
this.outputChannel,
this.context,
this.manuallySelectRuby.bind(this),
),
);
Expand All @@ -368,6 +375,7 @@ export class Ruby implements RubyInterface {
new RubyInstaller(
this.workspaceFolder,
this.outputChannel,
this.context,
this.manuallySelectRuby.bind(this),
),
);
Expand All @@ -377,6 +385,7 @@ export class Ruby implements RubyInterface {
new Custom(
this.workspaceFolder,
this.outputChannel,
this.context,
this.manuallySelectRuby.bind(this),
),
);
Expand All @@ -386,6 +395,7 @@ export class Ruby implements RubyInterface {
new None(
this.workspaceFolder,
this.outputChannel,
this.context,
this.manuallySelectRuby.bind(this),
),
);
Expand All @@ -395,6 +405,7 @@ export class Ruby implements RubyInterface {
new Shadowenv(
this.workspaceFolder,
this.outputChannel,
this.context,
this.manuallySelectRuby.bind(this),
),
);
Expand Down
5 changes: 3 additions & 2 deletions vscode/src/ruby/chruby.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,10 @@ export class Chruby extends VersionManager {
constructor(
workspaceFolder: vscode.WorkspaceFolder,
outputChannel: WorkspaceChannel,
context: vscode.ExtensionContext,
manuallySelectRuby: () => Promise<void>,
) {
super(workspaceFolder, outputChannel, manuallySelectRuby);
super(workspaceFolder, outputChannel, context, manuallySelectRuby);

const configuredRubies = vscode.workspace
.getConfiguration("rubyLsp")
Expand Down Expand Up @@ -199,7 +200,7 @@ export class Chruby extends VersionManager {
].join(";");

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

const [defaultGems, gemHome, yjit, version] =
Expand Down
3 changes: 2 additions & 1 deletion vscode/src/ruby/none.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ export class None extends VersionManager {
constructor(
workspaceFolder: vscode.WorkspaceFolder,
outputChannel: WorkspaceChannel,
context: vscode.ExtensionContext,
manuallySelectRuby: () => Promise<void>,
rubyPath?: string,
) {
super(workspaceFolder, outputChannel, manuallySelectRuby);
super(workspaceFolder, outputChannel, context, manuallySelectRuby);
this.rubyPath = rubyPath ?? "ruby";
}

Expand Down
53 changes: 28 additions & 25 deletions vscode/src/ruby/versionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,29 +14,29 @@ export interface ActivationResult {
gemPath: string[];
}

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

export abstract class VersionManager {
public activationScript = [
`STDERR.print("${ACTIVATION_SEPARATOR}" + `,
"{ env: ENV.to_h, yjit: !!defined?(RubyVM::YJIT), version: RUBY_VERSION, gemPath: Gem.path }.to_json + ",
`"${ACTIVATION_SEPARATOR}")`,
].join("");

protected readonly outputChannel: WorkspaceChannel;
protected readonly workspaceFolder: vscode.WorkspaceFolder;
protected readonly bundleUri: vscode.Uri;
protected readonly manuallySelectRuby: () => Promise<void>;

private readonly context: vscode.ExtensionContext;
private readonly customBundleGemfile?: string;

constructor(
workspaceFolder: vscode.WorkspaceFolder,
outputChannel: WorkspaceChannel,
context: vscode.ExtensionContext,
manuallySelectRuby: () => Promise<void>,
) {
this.workspaceFolder = workspaceFolder;
this.outputChannel = outputChannel;
this.context = context;
this.manuallySelectRuby = manuallySelectRuby;
const customBundleGemfile: string = vscode.workspace
.getConfiguration("rubyLsp")
Expand All @@ -59,28 +59,33 @@ export abstract class VersionManager {
// language server
abstract activate(): Promise<ActivationResult>;

protected async runEnvActivationScript(activatedRuby: string) {
protected async runEnvActivationScript(
activatedRuby: string,
): Promise<ActivationResult> {
const activationUri = vscode.Uri.joinPath(
this.context.extensionUri,
"activation.rb",
);

const result = await this.runScript(
`${activatedRuby} -W0 -rjson -e '${this.activationScript}'`,
`${activatedRuby} -EUTF-8:UTF-8 '${activationUri.fsPath}'`,
);

const activationContent = new RegExp(
`${ACTIVATION_SEPARATOR}(.*)${ACTIVATION_SEPARATOR}`,
`${ACTIVATION_SEPARATOR}([^]*)${ACTIVATION_SEPARATOR}`,
).exec(result.stderr);

return this.parseWithErrorHandling(activationContent![1]);
}

protected parseWithErrorHandling(json: string) {
try {
return JSON.parse(json);
} catch (error: any) {
this.outputChannel.error(
`Tried parsing invalid JSON environment: ${json}`,
);

throw error;
}
const [version, gemPath, yjit, ...envEntries] =
activationContent![1].split(FIELD_SEPARATOR);

return {
version,
gemPath: gemPath.split(","),
yjit: yjit === "true",
env: Object.fromEntries(
envEntries.map((entry) => entry.split(VALUE_SEPARATOR)),
),
};
}

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

return asyncExec(command, {
cwd: this.bundleUri.fsPath,
shell,
env: process.env,
encoding: "utf-8",
});
}

Expand Down
1 change: 1 addition & 0 deletions vscode/src/test/suite/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ async function launchClient(workspaceUri: vscode.Uri) {
get: (_name: string) => undefined,
update: (_name: string, _value: any) => Promise.resolve(),
},
extensionUri: vscode.Uri.file(path.join(workspaceUri.fsPath, "vscode")),
} as unknown as vscode.ExtensionContext;
const fakeLogger = new FakeLogger();
const outputChannel = new WorkspaceChannel("fake", fakeLogger as any);
Expand Down
2 changes: 2 additions & 0 deletions vscode/src/test/suite/debugger.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,12 +198,14 @@ suite("Debugger", () => {
'source "https://rubygems.org"\ngem "debug"',
);

const extensionPath = path.dirname(path.dirname(path.dirname(__dirname)));
const context = {
subscriptions: [],
workspaceState: {
get: () => undefined,
update: () => undefined,
},
extensionUri: vscode.Uri.file(extensionPath),
} as unknown as vscode.ExtensionContext;
const outputChannel = new WorkspaceChannel("fake", LOG_CHANNEL);
const workspaceFolder: vscode.WorkspaceFolder = {
Expand Down
21 changes: 13 additions & 8 deletions vscode/src/test/suite/ruby.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,12 @@ import { Ruby, ManagerIdentifier } from "../../ruby";
import { WorkspaceChannel } from "../../workspaceChannel";
import { LOG_CHANNEL } from "../../common";
import * as common from "../../common";
import { ACTIVATION_SEPARATOR } from "../../ruby/versionManager";
import { Shadowenv, UntrustedWorkspaceError } from "../../ruby/shadowenv";
import {
ACTIVATION_SEPARATOR,
FIELD_SEPARATOR,
VALUE_SEPARATOR,
} from "../../ruby/versionManager";

import { FAKE_TELEMETRY } from "./fakeTelemetry";

Expand All @@ -30,6 +34,7 @@ suite("Ruby environment activation", () => {
get: () => undefined,
update: () => undefined,
},
extensionUri: vscode.Uri.file(path.join(workspacePath, "vscode")),
} as unknown as vscode.ExtensionContext;
const outputChannel = new WorkspaceChannel("fake", LOG_CHANNEL);

Expand Down Expand Up @@ -125,16 +130,16 @@ suite("Ruby environment activation", () => {
},
} as unknown as vscode.WorkspaceConfiguration);

const envStub = {
env: { ANY: "true" },
yjit: true,
version: "3.3.5",
gemPath: ["~/.gem/ruby/3.3.5", "/opt/rubies/3.3.5/lib/ruby/gems/3.3.0"],
};
const envStub = [
"3.3.5",
"~/.gem/ruby/3.3.5,/opt/rubies/3.3.5/lib/ruby/gems/3.3.0",
"true",
`ANY${VALUE_SEPARATOR}true`,
].join(FIELD_SEPARATOR);

const execStub = sinon.stub(common, "asyncExec").resolves({
stdout: "",
stderr: `${ACTIVATION_SEPARATOR}${JSON.stringify(envStub)}${ACTIVATION_SEPARATOR}`,
stderr: `${ACTIVATION_SEPARATOR}${envStub}${ACTIVATION_SEPARATOR}`,
});

const ruby = new Ruby(
Expand Down
Loading

0 comments on commit b11d323

Please sign in to comment.