[V2] docs: rust: Clarify that 'rustup override' applies to build directory

  Rustup override is required to be set for the build directory and not
necessarily the kernel source tree (unless the build directory is its
subdir).

Clarify the same in quick-start guide.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2:
- Made few changes based on review comments.

 Documentation/rust/quick-start.rst | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
  

On Tue, Dec 12, 2023 at 8:43 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Rustup override is required to be set for the build directory and not
> necessarily the kernel source tree (unless the build directory is its
> subdir).
>
> Clarify the same in quick-start guide.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
  

On 12/12/23 08:43, Viresh Kumar wrote:
> Rustup override is required to be set for the build directory and not
> necessarily the kernel source tree (unless the build directory is its
> subdir).
> 
> Clarify the same in quick-start guide.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>
  

On 12/12/2023 07:43, Viresh Kumar wrote:
> Rustup override is required to be set for the build directory and not
> necessarily the kernel source tree (unless the build directory is its
> subdir).
> 
> Clarify the same in quick-start guide.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > ---
> V2:
> - Made few changes based on review comments.
> 
>   Documentation/rust/quick-start.rst | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/rust/quick-start.rst b/Documentation/rust/quick-start.rst
> index f382914f4191..7ea931f74e09 100644
> --- a/Documentation/rust/quick-start.rst
> +++ b/Documentation/rust/quick-start.rst
> @@ -33,14 +33,18 @@ A particular version of the Rust compiler is required. Newer versions may or
>   may not work because, for the moment, the kernel depends on some unstable
>   Rust features.
>   
> -If ``rustup`` is being used, enter the checked out source code directory
> -and run::
> +If ``rustup`` is being used, enter the kernel build directory (or use
> +`--path=<build-dir>` argument to the `set` sub-command) and run::
>   
>   	rustup override set $(scripts/min-tool-version.sh rustc)
>   

`scripts/min-tool-version.sh` won't exist within the build dir if the 
option the user takes is "enter the kernel build directory", right? It 
only works if they use the `--path` argument in the `rustup override 
set` option.

I gave this a spin and works as expected, just thought I would mention 
this given how users sometimes simply copy/paste and this may be confusing.

Tiago.
  

On Thu, Dec 14, 2023 at 2:26 PM Tiago Lam <tiagolam@gmail.com> wrote:
>
> `scripts/min-tool-version.sh` won't exist within the build dir if the
> option the user takes is "enter the kernel build directory", right? It
> only works if they use the `--path` argument in the `rustup override
> set` option.

Yeah, the script is in the source tree, and the path is the build
tree. Giving a single one-liner with `--path <builddir>` and
`<srctree>/scripts...` would be simplest in the sense that it would
allow us to remove even the "enter ..." part too. But then the command
cannot be copy-pasted and it is likely harder for newcomers that may
not be using `O=`.

Something like v1 but a bit simpler, e.g. keeping things as they are,
but with just a sentence after the command like "If you are building
the kernel with `O=`, i.e. specifying an output directory, then you
should append `--path <builddir>`." could work.

Or we could just provide a `rustupoverride` Make target to do this for
us [1], since we have all the information needed and would be
copy-pasteable by everybody. I can send it as a non-mangled patch and
then Viresh can redo this one on top using it.

Cheers,
Miguel

[1]

diff --git a/Makefile b/Makefile
index 70fc4c11dfc0..7fe82dd4dc6f 100644
--- a/Makefile
+++ b/Makefile
@@ -276,7 +276,8 @@ no-dot-config-targets := $(clean-targets) \
                         cscope gtags TAGS tags help% %docs check% coccicheck \
                         $(version_h) headers headers_% archheaders
archscripts \
                         %asm-generic kernelversion %src-pkg dt_binding_check \
-                        outputmakefile rustavailable rustfmt rustfmtcheck
+                        outputmakefile rustavailable rustfmt rustfmtcheck \
+                        rustupoverride
 no-sync-config-targets := $(no-dot-config-targets) %install
modules_sign kernelrelease \
                          image_name
 single-targets := %.a %.i %.ko %.lds %.ll %.lst %.mod %.o %.rsi %.s
%.symtypes %/
@@ -1611,6 +1612,7 @@ help:
        @echo  '                    (requires kernel .config;
downloads external repos)'
        @echo  '  rust-analyzer   - Generate rust-project.json
rust-analyzer support file'
        @echo  '                    (requires kernel .config)'
+       @echo  '  rustupoverride  - Set up a rustup override for the
build directory'
        @echo  '  dir/file.[os]   - Build specified target only'
        @echo  '  dir/file.rsi    - Build macro expanded source,
similar to C preprocessing.'
        @echo  '                    Run with RUSTFMT=n to skip
reformatting if needed.'
@@ -1735,6 +1737,11 @@ rustfmt:
 rustfmtcheck: rustfmt_flags = --check
 rustfmtcheck: rustfmt

+# `rustup override` setup target
+PHONY += rustupoverride
+rustupoverride:
+       $(Q)rustup override set $(shell
$(srctree)/scripts/min-tool-version.sh rustc)
+
 # Misc
 # ---------------------------------------------------------------------------
  

On Thu, Dec 14, 2023 at 6:22 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Or we could just provide a `rustupoverride` Make target to do this for
> us [1], since we have all the information needed and would be
> copy-pasteable by everybody. I can send it as a non-mangled patch and
> then Viresh can redo this one on top using it.

Patch at: https://lore.kernel.org/rust-for-linux/20231214222253.116734-1-ojeda@kernel.org/

Cheers,
Miguel
  

On 14-12-23, 18:22, Miguel Ojeda wrote:
> Something like v1 but a bit simpler, e.g. keeping things as they are,
> but with just a sentence after the command like "If you are building
> the kernel with `O=`, i.e. specifying an output directory, then you
> should append `--path <builddir>`." could work.
> 
> Or we could just provide a `rustupoverride` Make target to do this for
> us [1], since we have all the information needed and would be
> copy-pasteable by everybody. I can send it as a non-mangled patch and
> then Viresh can redo this one on top using it.

How about this ?

diff --git a/Documentation/rust/quick-start.rst b/Documentation/rust/quick-start.rst
index f382914f4191..367b06f3edc2 100644
--- a/Documentation/rust/quick-start.rst
+++ b/Documentation/rust/quick-start.rst
@@ -39,8 +39,17 @@ If ``rustup`` is being used, enter the checked out source code directory
        rustup override set $(scripts/min-tool-version.sh rustc)

 This will configure your working directory to use the correct version of
-``rustc`` without affecting your default toolchain. If you are not using
-``rustup``, fetch a standalone installer from:
+``rustc`` without affecting your default toolchain.
+
+If you are building the kernel with `O=`, i.e. specifying an output
+directory, then you should append `--path <builddir>` to the above
+command.
+
+Alternatively, you can use the ``rustupoverride`` Make target::
+
+       make LLVM=1 O=<builddir> rustupoverride
+
+If you are not using ``rustup``, fetch a standalone installer from:

        https://forge.rust-lang.org/infra/other-installation-methods.html#standalone
  

On 15/12/2023 06:48, Viresh Kumar wrote:
> On 14-12-23, 18:22, Miguel Ojeda wrote:
>> Something like v1 but a bit simpler, e.g. keeping things as they are,
>> but with just a sentence after the command like "If you are building
>> the kernel with `O=`, i.e. specifying an output directory, then you
>> should append `--path <builddir>`." could work.
>>
>> Or we could just provide a `rustupoverride` Make target to do this for
>> us [1], since we have all the information needed and would be
>> copy-pasteable by everybody. I can send it as a non-mangled patch and
>> then Viresh can redo this one on top using it.
> 
> How about this ?
> 
> diff --git a/Documentation/rust/quick-start.rst b/Documentation/rust/quick-start.rst
> index f382914f4191..367b06f3edc2 100644
> --- a/Documentation/rust/quick-start.rst
> +++ b/Documentation/rust/quick-start.rst
> @@ -39,8 +39,17 @@ If ``rustup`` is being used, enter the checked out source code directory
>          rustup override set $(scripts/min-tool-version.sh rustc)
> 
>   This will configure your working directory to use the correct version of
> -``rustc`` without affecting your default toolchain. If you are not using
> -``rustup``, fetch a standalone installer from:
> +``rustc`` without affecting your default toolchain.
> +
> +If you are building the kernel with `O=`, i.e. specifying an output
> +directory, then you should append `--path <builddir>` to the above
> +command.
> +

I think we can drop the reference to the `--path <buildir>` to avoid 
giving too much information to the users following the guide. It doesn't 
seem to bring anything given users should now always go through `make 
rustupoverride`.

> +Alternatively, you can use the ``rustupoverride`` Make target::
> +
> +       make LLVM=1 O=<builddir> rustupoverride
> +

But if I understood this correctly, the point here is that with the new 
target we can now abstract both cases behind the `make rustupoverride` 
target - i.e. we don't need to provide alternatives. So, maybe something 
like the following is clearer:

	If ``rustup`` is being used, enter the checked out source code 
directory, or your build directory (if you're using the `O=` option to 
build the kernel), and run::

         	make LLVM=1 rustupoverride

	This will configure your current directory to use the correct version 
of ``rustc`` without affecting your default toolchain.

	If you are not using ``rustup``, fetch a standalone installer from:
      	 
https://forge.rust-lang.org/infra/other-installation-methods.html#standalone

Tiago.
  

On 15-12-23, 11:14, Tiago Lam wrote:
> 	If ``rustup`` is being used, enter the checked out source code directory,
> or your build directory (if you're using the `O=` option to build the
> kernel), and run::

I thought people aren't required to enter the build directory now (but
just source code directory) and simply do:

         	make LLVM=1 O=<builddir> rustupoverride

> 
>         	make LLVM=1 rustupoverride

Will this still work if we are in the build directory ?

> 	This will configure your current directory to use the correct version of
> ``rustc`` without affecting your default toolchain.
> 
> 	If you are not using ``rustup``, fetch a standalone installer from:
>      	
> https://forge.rust-lang.org/infra/other-installation-methods.html#standalone
  

On Fri, Dec 15, 2023 at 12:14 PM Tiago Lam <tiagolam@gmail.com> wrote:
>
> I think we can drop the reference to the `--path <buildir>` to avoid
> giving too much information to the users following the guide. It doesn't
> seem to bring anything given users should now always go through `make
> rustupoverride`.

Yeah, the idea with the new target was to simplify this, rather than
have it as an additional way.

> But if I understood this correctly, the point here is that with the new
> target we can now abstract both cases behind the `make rustupoverride`
> target - i.e. we don't need to provide alternatives.

Exactly.

Cheers,
Miguel
  

On Fri, Dec 15, 2023 at 12:24 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> I thought people aren't required to enter the build directory now (but
> just source code directory) and simply do:
>
>                 make LLVM=1 O=<builddir> rustupoverride

Yeah, that is correct, but we don't need to write the `O=` in the
commands themselves. The idea is that 1) the commands can be easily
copy-pasted, 2) commands look simple (i.e. there are many other
variations and options you may pass), 3) newcomers do not need to care
about `O=` (so it is extra simple for them).

> Will this still work if we are in the build directory ?

Both should work (as long as the initial setup in the build folder is
done, of course), so I think we can simply remove the mention about
"enter ..." now and simply give the command.

In fact, even if Kbuild did not support that, we could still remove
the "enter ...", because then the `make` would need to be run like any
other target from the source tree. In other words, regardless of the
answer, we could remove it thanks to the new target, unless I am
missing something.

Cheers,
Miguel
  

On 15/12/2023 11:24, Viresh Kumar wrote:

[...]

> Will this still work if we are in the build directory ?

I've tried it and it does work. The build directory that's set up with 
`O=` ends up with a Makefile with an `include` to the original Makefile 
in my main linux source:
	include $MY_WORKSPACE/linux/Makefile

(But see Miguel's reply about dropping the mention to "enter ..." 
altogether)

Tiago.
  

On Fri, Dec 15, 2023 at 8:53 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Fri, Dec 15, 2023 at 12:24 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > I thought people aren't required to enter the build directory now (but
> > just source code directory) and simply do:
> >
> >                 make LLVM=1 O=<builddir> rustupoverride
>
> Yeah, that is correct, but we don't need to write the `O=` in the
> commands themselves. The idea is that 1) the commands can be easily
> copy-pasted, 2) commands look simple (i.e. there are many other
> variations and options you may pass), 3) newcomers do not need to care
> about `O=` (so it is extra simple for them).
>
> > Will this still work if we are in the build directory ?
>
> Both should work (as long as the initial setup in the build folder is
> done, of course), so I think we can simply remove the mention about
> "enter ..." now and simply give the command.
>
> In fact, even if Kbuild did not support that, we could still remove
> the "enter ...", because then the `make` would need to be run like any
> other target from the source tree.



FWIW.

Kbuild is designed to be able to initiate 'make' from anywhere,
even if the build directory is not set up.

In that case, you need to use -f option to point to the top Makefile.



You can enter a build directory, then do this:

  $ make -f <path/to/source/tree>/Makefile defconfig all




Likewise, both of the following should work.


1)  Enter the source directory, and

  $ make O=<path-to-build-directory> rustupoverride


2)  Enter the build directory, and


  $ make -f <path-to-source-directory>/Makefile rustupoverride






> In other words, regardless of the
> answer, we could remove it thanks to the new target, unless I am
> missing something.
>
> Cheers,
> Miguel
>


--
Best Regards
Masahiro Yamada
  

On Mon, Dec 18, 2023 at 1:07 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> FWIW.
>
> Kbuild is designed to be able to initiate 'make' from anywhere,
> even if the build directory is not set up.
>
> In that case, you need to use -f option to point to the top Makefile.

I meant for the command that Viresh mentioned (i.e. without `-f`), but
that `-f` is meant to work is good to know, thanks!

Cheers,
Miguel
  

On Tue, Dec 12, 2023 at 8:44 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Rustup override is required to be set for the build directory and not
> necessarily the kernel source tree (unless the build directory is its
> subdir).
>
> Clarify the same in quick-start guide.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Since we are not going to use v3 given we will not have the
`rustupoverride` Make target, I have applied this one. It is also the
one that got more `Reviewed-by`s, I think Andreas preferred too and
Masahiro was kind enough to be OK applying this one instead of his
(which would need to be rebased and submitted to the list), so I went
with that one. Tiago's concern is still there though (i.e. the script
is relative to the source tree), but we can improve things further
later (perhaps if we add a script for this sort of thing).

Applied to `rust-next` (reworded and fixed quotes for `--path` and
`set`) -- thanks everyone!

Cheers,
Miguel