feature: Substitute $saved_file in custom check commands #15476

Wilfred · 2023-08-17T23:54:06Z

If the custom command has a $saved_file placeholder, and we know the file being saved, replace the placeholder and run a check command.

If there's a placeholder and we don't know the saved file, do nothing.

This is a simplified version of #15381, which I hope is easier to review.

davidbarsky · 2023-08-18T02:17:58Z

Do we want to try moving these changes atop of my changes, maybe? Could simply review, potentially.

Wilfred · 2023-08-21T21:41:32Z

@davidbarsky Since this is a smaller change overall, I hope this is an easier review in the current form. I've amended your commit here, so the attribution is still clear :)

bors · 2023-08-29T07:33:15Z

☔ The latest upstream changes (presumably #15465) made this pull request unmergeable. Please resolve the merge conflicts.

P1n3appl3 · 2023-09-19T22:28:36Z

I've been using this patch on Fuchsia's codebase (a rust-project.json workspace with thousands of crates) and this dramatically improves the latency of seeing lints upon saving, especially when editing a crate deep in the dep graph.

I think you can close #12882 if this lands.

bors · 2023-09-22T16:27:43Z

☔ The latest upstream changes (presumably #15492) made this pull request unmergeable. Please resolve the merge conflicts.

Wilfred · 2023-10-23T23:16:02Z

Rebased. We're successfully using this patch internally and would love to see it upstreamed. @Veykril is there anything else you need from me here?

crates/flycheck/src/lib.rs

lnicola · 2023-11-20T18:30:21Z

crates/flycheck/src/lib.rs

+                    } else {
+                        // The custom command has a $saved_file placeholder,
+                        // but we had an IDE event that wasn't a file save. Do nothing.
+                        return None;


This means there's going to be no initial check when loading the project, but I don't see any way around it without configuring two check commands.

I guess this is not a problem for you, but Cargo users trying this might still be surprised (ref. #12882).

I suppose a workaround would be #15892, but yes, in this PR? it might confuse Cargo users.

(That being said, this is very much not a feature for Cargo, but I think that #15892 could make it a feature for Cargo users and resolve #12882 in the process).

An example this should work for is rust-lang/rust's cargo wrapper, ./x.py check.

I reckon: If the command has $saved_file in it, return None. If it doesn't, return Some, as it does not require substitution. That brings back the current behaviour until you start using $saved_file.

bors · 2024-01-02T10:09:06Z

☔ The latest upstream changes (presumably #16062) made this pull request unmergeable. Please resolve the merge conflicts.

P1n3appl3 · 2024-01-30T19:42:04Z

Rebased. We're successfully using this patch internally and would love to see it upstreamed. @Veykril is there anything else you need from me here?

Sorry to keep pinging this thread but we're in the same spot as @Wilfred. We'd prefer to avoid standing up our own fork of r-a for Fuchsia with just this patch applied and I'd be happy to help get this to a mergeable state if there's actionable work to be done.

@Veykril is this gated on review or are there already issues with it you'd like someone to address?

davidbarsky · 2024-01-30T21:29:54Z

Sorry to keep pinging this thread but we're in the same spot as @Wilfred. We'd prefer to avoid standing up our own fork of r-a for Fuchsia with just this patch applied and I'd be happy to help get this to a mergeable state if there's actionable work to be done.

@P1n3appl3 I'm increasingly of the opinion that the right solution would be building atop of #15892, which feels substantially less like a hack than the approach that @Wilfred commandeered. Would that approach fork work for Fuschia? (It's not clear to me if Fuschia builds with Bazel today or if does its own thing).

P1n3appl3 · 2024-01-30T22:39:13Z

I was worried you'd say that 😛. I agree that #15892 is a less hacky approach and we'd definitely like something like it in the long run, but I was hoping that we could land a simple replacement variable for overrideCommand given that it solves our biggest pain-point (single-crate flycheck in a 6000+ crate workspace).

If merging a stopgap isn't desirable then I'm ok with pulling this change into our rust-analyzer mirror for now and hopefully getting back to using upstream once the more robust solution lands. It felt a bit silly for us and wilfred to separately keep rebasing this patch while we wait, but it's not much code so I'm not too worried about it.

To answer your question Fuchsia mostly uses GN/Ninja for rust, though the codebase is generally moving towards bazel. That doesn't really concern us in terms of this issue though because they're all generating rust-project.json and all have the ability to run a check build and/or clippy on a single crate if they're given the info about which file was saved.

davidbarsky · 2024-01-31T02:04:52Z

I was worried you'd say that 😛. I agree that #15892 is a less hacky approach and we'd definitely like something like it in the long run, but I was hoping that we could land a simple replacement variable for overrideCommand given that it solves our biggest pain-point (single-crate flycheck in a 6000+ crate workspace).

That's certainly fair! I've been meaning to get back to #16135. I'll wire up the flycheck string to actually run flycheck in that PR in the next day or so and ping you once it's ready.

If merging a stopgap isn't desirable then I'm ok with pulling this change into our rust-analyzer mirror for now and hopefully getting back to using upstream once the more robust solution lands. It felt a bit silly for us and wilfred to separately keep rebasing this patch while we wait, but it's not much code so I'm not too worried about it.

In fairness, it's you, Wilfred, and I rebasing this change, as Wilfred and I work together 😄.

To answer your question Fuchsia mostly uses GN/Ninja for rust, though the codebase is generally moving towards bazel. That doesn't really concern us in terms of this issue though because they're all generating rust-project.json and all have the ability to run a check build and/or clippy on a single crate if they're given the info about which file was saved.

Gotcha! Can GN/Ninja build a crate if they're provided with a target label (to use Bazel/Buck terminology)? Do you currently infer the owning target(s) from the saved file here to build said crate?

(I'm asking to ensure that the PR I linked to works for Fuschia.)

Veykril · 2024-02-12T14:42:16Z

Went ahead and rebased. As said earlier, I'll merge this for now as a temporary thing until we get #16135 going
@bors r+

bors · 2024-02-12T14:42:19Z

📌 Commit 155e38b has been approved by Veykril

It is now in the queue for this repository.

bors · 2024-02-12T14:43:26Z

⌛ Testing commit 155e38b with merge 88b867d...

Substitute $saved_file in custom check commands If the custom command has a $saved_file placeholder, and we know the file being saved, replace the placeholder and run a check command. If there's a placeholder and we don't know the saved file, do nothing. This is a simplified version of #15381, which I hope is easier to review.

If the custom command has a $saved_file placeholder, and we know the file being saved, replace the placeholder and then run a check command. If there's a placeholder and we don't know the saved file, do nothing.

Veykril · 2024-02-12T14:46:07Z

@bors r+

bors · 2024-02-12T14:46:09Z

📌 Commit cdbf54f has been approved by Veykril

It is now in the queue for this repository.

bors · 2024-02-12T14:59:39Z

⌛ Testing commit cdbf54f with merge 1811210...

bors · 2024-02-12T15:10:54Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 1811210 to master...

The `--compile_one_dependency` flag ([docs](https://bazel.build/docs/user-manual#compile-one-dependency)) changes `bazel build` etc. to accept a file path and build the target corresponding to that file. This is useful for check-on-save with rust-analyzer in combination with the newly-added `$saved_file` command substitution (rust-lang/rust-analyzer#15476). Officially `--compile_one_dependency` only supports the builtin C++ and Java rules, but an [undocumented flag](https://github.com/bazelbuild/bazel/blob/7.1.1/src/main/java/com/google/devtools/build/lib/packages/Attribute.java#L102) can be added to attributes to turn them into sources supporting `--compile_one_dependency`. I'm not sure what the status of this support is, but it appears to work for all bazel versions up to at least 7.1.1, and if support is removed the flag is pretty harmless. Before this change: ``` > bazel build --compile_one_dependency tools/rust_analyzer/main.rs WARNING: Target pattern parsing failed. ERROR: Couldn't find dependency on target '//tools/rust_analyzer:main.rs' ERROR: Couldn't find dependency on target '//tools/rust_analyzer:main.rs' INFO: Elapsed time: 0.956s INFO: 0 processes. ERROR: Build did NOT complete successfully ``` After: ``` > bazel build --compile_one_dependency tools/rust_analyzer/main.rs INFO: Analyzed target //tools/rust_analyzer:gen_rust_project (0 packages loaded, 0 targets configured). INFO: Found 1 target... Target //tools/rust_analyzer:gen_rust_project up-to-date: bazel-bin/tools/rust_analyzer/gen_rust_project INFO: Elapsed time: 0.341s, Critical Path: 0.00s INFO: 1 process: 1 internal. INFO: Build completed successfully, 1 total action ``` --------- Co-authored-by: Daniel Wagner-Hall <[email protected]>

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 17, 2023

Wilfred force-pushed the implement-saved-file3 branch 3 times, most recently from a7464f6 to 79fa8f6 Compare August 21, 2023 23:45

Wilfred force-pushed the implement-saved-file3 branch from 79fa8f6 to f061b31 Compare August 29, 2023 15:52

davidbarsky force-pushed the implement-saved-file3 branch from f061b31 to 07effec Compare September 11, 2023 17:35

Veykril self-assigned this Sep 23, 2023

Wilfred force-pushed the implement-saved-file3 branch from 07effec to b5eafbf Compare October 4, 2023 14:36

Wilfred force-pushed the implement-saved-file3 branch from b5eafbf to 097033c Compare October 23, 2023 22:33

davidbarsky mentioned this pull request Nov 2, 2023

flycheck: initial implementation of $saved_file #15381

Closed

davidbarsky mentioned this pull request Nov 14, 2023

Add an Optional CargoTargetSpec-like Data to rust-project.json #15892

Open

davidbarsky force-pushed the implement-saved-file3 branch 2 times, most recently from c61adae to ebea717 Compare November 20, 2023 17:55

lnicola reviewed Nov 20, 2023

View reviewed changes

crates/flycheck/src/lib.rs Show resolved Hide resolved

lnicola reviewed Nov 20, 2023

View reviewed changes

davidbarsky deleted the implement-saved-file3 branch December 4, 2023 19:14

davidbarsky mentioned this pull request Dec 15, 2023

Extend TargetSpec functionality to rust-project.json #16135

Closed

Wilfred force-pushed the implement-saved-file3 branch 2 times, most recently from 008f797 to d2fbc83 Compare January 27, 2024 00:24

flycheck: initial implementation of $saved_file

cdbf54f

If the custom command has a $saved_file placeholder, and we know the file being saved, replace the placeholder and then run a check command. If there's a placeholder and we don't know the saved file, do nothing.

Veykril force-pushed the implement-saved-file3 branch from 155e38b to cdbf54f Compare February 12, 2024 14:45

bors merged commit 1811210 into rust-lang:master Feb 12, 2024
11 checks passed

P1n3appl3 mentioned this pull request Feb 12, 2024

Support flychecking only the current crate #12882

Closed

lnicola changed the title ~~Substitute $saved_file in custom check commands~~ feature: Substitute $saved_file in custom check commands Feb 13, 2024

william-smith-skydio mentioned this pull request Apr 1, 2024

Add support for --compile_one_dependency bazelbuild/rules_rust#2598

Merged

philips-software-forest-releaser bot mentioned this pull request Jun 4, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#436

Merged

This was referenced Jun 11, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#445

Merged

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#447

Merged

philips-software-forest-releaser bot mentioned this pull request Jun 18, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#456

Merged

philips-software-forest-releaser bot mentioned this pull request Jun 25, 2024

chore(deps,rust): update rust-lang.rust-analyzer philips-software/amp-devcontainer#469

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Substitute $saved_file in custom check commands #15476

feature: Substitute $saved_file in custom check commands #15476

Wilfred commented Aug 17, 2023 •

edited

Loading

davidbarsky commented Aug 18, 2023

Wilfred commented Aug 21, 2023 •

edited

Loading

bors commented Aug 29, 2023

P1n3appl3 commented Sep 19, 2023

bors commented Sep 22, 2023

Wilfred commented Oct 23, 2023

lnicola Nov 20, 2023

davidbarsky Nov 20, 2023

cormacrelf Nov 21, 2023

bors commented Jan 2, 2024

P1n3appl3 commented Jan 30, 2024

davidbarsky commented Jan 30, 2024

P1n3appl3 commented Jan 30, 2024

davidbarsky commented Jan 31, 2024

Veykril commented Feb 12, 2024

bors commented Feb 12, 2024

bors commented Feb 12, 2024

Veykril commented Feb 12, 2024

bors commented Feb 12, 2024

bors commented Feb 12, 2024

bors commented Feb 12, 2024

feature: Substitute $saved_file in custom check commands #15476

feature: Substitute $saved_file in custom check commands #15476

Conversation

Wilfred commented Aug 17, 2023 • edited Loading

davidbarsky commented Aug 18, 2023

Wilfred commented Aug 21, 2023 • edited Loading

bors commented Aug 29, 2023

P1n3appl3 commented Sep 19, 2023

bors commented Sep 22, 2023

Wilfred commented Oct 23, 2023

lnicola Nov 20, 2023

Choose a reason for hiding this comment

davidbarsky Nov 20, 2023

Choose a reason for hiding this comment

cormacrelf Nov 21, 2023

Choose a reason for hiding this comment

bors commented Jan 2, 2024

P1n3appl3 commented Jan 30, 2024

davidbarsky commented Jan 30, 2024

P1n3appl3 commented Jan 30, 2024

davidbarsky commented Jan 31, 2024

Veykril commented Feb 12, 2024

bors commented Feb 12, 2024

bors commented Feb 12, 2024

Veykril commented Feb 12, 2024

bors commented Feb 12, 2024

bors commented Feb 12, 2024

bors commented Feb 12, 2024

Wilfred commented Aug 17, 2023 •

edited

Loading

Wilfred commented Aug 21, 2023 •

edited

Loading