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

Canonicalize rust-project.json path for determining the project root #14168

Closed
wants to merge 1 commit into from

Conversation

sdroege
Copy link

@sdroege sdroege commented Feb 16, 2023

The meson build system places an autogenerated rust-project.json inside the build directory instead of the source/project root. Using it with rust-analyzer requires additional configuration so that it is found.

Alternatively, by creating a symlink inside the project root to the file in the build directory it can be found automatically. Unfortunately the paths in rust-project.json are relative so rust-analyzer would use the location of the symlink to determine where files are placed and not find any files.

By resolving any symlink before determining it as project root this works correctly.

Only automatically discovered rust-project.json are handled like this. If specified via the configuration file then the configured path is taken as-is.

This also seems to be the behaviour of clangd when loading compile_commands.json, or at least the same approach with a symlink works for clangd.


meson PR to write absolute paths: mesonbuild/meson#11467

The meson build system places an autogenerated rust-project.json inside
the build directory instead of the source/project root. Using it with
rust-analyzer requires additional configuration so that it is found.

Alternatively, by creating a symlink inside the project root to the file
in the build directory it can be found automatically. Unfortunately the
paths in rust-project.json are relative so rust-analyzer would use the
location of the symlink to determine where files are placed and not find
any files.

By resolving any symlink before determining it as project root this
works correctly.

Only automatically discovered rust-project.json are handled like this.
If specified via the configuration file then the configured path is
taken as-is.

This also seems to be the behaviour of clangd when loading
compile_commands.json, or at least the same approach with a symlink
works for clangd.
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 16, 2023
let file = fs::read_to_string(&project_json).with_context(|| {
format!("Failed to read json file {}", project_json.display())
})?;
let data = serde_json::from_str(&file).with_context(|| {
format!("Failed to deserialize json file {}", project_json.display())
})?;
let project_location = project_json.parent().to_path_buf();
let project_location = project_json.parent().unwrap().to_path_buf();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could potentially move the canonicalization to ManifestPath to avoid the unwrap() here, or do the canonicalization in every case inside ProjectJson::new(), or do the canonicalization already when creating the ProjectManifest. Many options :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or alternatively the rust-project.json could have absolute paths generated by meson for root_module and others but not sure that's allowed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolute paths should work fine for root_module and friends. Not sure if canonicalizing is the right behavior we want here or not.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As relative paths can be used in rust-project.json it would make sense to at least resolve any symlinks for getting the real path of the file.

I'll also submit a PR to meson for writing absolute paths in there though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't resolving symlinks when loading rust-project.json break when the editor doesn't resolve symlinks before making lsp requests?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how that works in detail, but with nvim/coc it works fine this way

@sdroege
Copy link
Author

sdroege commented Feb 16, 2023

This also seems to be the behaviour of clangd when loading compile_commands.json, or at least the same approach with a symlink works for clangd.

Or maybe simply because the directory key in compile_commands.json is (always?) an absolute path

@sdroege
Copy link
Author

sdroege commented Feb 27, 2023

See also mesonbuild/meson#11467

@Veykril
Copy link
Member

Veykril commented Mar 15, 2023

The meson PR got merged from what I can tell, so is this issue still relevant for you? I am unsure whether we really want to resolve symlinks on the server side here

@sdroege
Copy link
Author

sdroege commented Mar 16, 2023

I'm fine with closing it here but rust-analyzer should probably refuse to load symlink rust-project.json that contain relative file locations to make it less hard to debug why such situations just don't work :)

@Veykril
Copy link
Member

Veykril commented Mar 16, 2023

Ye that is a good point :)

@Veykril Veykril closed this Mar 25, 2023
bors added a commit that referenced this pull request Mar 25, 2023
internal: Reject symlinks in project-json

cc #14168
bors added a commit that referenced this pull request Mar 29, 2023
fix: Canonicalize rust-project.json manifest path

Looked a bit more into this and I think we can do this after all, I don't see any place where this should break things
cc #14168 #14402 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants