Skip to content

Support repo-relative paths in rust-analyzer.server.path #13939

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

Closed
OmarTawfik opened this issue Jan 13, 2023 · 11 comments · Fixed by #13993
Closed

Support repo-relative paths in rust-analyzer.server.path #13939

OmarTawfik opened this issue Jan 13, 2023 · 11 comments · Fixed by #13993
Labels
A-config configuration

Comments

@OmarTawfik
Copy link
Contributor

The VS Code configuration rust-analyzer.server.path exists to use a local binary, which is great when testing stuff, and also when using from a dev container. However, it expects a full path, which doesn't work well with version-controlled binaries. For example, if I set it to:

    // relative to VS Code workspace/repo root
    "rust-analyzer.server.path": "bin/rust-analyzer",

The extension will fail, printing the following error to Rust Analyzer Client VS Code output channel:

INFO [1/12/2023, 7:10:00 PM]: Starting language client
INFO [1/12/2023, 7:10:00 PM]: Using server binary at bin/rust-analyzer
ERROR [1/12/2023, 7:10:00 PM]: Bootstrap error Error: Failed to execute bin/rust-analyzer --version. `config.server.path` or `config.serverPath` has been set explicitly. Consider removing this config or making a valid server binary available at that path.

I wanted to propose executing that command relative to the workspace directory, to allow users to specify paths that are relative to the repository they are working on, instead of forcing them to use machine-wide installations.

@OmarTawfik OmarTawfik changed the title Support relative paths in rust-analyzer.server.path Support repo-relative paths in rust-analyzer.server.path Jan 13, 2023
@Veykril Veykril added the A-config configuration label Jan 13, 2023
@lnicola
Copy link
Member

lnicola commented Jan 13, 2023

If you can set PATH in your environment, that should also work.

@OmarTawfik
Copy link
Contributor Author

@lnicola .. Thanks for the suggestion! But as it requires changing the environment BEFORE loading VS Code, I'm not sure it would be ideal/as easy for most use cases:

  1. It won't work if people are switching between different projects in the same window (open recent/^R hotkey for example).
  2. It would require much more complex setup in case of running remote containers, or using GitHub workspaces, etc...
  3. In some cases, it would defeat the purpose of letting repositories specify their own paths/configs. i.e. if I already know the path to the repository-specific binary (without opening it first), I can already set my machine-local settings.json.

@OmarTawfik
Copy link
Contributor Author

I think the fix would come to setting cwd in the spanSync() call on the following line to the value of workspace.uri.fsPath. I assume there are no further security concerns, if we also check the workspace.isTrusted flag. Please let me know if I missed something.

export function isValidExecutable(path: string): boolean {
log.debug("Checking availability of a binary at", path);
const res = spawnSync(path, ["--version"], { encoding: "utf8" });
const printOutput = res.error && (res.error as any).code !== "ENOENT" ? log.warn : log.debug;
printOutput(path, "--version:", res);
return res.status === 0;
}

@bjorn3
Copy link
Member

bjorn3 commented Jan 14, 2023

I think you can set PATH using rust-analyzer.server.extraEnv, which can be put in the project specific configuration.

@OmarTawfik
Copy link
Contributor Author

@bjorn3 thanks for the suggestion. can you please clarify? something like below won't work, since:

  • It won't expand existin $PATH values.
  • It won't still know how to resolve ${workspaceFolder}.
 "rust-analyzer.server.extraEnv": {
    "PATH": "$PATH:${workspaceFolder}/bin"
  }

@bjorn3
Copy link
Member

bjorn3 commented Jan 18, 2023

I see, I assumed it would work.

@Veykril
Copy link
Member

Veykril commented Jan 19, 2023

"rust-analyzer.server.path": "${workspaceFolder}/bin/rust-analyzer",

should work

@OmarTawfik
Copy link
Contributor Author

OmarTawfik commented Jan 19, 2023

@Veykril It does not. See logs below:

INFO [1/19/2023, 2:05:40 PM]: Starting language client
INFO [1/19/2023, 2:05:40 PM]: Using server binary at ${workspaceFolder}/bin/rust-analyzer
ERROR [1/19/2023, 2:05:40 PM]: Bootstrap error Error: Failed to execute ${workspaceFolder}/bin/rust-analyzer --version. `config.server.path` or `config.serverPath` has been set explicitly. Consider removing this config or making a valid server binary available at that path.

It has been a while since I worked with VS Code extensions, but my understanding is that VSCode itself does not resolve ${var} variables in settings.json before passing it to extensions. It only does so for tasks.json. It also doesn't provide an API for extensions to resolve such variables themselves. So that resolution would have to be implemented on rust-analyzer side, or as my initial suggestion, set cwd when executing that command to the workspace folder directly.

See this for more information: microsoft/vscode#2809

@Veykril
Copy link
Member

Veykril commented Jan 20, 2023

Ah, apologies, we do support variable substitution (we re-implemented it), but we do it after spawning the server I think. That is, that should work but it currently doesn't.

@Veykril
Copy link
Member

Veykril commented Jan 20, 2023

const newEnv = substituteVariablesInEnv(
Object.assign({}, process.env, this.config.serverExtraEnv)
);
const run: lc.Executable = {
command: this._serverPath,
options: { env: newEnv },
};

Yep, we pull the server path first, then substitute the configs, that's the wrong order.

@OmarTawfik
Copy link
Contributor Author

@Veykril thanks for explaining! I sent #13993 to fix it.

@bors bors closed this as completed in 9a6294d Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-config configuration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants