[PATCH] Makefile: rust-analyzer target: better error handling and comments

[PATCH] Makefile: rust-analyzer target: better error handling and comments

[John Hubbard]

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(-)

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

base-commit: b050496579632f86ee1ef7e7501906db579f3457
-- 
2.45.1

Re: [PATCH] Makefile: rust-analyzer target: better error handling and comments

[Alice Ryhl]

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>

Re: [PATCH] Makefile: rust-analyzer target: better error handling and comments

[Miguel Ojeda]

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

Re: [PATCH] Makefile: rust-analyzer target: better error handling and comments

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

Re: [PATCH] Makefile: rust-analyzer target: better error handling and comments

[Miguel Ojeda]

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

Re: [PATCH] Makefile: rust-analyzer target: better error handling and comments

[Greg KH]

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

Re: [PATCH] Makefile: rust-analyzer target: better error handling and comments

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA