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

Integration with rust-analyzer's automatic project discovery #2755

Open
davidbarsky opened this issue Jul 22, 2024 · 14 comments · May be fixed by #3073
Open

Integration with rust-analyzer's automatic project discovery #2755

davidbarsky opened this issue Jul 22, 2024 · 14 comments · May be fixed by #3073

Comments

@davidbarsky
Copy link

Hi folks! I added Cargo-style automatic project discovery in rust-analyzer for non-Cargo build systems (Buck primarily, but it's designed to be usable by Bazel...) in rust-lang/rust-analyzer#17246. The documentation for this feature is in the manual, under "rust-analyzer.workspace.discoverConfig". I've been using it with Buck for the last month and a half, and while some details are still likely to change, the general shape of the API is in a spot that I'm happy with. I think this would make the Rust IDE experience with Bazel a lot nicer and I'd be very happy to provide guidance on integrating this feature into rules_rust!

@jondo2010
Copy link
Contributor

For reference, the Buck-side implementation of this can be found here: https://github.com/facebook/buck2/tree/main/integrations/rust-project/src

@otiv-emiel-vanseveren
Copy link

otiv-emiel-vanseveren commented Jul 23, 2024

Hi, @davidbarsky I read a few of the discussions on RAtoml (including: rust-lang/rust-analyzer#17246). But I don't fully understand the concept/advantages of ra's automatic project discovery. Would you mind explaining? In what ways will it improve the Rust Bazel IDE experience?

@davidbarsky
Copy link
Author

But I don't fully understand the concept/advantages of ra's automatic project discovery. Would you mind explaining? In what ways will it improve the Rust Bazel IDE experience?

I think it improves the situation in two major ways:

  1. For VS Code, Bazel directs users to setup a task that generates a rust-project.json for the entire workspace. In my opinion as a rust-analyzer team member, this has two downsides:
    1. Generating a rust-project.json for the entire workspace might not scale to larger workspaces. I don't believe this is an inherent limitation to @rules_rust//tools/rust_analyzer:gen_rust_project , but feature: teach rust-analyzer to discover linked_projects rust-lang/rust-analyzer#17246 can be substantially more fine-grained with what is and isn't indexed.
    2. @rules_rust//tools/rust_analyzer:gen_rust_project runs on editor starting up, but doesn't necessarily refresh the workspace if dependencies are added or removed. The PR I linked to can reload when a BUILD file changes.
  2. Since the functionality lives in the rust-analyzer server, support across editors wouldn't require per-editor work—the required integrations are O(build_system), not O(build system * editor). I also think that the video I attached to the PR is pretty nice—it looks decently Cargo-like.

@otiv-emiel-vanseveren
Copy link

otiv-emiel-vanseveren commented Jul 23, 2024

Thanks for the explanation!

  1. Generating a rust-project.json for the entire workspace might not scale to larger workspaces. I don't believe this is an inherent limitation to @rules_rust//tools/rust_analyzer:gen_rust_project

Does this mean it could generate based on the open buffers / crate you're currently in ?

@davidbarsky
Copy link
Author

Does this mean it could generate based on the open buffers / crate you're currently in ?

Yes, but you can still can still generate a rust-project.json for the entire workspace if somthing like /... is provided as an argument to the command—the scaling is just a bit more graceful.

@illicitonion
Copy link
Collaborator

Thanks for the pointer @davidbarsky, this looks really nice!

I'd happily review a contribution if someone wanted to put one together :)

@otiv-emiel-vanseveren
Copy link

otiv-emiel-vanseveren commented Aug 15, 2024

Hi @davidbarsky, I’d like to contribute, but I’ll need some guidance. It looks like I'll need to update the existing rust-project-gen similar to the one Buck has (reference above)? mainly what buck's rust-project develop(+json) and check cmds implement? What else is needed?

Does rust-analyzer provide the currently edited files?

@davidbarsky
Copy link
Author

Sorry, I thought I replied, @otiv-emiel-vanseveren.

It looks like I'll need to update the existing rust-project-gen similar to the one Buck has (reference above)?

Yup!

mainly what buck's rust-project develop(+json) and check cmds implement? What else is needed?
That's all there is! In the manual, look for rust-analyzer.workspace.discoverConfig.

@sam-mccall
Copy link
Contributor

sam-mccall commented Nov 29, 2024

This is great :-)
I have a hacked up script that works-ish: https://gist.github.com/sam-mccall/1d0dedf61f45529394f4ae54345c0d25. It walks up from root/a/b/c/d.rs to find root/a/b/BUILD, and then runs gen_rust_project //a/b:all and returns the result to rust_analyzer.

The main awkwardness is relative paths:
rust-analyzer interprets them relative to the BUILD file (virtual root/a/b/rust-project.json)
Bazel would really prefer paths to be relative to the workspace root (virtual root/rust-project.json).
The latter seems more natural because the paths aren't all in root/a/b/* but also dependencies root/c/d/* and also root/sysroot/*.

@davidbarsky I guess Buck has the same issue and forcing the paths to be relative anyway (../../sysroot/*) is the thing to do?

(My script just pokes around making all the paths absolute, which is ugly but works. We can't generate absolute paths in the first place, because the hermetic build doesn't know its location)

@davidbarsky
Copy link
Author

@sam-mccall

As a preface, I don't really understand or know Blaze/Bazel, so I might say things that don't really make sense or aren't applicable.

I have a hacked up script that works-ish: https://gist.github.com/sam-mccall/1d0dedf61f45529394f4ae54345c0d25. It walks up from root/a/b/c/d.rs to find root/a/b/BUILD, and then runs gen_rust_project //a/b:all and returns the result to rust_analyzer.

Nice!

The main awkwardness is relative paths:
rust-analyzer interprets them relative to the BUILD file (virtual root/a/b/rust-project.json)
Bazel would really prefer paths to be relative to the workspace root (virtual root/rust-project.json).
The latter seems more natural because the paths aren't all in root/a/b/* but also dependencies root/c/d/* and also root/sysroot/*.

A question and a comment:

  • Question: in those paths, root is @, right?
  • Comment: rust-analyzer can interpret paths in two ways: relative to the BUILD or as absolute paths. I could see an argument for a third (relative to @) inside of rust-analyzer for Bazel and Buck-style build systems, or for virtualized file systems, where you can't necessarily materialize files onto a traditional file system.

I guess Buck has the same issue and forcing the paths to be relative anyway (../../sysroot/*) is the thing to do?

It does have the same issue, but no: we force all paths to be absolute in two steps:

  1. When inside of Buck with a BXL script (think adhoc build graph analysis/exploration, but written in Starlark), we make all the paths relative to the the project root ("@").
    1. Buck's "project root" is equivalent to Bazel/Blaze's "workspace root". I don't know if Bazel/Blaze have this feature, but I can find Buck's project root by running buck root --kind=project.
  2. Once the BXL script returns to the rust-project Rust CLI that invoked it, the rust-project CLI typically joins the all the paths with the project root. We only keep the paths relative to the workspace root when we generate a Glean index.

This allows us to keep the core, heavy work of generating a rust-project.json hermetic, but have impure post-processing/telemetry/etc. live in something that doesn't care about hermeticity. Let me know if the thing I described can work for Bazel/Blaze: if not, we can explore adding a third option to rust-analyzer (relative to @).

sam-mccall added a commit to sam-mccall/rules_rust that referenced this issue Dec 2, 2024
This is a step towards supporting automatic project discovery (bazelbuild#2755).

Relative paths are resolved against the location of rust-project.json,
but in auto-discovery this is the location of the BUILD file being
discovered, not the workspace root.

We already use absolute paths for generated files, so rust-project.json
is not workspace-independent today.

The alternatives seem more complex for little gain:

- Only use absolute paths for auto-discovery mode, relative otherwise.
- Use ../../ to express all workspace paths relative to the BUILD.

See bazelbuild#2755 (comment)
@sam-mccall
Copy link
Contributor

Thanks David!

I have a hacked up script that works-ish: https://gist.github.com/sam-mccall/1d0dedf61f45529394f4ae54345c0d25. It walks up from root/a/b/c/d.rs to find root/a/b/BUILD, and then runs gen_rust_project //a/b:all and returns the result to rust_analyzer.

  • Question: in those paths, root is @, right?

Yeah, I think technically root/ in the filesystem is @// in blaze-label terms (main repository, root).
But I think multi-repo is not so relevant here, so I'd usually just call it // (current repository, root).

(To be honest, I live in our monorepo and don't understand multi-repo too well)

I guess Buck has the same issue and forcing the paths to be relative anyway (../../sysroot/*) is the thing to do?

It does have the same issue, but no: we force all paths to be absolute in two steps:

Ah, that seems easier. The existing gen_rust_project tool already does this as you describe for generated files (which aren't in the workspace) - the hermetic piece emits placeholders which the driver piece fills in. Reusing this for paths inside the workspace sounds good.

The structure we have is very similar to yours: hermetic piece of aspect/BXL, driver glue of gen_rust_project/rust-project.

I could see an argument for a third (relative to @) inside of rust-analyzer for Bazel and Buck-style build systems, or for virtualized file systems, where you can't necessarily materialize files onto a traditional file system.
...
We only keep the paths relative to the workspace root when we generate a Glean index.

As it happens I'm also trying to set up rust-analyzer based static analysis, which will use VFS, but I think we can just pick a fixed concrete layout for the VFS (e.g. /src, /generated, /sysroot) and modify the driver to fill those into the placeholders.

@sam-mccall
Copy link
Contributor

@otiv-emiel-vanseveren are you currently working on this?

It looks like there are a few separable pieces:

  • absolute file paths as discussed above (I sent rust_analyzer: make all paths in rust-project.json absolute #3033)
  • running a separate --output_base bazel instance so the tool doesn't fight with the user over the bazel lock
  • make the tool work outside bazel run so it can be invoked by rust-analyzer
  • speak the required argv/stdout protocol

I'd like to contribute whatever's needed to get this working. Please let me know if there are things you have in flight!

@otiv-emiel-vanseveren
Copy link

otiv-emiel-vanseveren commented Dec 2, 2024

Hi @sam-mccall, unfortunately I did not have the time yet. Looking forward to your contributions!

Please let me know if there are things you have in flight!

I initially started working on something very similar to buck's RA cli tool, the one in rules_rust is not that nice.

github-merge-queue bot pushed a commit that referenced this issue Dec 2, 2024
This is a step towards supporting automatic project discovery (#2755).

Relative paths are resolved against the location of rust-project.json,
but in auto-discovery this is the location of the BUILD file being
discovered, not the workspace root.

We already use absolute paths for generated files, so rust-project.json
is not workspace-independent today.

The alternatives seem more complex for little gain:

- Only use absolute paths for auto-discovery mode, relative otherwise.
- Use ../../ to express all workspace paths relative to the BUILD.

See
#2755 (comment)
@davidbarsky
Copy link
Author

@sam-mccall

Yeah, I think technically root/ in the filesystem is @// in blaze-label terms (main repository, root). But I think multi-repo is not so relevant here, so I'd usually just call it // (current repository, root).

(To be honest, I live in our monorepo and don't understand multi-repo too well)

Thankfully, it's not too hard to support multiple repos! We also technically support multiple repos, but if you know where your impure driver is being invoked from, it's generally pretty easy to normalize the paths to the correct repo root. We do this in Buck's rust-project because people might have multiple checkouts of fbsource for one reason or another, and multiple checkouts are cheap.

Ah, that seems easier. The existing gen_rust_project tool already does this as you describe for generated files (which aren't in the workspace) - the hermetic piece emits placeholders which the driver piece fills in. Reusing this for paths inside the workspace sounds good.

The structure we have is very similar to yours: hermetic piece of aspect/BXL, driver glue of gen_rust_project/rust-project.

Cool, yeah! I'm glad we converged on the same solution!

As it happens I'm also trying to set up rust-analyzer based static analysis, which will use VFS, but I think we can just pick a fixed concrete layout for the VFS (e.g. /src, /generated, /sysroot) and modify the driver to fill those into the placeholders.

Yeah, for Glean—code indexing—it's static analysis if you squint and we simply make it relative to the repo root. Everything is keyed off fbsource (e.g., fbsource/fbcode, fbsource/third-party, etc. I forget where the sysroot lives, but it's one of those folders and it's enough).

speak the required argv/stdout protocol

Lemme know how that protocol feels: I'm happy to change/evolve it in response to feedback! It was very Buck-centric, so if anything feels off, let me know!

@bobozaur bobozaur linked a pull request Dec 10, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants