| Message ID | 20240601004856.206682-1-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Makefile: rust-analyzer target: better error handling and comments | expand |
On Sat, Jun 1, 2024 at 2:49 AM John Hubbard <jhubbard@nvidia.com> wrote: > > 1) Provide a more self-explanatory error message for the "Rust not > available" case. Without this patch, if Rust is not set up properly > (which happens a lot, seeing as how one must routinely run "rustup > override ..." with each new kernel release), the "make rust-analyzer" > invocation generates a somewhat confusing message: > > "No rule to make target 'rust-analyzer" > > This is confusing at first, because there is, in fact, a rust-analyzer > build target. It's just not set up to handle errors gracefully. > > Instead of inflicting that on the developer, just print that Rust is > not available, with a blank line above and below, so it doesn't get lost > in the noise. Now the error case looks like this: > > $ make rust-analyzer > > Rust is not available > > make[1]: *** [/kernel_work/linux-github/Makefile:1975: rust-analyzer] Error 1 > make: *** [Makefile:240: __sub-make] Error 2 > > 2) As long as I'm there, also add some documentation about what > rust-analyzer provides. > > Signed-off-by: John Hubbard <jhubbard@nvidia.com> Reviewed-by: Alice Ryhl <aliceryhl@google.com> Tested-by: Alice Ryhl <aliceryhl@google.com>
On Sat, Jun 1, 2024 at 2:49 AM John Hubbard <jhubbard@nvidia.com> wrote: > > Rust is not available Maybe we should use the `***` notation that is used elsewhere? > 2) As long as I'm there, also add some documentation about what > rust-analyzer provides. Perhaps this could go in a separate patch. But those are nits -- if Masahiro is OK with this approach: Acked-by: Miguel Ojeda <ojeda@kernel.org> Happy to take it too. Cheers, Miguel
On 6/19/24 5:25 AM, Miguel Ojeda wrote: > On Sat, Jun 1, 2024 at 2:49 AM John Hubbard <jhubbard@nvidia.com> wrote: >> >> Rust is not available > > Maybe we should use the `***` notation that is used elsewhere? What exactly did you have in mind for how that should look? The "make rustavailable" target has some leading *** and some bare statements, so I'm not quite sure exactly how to lay it out: $ make rustavailable *** *** Rust bindings generator 'bindgen' is too new. This may or may not work. *** Your version: 0.69.4 *** Expected version: 0.65.1 *** *** *** Please see Documentation/rust/quick-start.rst for details *** on how to set up the Rust support. *** Rust is available! > >> 2) As long as I'm there, also add some documentation about what >> rust-analyzer provides. > > Perhaps this could go in a separate patch. Sure, I'll split it out if you all prefer. > > But those are nits -- if Masahiro is OK with this approach: > > Acked-by: Miguel Ojeda <ojeda@kernel.org> > > Happy to take it too. > > Cheers, > Miguel thanks,
On Thu, Jun 20, 2024 at 8:13 AM John Hubbard <jhubbard@nvidia.com> wrote: > > What exactly did you have in mind for how that should look? The > "make rustavailable" target has some leading *** and some bare > statements, so I'm not quite sure exactly how to lay it out: I was thinking something like: *** *** Rust is not available. *** (the `***` prefix is used also in other similar scripts and by Make itself). However, thinking about it a bit more, we should perhaps just let `rust_is_available.sh` tell the user why it fails, since it is likely the next step the user would do anyway: $ make LLVM=1 rust-analyzer *** *** Rust compiler 'rustc' is too old. *** Your version: 1.62.0 *** Minimum version: 1.78.0 *** *** *** Please see Documentation/rust/quick-start.rst for details *** on how to set up the Rust support. *** make[1]: *** [linux/Makefile:1973: rust-analyzer] Error 1 make: *** [Makefile:240: __sub-make] Error 2 What do you think? Then there is no need for extra output here and the patch becomes simpler too. The bare statement we have there for the successful case was mainly so that the explicit `make rustavailable` did not look empty if there was no issue, i.e. we don't print anything extra when there is an error (and if we wanted to print something for the failure case, then we should probably do it in the script, rather than here). Cheers, Miguel
On Thu, Jun 20, 2024 at 10:31:53AM +0200, Miguel Ojeda wrote: > On Thu, Jun 20, 2024 at 8:13 AM John Hubbard <jhubbard@nvidia.com> wrote: > > > > What exactly did you have in mind for how that should look? The > > "make rustavailable" target has some leading *** and some bare > > statements, so I'm not quite sure exactly how to lay it out: > > I was thinking something like: > > *** > *** Rust is not available. > *** > > (the `***` prefix is used also in other similar scripts and by Make itself). > > However, thinking about it a bit more, we should perhaps just let > `rust_is_available.sh` tell the user why it fails, since it is likely > the next step the user would do anyway: > > $ make LLVM=1 rust-analyzer > *** > *** Rust compiler 'rustc' is too old. > *** Your version: 1.62.0 > *** Minimum version: 1.78.0 > *** > *** > *** Please see Documentation/rust/quick-start.rst for details > *** on how to set up the Rust support. > *** > make[1]: *** [linux/Makefile:1973: rust-analyzer] Error 1 > make: *** [Makefile:240: __sub-make] Error 2 > > What do you think? Then there is no need for extra output here and the > patch becomes simpler too. As someone who just ran into the "wait, how do I get rust to build on this machine again?" problem, yes, having the link to the documentation right there would be helpful. I did know where to find it, but others might not, and it's free to add. thanks, greg k-h
On 6/20/24 1:45 AM, Greg KH wrote: > On Thu, Jun 20, 2024 at 10:31:53AM +0200, Miguel Ojeda wrote: >> On Thu, Jun 20, 2024 at 8:13 AM John Hubbard <jhubbard@nvidia.com> wrote: >>> >>> What exactly did you have in mind for how that should look? The >>> "make rustavailable" target has some leading *** and some bare >>> statements, so I'm not quite sure exactly how to lay it out: >> >> I was thinking something like: >> >> *** >> *** Rust is not available. >> *** >> >> (the `***` prefix is used also in other similar scripts and by Make itself). >> >> However, thinking about it a bit more, we should perhaps just let >> `rust_is_available.sh` tell the user why it fails, since it is likely >> the next step the user would do anyway: >> >> $ make LLVM=1 rust-analyzer >> *** >> *** Rust compiler 'rustc' is too old. >> *** Your version: 1.62.0 >> *** Minimum version: 1.78.0 >> *** >> *** >> *** Please see Documentation/rust/quick-start.rst for details >> *** on how to set up the Rust support. >> *** >> make[1]: *** [linux/Makefile:1973: rust-analyzer] Error 1 >> make: *** [Makefile:240: __sub-make] Error 2 >> >> What do you think? Then there is no need for extra output here and the >> patch becomes simpler too. Yes, that's perfect, actually. > As someone who just ran into the "wait, how do I get rust to build on > this machine again?" problem, yes, having the link to the documentation > right there would be helpful. I did know where to find it, but others > might not, and it's free to add. > > thanks, > > greg k-h Right, we get this for free by just letting scripts/rust_is_available.sh report its results "out loud". I'll post a v2, and with the comment part split into a separate patch as requested. thanks,
diff --git a/Makefile b/Makefile index f975b6396328..aca2c96820aa 100644 --- a/Makefile +++ b/Makefile @@ -1967,9 +1967,13 @@ quiet_cmd_tags = GEN $@ tags TAGS cscope gtags: FORCE $(call cmd,tags) -# IDE support targets +# Generate rust-project.json, which does for Rust what clangd's +# compile_commands.json does for C/C++: provides a browsing database for code +# editors and IDEs. PHONY += rust-analyzer rust-analyzer: + $(Q)$(CONFIG_SHELL) $(srctree)/scripts/rust_is_available.sh 2>/dev/null || \ + { echo; echo "Rust is not available"; echo; false; } $(Q)$(MAKE) $(build)=rust $@ # Script to generate missing namespace dependencies
1) Provide a more self-explanatory error message for the "Rust not available" case. Without this patch, if Rust is not set up properly (which happens a lot, seeing as how one must routinely run "rustup override ..." with each new kernel release), the "make rust-analyzer" invocation generates a somewhat confusing message: "No rule to make target 'rust-analyzer" This is confusing at first, because there is, in fact, a rust-analyzer build target. It's just not set up to handle errors gracefully. Instead of inflicting that on the developer, just print that Rust is not available, with a blank line above and below, so it doesn't get lost in the noise. Now the error case looks like this: $ make rust-analyzer Rust is not available make[1]: *** [/kernel_work/linux-github/Makefile:1975: rust-analyzer] Error 1 make: *** [Makefile:240: __sub-make] Error 2 2) As long as I'm there, also add some documentation about what rust-analyzer provides. Signed-off-by: John Hubbard <jhubbard@nvidia.com> --- Makefile | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) base-commit: b050496579632f86ee1ef7e7501906db579f3457