Add rust_analyzer rule to generate a rust-project.json #505

djmarcin · 2020-11-26T12:10:46Z

This PR builds on the work in #384 to add a rust_analyzer rule which generates a rust-project.json. This file allows rust-analyzer to function without cargo by describing the code structure, layout, and dependency graph.

I think this PR is ready for review, but looking to get feedback on the following:

The rule requires the user to manually enter their bazel info execution_root in order to locate remote crates and out_dirs. Is there a better way to retrieve this from inside starlark without resorting to genrules? I'd like to leave a genrule rewrite to a later date to unblock the relatively large win this PR brings.
The rule assumes that the generated rust-project.json will be used directly from bazel-bin and generates relative paths for the source code under the workspace root. If there is a way to get the absolute path to the workspace root, this would not be necessary.
How should this be tested?

Other than that, I think this is good to go.

UebelAndre

A few comments from a non maintainer 😀

You should also wire all this up to stardoc and run docs/update_docs.sh.

This is looking super awesome!!

UebelAndre · 2020-11-27T15:14:02Z

rust/private/rust_analyzer.bzl

+    for flag in ctx.rule.attr.rustc_flags:
+        # --cfg flags should be passed as well but as an atomic
+        # config, not key/value
+        if flag.startswith("--cfg"):


Should this not be --cfg=?

The = is optional, and could be a space. Updated to be explicit about both possibilities.

Ah cool 👍

The [6:] was throwing me off and seemed like an oversight. Thanks for updating that 🙏

rust/private/rust_analyzer.bzl

UebelAndre · 2020-11-27T15:21:04Z

rust/private/rust_analyzer.bzl

+            idx += 1
+            output["crates"].append(crate)
+
+    ctx.actions.write(output = ctx.outputs.filename, content = struct(**output).to_json())


You might want to add a todo or something to replace the to_json with https://docs.bazel.build/versions/master/skylark/lib/json.html just to let future authors know about it

Added a TODO, it's not in any released version yet

UebelAndre · 2020-11-27T15:22:37Z

rust/private/rust_analyzer.bzl

+            aspects = [rust_analyzer_aspect],
+            doc = "List of all targets to be included in the index",
+        ),
+        "exec_root": attr.string(


Very interested in avoiding this. Is there no way to get this infer this information?

The rule requires the user to manually enter their bazel info execution_root in order to locate remote crates and out_dirs. Is there a better way to retrieve this from inside starlark without resorting to genrules? I'd like to leave a genrule rewrite to a later date to unblock the relatively large win this PR brings.

Can you explain more on why needing execution_root is necessary?

I agree, this is the biggest issue with this PR. I believe the one real viable alternative is setting a custom --workspace_status_command https://docs.bazel.build/versions/master/user-manual.html#flag--workspace_status_command to provide this information via an environment variable, but very interested to know if I missed another way.

Maybe @damienmg or @mfarrugi might know?

The thing I want from this PR is to be able to push a change to my repo and have enabled rust-analyzer support for anyone with no additional work. Having someone need to explicitly set the output of bazel info execution_root sounds like it wouldn't support that. I think --workspace_status_command might be something that works? Since I could commit a .bazelrc file to the repo to enable these rules. But definitely interested in finding something more contained. I'm trying to find such a solution but am having no luck 😞

The reason why it's necessary to get this information is that we must provide a path to various external source files, e.g. the rust sysroot, any third party crates you depend on, etc. All of the paths generated during the action analysis phase are relative to the execution root, and are not available via the bazel-* symlinks, so it's not possible to generate a relative path from the user's source directory to the external sources.

I'm toying with the idea of a simple genrule that can get execution_root and uses sed as a template expander, but it's not portable (e.g. to windows). There is a built-in action ctx.actions.expand_template, but unfortunately in our case the substitution would be in an output and that rule requires it to be available as a starlark variable, so I don't think it will work.

I should also mention that I'm really not sure how --workspace_status_command or even the genfiles solution would work, because you can't recursively run bazel info in order to find the workspace at runtime. So, passing the exec_root is still the best solution I have that works at the moment, unsatisfying as it is.

If there's a path forward with genrule, you could use cmd_ps or cmd_bat to handle the windows specific behavior, right?

Ok a few updates here:

The bazel-{workspace} symlink does point at the execution root, I don't know how I missed that before. I'm not sure this actually helps because I'm not sure I can get the workspace name in a normal (non-repository) rule either, and also because of (2)

Due to how symlinks work, bazel-bin/.. is not your current directory, it is actually a path in the execution root. For example, if bazel-bin -> /home/vagrant/.cache/bazel/_bazel_vagrant/71ed060a30795ab0736b520a305b19cd/execroot/__main__/bazel-out/k8-fastbuild/bin then bazel-bin/.. is actually .../execroot/__main__/bazel-out/k8-fastbuild
This has a couple of consequences. First, the way this PR resolves local source code is wrong, but for whatever reason it doesn't look like this is breaking anything. Second, we could drop the exec_root parameter if we were willing to make the assumption that the execroot is at bazel-bin/../../../, which I think is valid and fairly safe. This is assuming that the generated rust-project.json will be used directly from bazel-bin, which is the current assumption made in this PR.

For some reason I don't fully understand yet, switching to entirely relative paths based on this assumption breaks rust-analyzer.

This is fixed by adding the gazelle-like utility run target.

rust/repositories.bzl

rust/private/rust_analyzer.bzl

UebelAndre · 2020-11-27T15:28:36Z

rust/private/rust_analyzer.bzl

+]
+
+RustAnalyzerInfo = provider(
+    "RustAnalyzerInfo holds rust crate metadata for targets",


Not, can you use the doc keyword here?

UebelAndre · 2021-02-11T18:57:17Z

@djmarcin Hey, can you take a moment to fix the conflicts?

@illicitonion @hlopko Can you guys take a look at this? I think it'd be best to get this merged and allow more people to use it and make incremental improvements to it.

djmarcin · 2021-02-14T09:53:35Z

I merged up to latest HEAD, but it seems that something in the intervening changes (or maybe a bad merge?) busted this PR so that now it gets stuck in an infinite loop inside bazel and eventually OOMs. This happens after the aspect evaluation but before the rust_analyzer rule executes, so I'm not entirely sure how to debug it.

Nevermind, it seems that my local bazel server was in a bad state. Restarting it seems to have fixed it.

illicitonion

Looks great - thanks!

djmarcin · 2021-02-16T19:30:19Z

Thanks for merging, here are some quick notes on the latest "how to use" and future work.

How to use:

Add the following to your WORKSPACE

load("@rules_rust//tools/rust_analyzer/raze:crates.bzl", "rules_rust_tools_rust_analyzer_fetch_remote_crates")
rules_rust_tools_rust_analyzer_fetch_remote_crates()

Add a rule to your root BUILD file like the following. You can generate the interior targets list with something like: bazel query 'kind("rust_library|rust_binary", //...:all)' | grep -v build_script | grep "//" | awk " { print \"'\" \$0 \"',\" }". Note that visibility rules apply.

rust_analyzer(
    name = "rust_analyzer",
    targets = [ /* all the binary/library targets you want in the rust-project.json */ ],
)

Run bazel run @rules_rust//tools/rust_analyzer:gen_rust_project whenever your dependencies change to regenerate rust-project.json. We .gitignore this file, because it's large and effectively just a build artifact. We use a VSCode task to run gen_rust_project automatically whenever VSCode starts, or manually whenever things get wonky. Once the rust-project.json file exists at your workspace root, rust-analyzer should automatically use it. We sometimes need to force a rust-analyzer restart to pick up changes.

Future work:

Since we're already running a separate binary for this, we should be able to autodetect the targets in the workspace without needing the root rust_analyzer rule. Eliminate the need for this rule.
Potentially, automatically include rules_rust_tools_rust_analyzer_fetch_remote_crates() when the user adds rust_repositories
Improve the documentation once 1 and maybe 2 are done.

Known issues:

The --remote_download family of flags breaks proc_macro support because the proc_macro .so files are not downloaded. rust_analyzer needs these files present so it can load and run them. Solution: remove the --remote_download flags.
rust-analyzer uses the source code that is downloaded and extracted during a build. If you run bazel clean, rust-analyzer will be unable to find the symbols. Solution: rerun bazel build

hlopko

I'm sorry, I didn't have time to do the thorough review :/

Rust analyzer is updated very often, do you plan to keep updating our checkout with every release? If not, maybe we should add a mechanism to choose an in-workspace version instead of the one bundled with the rules..

Could you check in the docs you mentioned in the last comment? That's already better than hunting for the PR to know how to use rust-analyzer.

I'll try to look into it more thoroughly soon. Thank you so much for working on this!

hlopko · 2021-02-16T19:58:39Z

rust/private/rust_analyzer.bzl

+to Cargo.toml files.
+"""
+
+load("//rust/platform:triple_mappings.bzl", "system_to_dylib_ext", "triple_to_system")


It looks like this file doesn't need anything special from the //rust/private and could potentially be moved out and only rely on the stable (non-private) APIs (BuildInfo and find_toolchain are certainly things that should be accessible through rust_common). By moving it out imho we:

dogfood our rust_common APIs

simplify the private package

impose stricter layering

Would you agree that moving it out is a reasonable thing to do?

Yes, I don't see any reason that would be a problem.

hlopko · 2021-02-16T20:48:21Z

rust/private/rust_analyzer.bzl

+        build_info = build_info,
+    )]
+
+def find_proc_macro_dylib_path(toolchain, target):


Nit: Why not leading underscore?

I can update these to leading underscore in a follow-up.

djmarcin · 2021-02-16T21:59:06Z

Rust analyzer is updated very often, do you plan to keep updating our checkout with every release? If not, maybe we should add a mechanism to choose an in-workspace version instead of the one bundled with the rules..

This PR does not bundle rust-analyzer, it merely generates a rust-project.json file, which appears to be a stable format.

This commit adds the instructions for setting up rust-analyzer support to the main documentation landing page. The instructions were originally given in bazelbuild#505.

benbrittain and others added 26 commits August 2, 2020 12:17

add rust-src to known shas

19d0e8f

rust-analyzer support

432444e

populate sysroot

82895e5

use workspace_root instead of path hack

29bc845

generate correct cfgs & workspace markers

8b12f2d

select proper source file

db78181

add basic example generating rust-project.json

f94e79c

fix example

4f8b492

implement suggested fixes

999f5df

Merge remote-tracking branch 'upstream/master'

3082f7a

Allow specifying sha256 for non-blessed nightly versions

610f3ae

Hardcoded paths and excessive debugging

18283bc

fix formatting

b1d24bc

remove debug statements

8bbd77b

Remove sysroot code, simplify file generation

9eb96ed

clippy stuff

e1d451a

Move to rust/private

a6dcdd3

Refactor generation code

536cf28

Merge remote-tracking branch 'upstream/master' into rust-analyzer

cfedaa8

Keep rustc-src in a subdirectory

7ebab90

Proc macro support and more standard src location

a5e2f9b

Fix include! macros

77f8c84

revert extraneous clippy change

1530692

Handle the rule at any depth in the workspace

8ab7fb3

revert extraneous changes

9703cbb

Remove CARGO_MANIFEST_DIR since it doesn't appear to work

a522e5d

google-cla bot added the cla: yes label Nov 26, 2020

djmarcin added 2 commits November 26, 2020 12:13

Fix example

0f487c9

update shas

005d389

UebelAndre reviewed Nov 27, 2020

View reviewed changes

djmarcin added 2 commits February 1, 2021 21:59

simplify with getattr

acd6c70

trim command output

a00d1a0

djmarcin added 3 commits February 14, 2021 06:17

Merge branch 'main' into rust-analyzer

bb92b67

fix merge

8702431

Refactor to use command line flags and bazel info to determine workspace

a3a1ddb

djmarcin force-pushed the rust-analyzer branch from ddbc8ef to a3a1ddb Compare February 14, 2021 09:22

Use the new rust_common.crate_info, but now this OOMs...

2b0d3e3

djmarcin force-pushed the rust-analyzer branch from bbf3c2f to 0a25a17 Compare February 14, 2021 10:15

Make buildifier happy

4a55fb8

djmarcin force-pushed the rust-analyzer branch from 0a25a17 to 4a55fb8 Compare February 14, 2021 10:15

djmarcin added 4 commits February 14, 2021 10:21

Regenerate documentation

5a1d449

Regenerate known_shas

d5d58a0

Remove extra /../ that isn't actually necessary

227ece9

Remove debugging statement

1aa7f8e

illicitonion approved these changes Feb 16, 2021

View reviewed changes

illicitonion merged commit 8a56582 into bazelbuild:main Feb 16, 2021

djmarcin deleted the rust-analyzer branch February 16, 2021 19:31

hlopko reviewed Feb 16, 2021

View reviewed changes

This was referenced Mar 2, 2021

Integrate with RLS #95

Closed

Support generating rust-analyzer rust-project.json #378

Closed

hlopko mentioned this pull request Mar 18, 2021

Documentation for rust-analyzer rule #636

Closed

yagehu mentioned this pull request Mar 27, 2021

Surfacing rust_analyzer usage documentation #648

Closed

yagehu mentioned this pull request Mar 27, 2021

Add rust-analyzer usage to documentation #649

Closed

UebelAndre mentioned this pull request Apr 22, 2021

Path to "non-experimental" rust_analyzer support #704

Closed

3 tasks

jon-flatley mentioned this pull request May 26, 2022

[bazel] Support rust-analyzer lowRISC/opentitan#12887

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rust_analyzer rule to generate a rust-project.json #505

Add rust_analyzer rule to generate a rust-project.json #505

djmarcin commented Nov 26, 2020 •

edited

Loading

UebelAndre left a comment

UebelAndre Nov 27, 2020

djmarcin Nov 27, 2020

UebelAndre Nov 27, 2020

UebelAndre Nov 27, 2020

djmarcin Nov 27, 2020

UebelAndre Nov 27, 2020

UebelAndre Nov 27, 2020

djmarcin Nov 27, 2020

UebelAndre Nov 27, 2020

djmarcin Nov 27, 2020

djmarcin Nov 27, 2020

UebelAndre Nov 27, 2020

djmarcin Nov 27, 2020 •

edited

Loading

djmarcin Nov 27, 2020

djmarcin Dec 2, 2020

UebelAndre Nov 27, 2020

UebelAndre commented Feb 11, 2021

djmarcin commented Feb 14, 2021 •

edited

Loading

illicitonion left a comment

djmarcin commented Feb 16, 2021

hlopko left a comment

hlopko Feb 16, 2021

djmarcin Feb 16, 2021

hlopko Feb 16, 2021

djmarcin Feb 16, 2021

djmarcin commented Feb 16, 2021

Add rust_analyzer rule to generate a rust-project.json #505

Add rust_analyzer rule to generate a rust-project.json #505

Conversation

djmarcin commented Nov 26, 2020 • edited Loading

UebelAndre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djmarcin Nov 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UebelAndre commented Feb 11, 2021

djmarcin commented Feb 14, 2021 • edited Loading

illicitonion left a comment

Choose a reason for hiding this comment

djmarcin commented Feb 16, 2021

hlopko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djmarcin commented Feb 16, 2021

djmarcin commented Nov 26, 2020 •

edited

Loading

djmarcin Nov 27, 2020 •

edited

Loading

djmarcin commented Feb 14, 2021 •

edited

Loading