`rust-project check TARGET` #767

cormacrelf · 2024-09-04T05:24:45Z

Context: relies on the as-yet unmerged rust-lang/rust-analyzer#18043, which adds {label} interpolation in rust-analyzer flycheck commands and allows rust-project develop-json to supply the flycheck commands.

Previously rust-project worked with the $saved_file interpolation in rust-analyzer. This PR makes it work with a target label, substituted as {label} instead. So it's not really backwards compatible, but we could make it so. Submitting this now to get feedback.

We also now emit flycheck runnables that configure rust-analyzer to do flychecks using rust-project check.

Also:

rust-project develop-json --use-clippy false
Don't just exit silently if check.bxl fails, show us what went wrong
Errors out if your rustc sysroot does not have the rust-src component. This was an undocumented requirement and rust-analyzer just doesn't work without it, so we should fail as early as possible.
#[cfg(fbcode_build)]s the existing buck2 test runnables, the new ones work with the rustc_test harness and OSS buck2.

Attn @davidbarsky

…bute

By fixing & removing the workaround for missing JSON output that swallowed all the BXL errors too.

…otherwise

This became necessary when I was debugging a sysroot that by mistake did not have rust-src due to being a "minimal" rustup toolchain. I figure that might be a common mistake.

davidbarsky · 2024-09-05T16:06:48Z

Previously rust-project worked with the $saved_file interpolation in rust-analyzer. This PR makes it work with a target label, substituted as {label} instead. So it's not really backwards compatible, but we could make it so. Submitting this now to get feedback.

My feeling is that we should move over to {label} can be done in a month or so after this PR lands and we can simply get rid of $saved_file in rust-project. I think we'll need to keep $saved_file in rust-analyzer for a little while longer, as I know the Fuchsia folks use it.

cormacrelf · 2024-09-05T21:59:06Z

Only problem is there are other buck users that are adopting rust-project and using saved file. Maybe it's still few enough people that we can break the CLI.

davidbarsky · 2024-09-05T16:35:56Z

rust-toolchain

@@ -11,4 +11,4 @@

 # @rustc_version: rustc 1.80.0-nightly (804421dff 2024-06-07)
 channel = "nightly-2024-06-08"
-components = ["llvm-tools-preview","rustc-dev","rust-src"]
+components = ["llvm-tools-preview","rustc-dev","rust-src", "rust-analyzer"]


this is for neovim support, right?

For any support -- you need this component to have the proc macro server even if you're using a custom build of rust analyzer, IIRC. Just generally very difficult to contribute without it!

ah! despite being on the rust-analyzer team, I didn't know that rust-analyzer implies the proc macro server as well.

davidbarsky · 2024-09-05T16:37:46Z

integrations/rust-project/src/json_project.rs

+    /// It also claims that `${sysroot}/lib/rustlib/src/rust/library` is the
+    /// default value. But it fails to add `std` and `core` as dependencies
+    /// if you do not provide a value. So we will always provide one.


I don't know if that's inaccurate documentation or an intentional change: Rust-for-Linux/linux@fe99216. I'll try to clarify.

#756 did the same thing. I was just carrying this patch for a while and documented it a little. I kinda think it is a bug in RA.

davidbarsky · 2024-09-05T16:46:20Z

integrations/rust-project/src/buck.rs

@@ -485,6 +550,24 @@ impl Buck {
        cmd
    }

+    /// Return the absolute path of the CELL root for a given buck target label


Suggested change

/// Return the absolute path of the CELL root for a given buck target label

/// Return the absolute path of the cell root for a given target.

davidbarsky · 2024-09-05T16:47:06Z

integrations/rust-project/src/buck.rs

-        runnables: vec![
+    let mut runnables = vec![
+        Runnable {
+            kind: RunnableKind::Flycheck,


whoops, i thought i called this check. we should probably resolve this in rust-analyzer, but i'm not sure in favor of what.

I think check should, in VSCode, open the terminal with the text rendered diagnostics from cargo/buck etc. Very often the inline diagnostics from LSP do not live up to the standard set by rustc's text renderer and you need to see it in the terminal to understand what rustc is honking about. Flycheck should be for the JSON output.

The existing "check" runnable just runs buck2 build, which is what I'm describing already.

davidbarsky · 2024-09-06T13:30:03Z

integrations/rust-project/src/sysroot.rs

+    })
+}
+
+fn validate(sysroot_src: &Path) -> Result<(), anyhow::Error> {


naive question: how would you feel about reducing this to a rustup component list and scanning for a rust-src? rustup doesn't really have machine-readable output, but that might simplify this a bit...

Doesn't work if you use Nix without rustup. Which I do. This way works for all methods including, for example, a custom sysroot built by buck.

Ah, gotcha. I need to remind myself of what rust-analyzer does to handle sysroots, but checking whether core gives me some slight unease: I think if $SYSROOT/lib/rustlib/src/rust/library exists, I'm not sure how useful it'd be to check that $SYSROOT/lib/rustlib/src/rust/library/core also exists: something with the packaging has already gone terribly wrong if core (or any other path!) doesn't exist in the sysroot_src directory.

Stepping back, I'll also check how stable the directory layout of sysroot_src is and figure out when/how we might need to change it in rust-project and rust-analyzer. My personal feeling is that I'm fine if it's not stable, but I'd prefer to only make the change in one spot—rust-analyzer, not rust-analyzer and rust-project. I'd like to have that solved for every user of rust-analyzer, not just the somewhat nice minority who use buck2 and rust-analyzer. There's also substantial logistical benefits: a change in rust-lang/rust could also immediately update rust-analyzer's logic in a single PR.

(For context, I use two workflows: rustup install nightly for Cargo-based projects + a Buck-managed sysroot for Buck stuff.)

davidbarsky · 2024-09-06T13:58:08Z

Only problem is there are other buck users that are adopting rust-project and using saved file. Maybe it's still few enough people that we can break the CLI.

Gotcha! I think we should keep the $saved_file stuff around for a month or so, but remove it then. I think while a few folks might be using it, but I think a ping on the buck2 fans Discord + some time should be enough should be enough to migrate to the {label} approach.

For what it's worth, I always viewed the $saved_file stuff as temporary workaround and my goal with rust-project was to always make the integration as Cargo-like as possible to ensure that I'd be imposing the smallest possible support burden on rust-analyzer, and {label} feels more conceptually minimal than $saved_file—the former, at least, can be reused with Cargo, but $saved_file can't.

cormacrelf added 7 commits September 4, 2024 14:47

TEMPORARY: add rust-analyzer to rust-toolchain so OSS devs can contri…

a838ef6

…bute

rust-project: make it possible to debug errors in check.bxl again

f8d80ea

By fixing & removing the workaround for missing JSON output that swallowed all the BXL errors too.

rust-project check TARGET instead of SAVED_FILE

839aa95

Add some OSS-compatible runnables

4c8384f

rust-project develop --use-clippy false

5b59f18

rust-project: Always write sysroot_src as std/core are not available …

0917216

…otherwise

Fully custom --sysroot --sysroot-src + sanity checks

0790b05

This became necessary when I was debugging a sysroot that by mistake did not have rust-src due to being a "minimal" rustup toolchain. I figure that might be a common mistake.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 4, 2024

use cfg(fbcode_build) to determine which runnables to add

fe75587

davidbarsky reviewed Sep 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`rust-project check TARGET` #767

`rust-project check TARGET` #767

cormacrelf commented Sep 4, 2024 •

edited

Loading

davidbarsky commented Sep 5, 2024

cormacrelf commented Sep 5, 2024

davidbarsky Sep 5, 2024

cormacrelf Sep 6, 2024

davidbarsky Sep 6, 2024

davidbarsky Sep 5, 2024

cormacrelf Sep 6, 2024 •

edited

Loading

davidbarsky Sep 5, 2024

davidbarsky Sep 5, 2024

cormacrelf Sep 6, 2024 •

edited

Loading

davidbarsky Sep 6, 2024

cormacrelf Sep 6, 2024

davidbarsky Sep 6, 2024

davidbarsky commented Sep 6, 2024

	/// Return the absolute path of the CELL root for a given buck target label
	/// Return the absolute path of the cell root for a given target.

rust-project check TARGET #767

Are you sure you want to change the base?

rust-project check TARGET #767

Conversation

cormacrelf commented Sep 4, 2024 • edited Loading

davidbarsky commented Sep 5, 2024

cormacrelf commented Sep 5, 2024

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

cormacrelf Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cormacrelf Sep 6, 2024 • 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

davidbarsky commented Sep 6, 2024

`rust-project check TARGET` #767

`rust-project check TARGET` #767

cormacrelf commented Sep 4, 2024 •

edited

Loading

cormacrelf Sep 6, 2024 •

edited

Loading

cormacrelf Sep 6, 2024 •

edited

Loading