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

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

[Viresh Kumar]

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>
---
 Documentation/rust/quick-start.rst | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/rust/quick-start.rst b/Documentation/rust/quick-start.rst
index f382914f4191..a7a08955fe46 100644
--- a/Documentation/rust/quick-start.rst
+++ b/Documentation/rust/quick-start.rst
@@ -39,8 +39,13 @@ 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.
+
+Note that the override applies to the build directory (and its sub-directories).
+If the kernel is built with `O=<build directory>`, the override must be set for
+the build directory instead.
+
+If you are not using ``rustup``, fetch a standalone installer from:
 
 	https://forge.rust-lang.org/infra/other-installation-methods.html#standalone
 
-- 
2.31.1.272.g89b43f80a514

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

[Alice Ryhl]

On Fri, Dec 8, 2023 at 11:18 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>

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

[Miguel Ojeda]

On Fri, Dec 8, 2023 at 11:18 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> @@ -39,8 +39,13 @@ 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.
> +
> +Note that the override applies to the build directory (and its sub-directories).
> +If the kernel is built with `O=<build directory>`, the override must be set for
> +the build directory instead.
> +
> +If you are not using ``rustup``, fetch a standalone installer from:

Thanks Viresh!

I think it may be a bit more clear/compact if we simply change the
"enter the checked out source code directory" sentence instead above?
What do you think?

Cheers,
Miguel

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

[Benno Lossin]

On 12/8/23 11:18, 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>
> ---
>  Documentation/rust/quick-start.rst | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/rust/quick-start.rst b/Documentation/rust/quick-start.rst
> index f382914f4191..a7a08955fe46 100644
> --- a/Documentation/rust/quick-start.rst
> +++ b/Documentation/rust/quick-start.rst
> @@ -39,8 +39,13 @@ 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.
> +
> +Note that the override applies to the build directory (and its sub-directories).

Shouldn't this be "Note that the override only applies to the current
working directory (and its sub-directories)."?
I think it would also be useful to continue with this: "But in order
to build the kernel, this override must affect the build directory.".

And then you could also mention that in the default location for the
build directory is in the repository.

-- 
Cheers,
Benno

> +If the kernel is built with `O=<build directory>`, the override must be set for
> +the build directory instead.
> +
> +If you are not using ``rustup``, fetch a standalone installer from:
> 
>  	https://forge.rust-lang.org/infra/other-installation-methods.html#standalone

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

[Viresh Kumar]

On 08-12-23, 18:04, Benno Lossin wrote:
> Shouldn't this be "Note that the override only applies to the current
> working directory (and its sub-directories)."?
> I think it would also be useful to continue with this: "But in order
> to build the kernel, this override must affect the build directory.".
> 
> And then you could also mention that in the default location for the
> build directory is in the repository.

Based on feedback from Miguel and Benno, how about this instead ?

diff --git a/Documentation/rust/quick-start.rst b/Documentation/rust/quick-start.rst
index f382914f4191..dee787f92d26 100644
--- a/Documentation/rust/quick-start.rst
+++ b/Documentation/rust/quick-start.rst
@@ -33,14 +33,17 @@ 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 and run::

        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.
+
+Note that the override applies to the current working directory (and its
+sub-directories).
+
+If you are not using ``rustup``, fetch a standalone installer from:

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

-- 
viresh

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

[Andreas Hindborg (Samsung)]

Thanks for fixing this Viresh!

Viresh Kumar <viresh.kumar@linaro.org> writes:

> diff --git a/Documentation/rust/quick-start.rst b/Documentation/rust/quick-start.rst
> index f382914f4191..dee787f92d26 100644
> --- a/Documentation/rust/quick-start.rst
> +++ b/Documentation/rust/quick-start.rst
> @@ -33,14 +33,17 @@ 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 and run::
>
>         rustup override set $(scripts/min-tool-version.sh rustc)

How about just specifying the path here:

         rustup override set --path=<build-dir> $(scripts/min-tool-version.sh rustc)

Best regards,
Andreas

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

[Viresh Kumar]

On 11-12-23, 09:09, Andreas Hindborg (Samsung) wrote:
> 
> Thanks for fixing this Viresh!
> 
> Viresh Kumar <viresh.kumar@linaro.org> writes:
> 
> > diff --git a/Documentation/rust/quick-start.rst b/Documentation/rust/quick-start.rst
> > index f382914f4191..dee787f92d26 100644
> > --- a/Documentation/rust/quick-start.rst
> > +++ b/Documentation/rust/quick-start.rst
> > @@ -33,14 +33,17 @@ 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 and run::
> >
> >         rustup override set $(scripts/min-tool-version.sh rustc)
> 
> How about just specifying the path here:
> 
>          rustup override set --path=<build-dir> $(scripts/min-tool-version.sh rustc)

Hmm, this sounds good too. In that case the above line can be changed to:

"If ``rustup`` is being used, run::"

Looks okay ?

-- 
viresh

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

[Benno Lossin]

On 12/11/23 09:23, Viresh Kumar wrote:
> On 11-12-23, 09:09, Andreas Hindborg (Samsung) wrote:
>>
>> Thanks for fixing this Viresh!
>>
>> Viresh Kumar <viresh.kumar@linaro.org> writes:
>>
>>> diff --git a/Documentation/rust/quick-start.rst b/Documentation/rust/quick-start.rst
>>> index f382914f4191..dee787f92d26 100644
>>> --- a/Documentation/rust/quick-start.rst
>>> +++ b/Documentation/rust/quick-start.rst
>>> @@ -33,14 +33,17 @@ 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 and run::
>>>
>>>         rustup override set $(scripts/min-tool-version.sh rustc)
>>
>> How about just specifying the path here:
>>
>>          rustup override set --path=<build-dir> $(scripts/min-tool-version.sh rustc)
> 
> Hmm, this sounds good too. In that case the above line can be changed to:
> 
> "If ``rustup`` is being used, run::"
> 
> Looks okay ?

I think it should also mention that you do not need the
`--path=<build-dir>`. It might be confusing for new users who do not
know where the build directory is.
It might also work to put both commands there (i.e. with
`--path=<build-dir>` and without) and explaining what each of them do.

-- 
Cheers,
Benno

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

[John Hubbard]

On 12/10/23 22:47, Viresh Kumar wrote:
> On 08-12-23, 18:04, Benno Lossin wrote:
>> Shouldn't this be "Note that the override only applies to the current
>> working directory (and its sub-directories)."?
>> I think it would also be useful to continue with this: "But in order
>> to build the kernel, this override must affect the build directory.".
>>
>> And then you could also mention that in the default location for the
>> build directory is in the repository.
> 
> Based on feedback from Miguel and Benno, how about this instead ?
> 
> diff --git a/Documentation/rust/quick-start.rst b/Documentation/rust/quick-start.rst
> index f382914f4191..dee787f92d26 100644
> --- a/Documentation/rust/quick-start.rst
> +++ b/Documentation/rust/quick-start.rst
> @@ -33,14 +33,17 @@ 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 and run::

That "enter the kernel build directory" phrase is much better than the
"enter the checked out source code directory".

I feel confident saying this, because I just read this document over the
weekend, while getting set up to build rust for Linux. And this phrase
was a little jarring and weird to me.

This is after all a minor point, but it's nice to polish up this getting
started guide. It's already a concise and excellent guide, by the way.


thanks,
-- 
John Hubbard
NVIDIA

> 
>          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.
> +
> +Note that the override applies to the current working directory (and its
> +sub-directories).
> +
> +If you are not using ``rustup``, fetch a standalone installer from:
> 
>          https://forge.rust-lang.org/infra/other-installation-methods.html#standalone
>