[PATCH] kbuild: rust: add `rustupoverride` target

[PATCH] kbuild: rust: add `rustupoverride` target

[Miguel Ojeda]

When setting up the Rust support via `rustup`, one may use an override
in order to select the right version of the Rust toolchain.

The current instructions at Documentation/rust/quick-start.rst assume
one is using an in-tree kernel build (i.e. no `O=`) [1]. We would like
to provide also the way to do so for `O=` builds, but ideally in a way
that keeps the one-liner copy-pastable and without duplication [2].

Thus provide a new Make target, `rustupoverride`, that sets it up for
the user given their build options/variables.

Link: https://lore.kernel.org/rust-for-linux/20231207084846.faset66xzuoyvdlg@vireshk-i7/ [1]
Link: https://lore.kernel.org/rust-for-linux/CANiq72=mvca8PXoxwzSao+QFbAHDCecSKCDtV+ffd+YgZNFaww@mail.gmail.com/ [2]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
Viresh may send a patch on top of this to refer to `rustupoverride`
from the Quick Start guide in `Documentation/rust` -- that one should
probably be applied right after this one.

 Makefile | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

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
 # ---------------------------------------------------------------------------


base-commit: a39b6ac3781d46ba18193c9dbb2110f31e9bffe9
--
2.43.0

Re: [PATCH] kbuild: rust: add `rustupoverride` target

[Martin Rodriguez Reboredo]

On 12/14/23 19:22, Miguel Ojeda wrote:
> When setting up the Rust support via `rustup`, one may use an override
> in order to select the right version of the Rust toolchain.
> 
> The current instructions at Documentation/rust/quick-start.rst assume
> one is using an in-tree kernel build (i.e. no `O=`) [1]. We would like
> to provide also the way to do so for `O=` builds, but ideally in a way
> that keeps the one-liner copy-pastable and without duplication [2].
> 
> Thus provide a new Make target, `rustupoverride`, that sets it up for
> the user given their build options/variables.
> 
> Link: https://lore.kernel.org/rust-for-linux/20231207084846.faset66xzuoyvdlg@vireshk-i7/ [1]
> Link: https://lore.kernel.org/rust-for-linux/CANiq72=mvca8PXoxwzSao+QFbAHDCecSKCDtV+ffd+YgZNFaww@mail.gmail.com/ [2]
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH] kbuild: rust: add `rustupoverride` target

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]

On Fri, 15 Dec 2023 at 06:23, Miguel Ojeda <ojeda@kernel.org> wrote:
>
> When setting up the Rust support via `rustup`, one may use an override
> in order to select the right version of the Rust toolchain.
>
> The current instructions at Documentation/rust/quick-start.rst assume
> one is using an in-tree kernel build (i.e. no `O=`) [1]. We would like
> to provide also the way to do so for `O=` builds, but ideally in a way
> that keeps the one-liner copy-pastable and without duplication [2].
>
> Thus provide a new Make target, `rustupoverride`, that sets it up for
> the user given their build options/variables.
>
> Link: https://lore.kernel.org/rust-for-linux/20231207084846.faset66xzuoyvdlg@vireshk-i7/ [1]
> Link: https://lore.kernel.org/rust-for-linux/CANiq72=mvca8PXoxwzSao+QFbAHDCecSKCDtV+ffd+YgZNFaww@mail.gmail.com/ [2]
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
> Viresh may send a patch on top of this to refer to `rustupoverride`
> from the Quick Start guide in `Documentation/rust` -- that one should
> probably be applied right after this one.
>

This worked fine for me testing the 1.74.1 upgrade, thanks.

Tested-by: David Gow <davidgow@google.com>

Would having similar targets for bindgen help? What about having this
install rust-src? Updating / installing those required a lot more
looking up of documentation (and then adding --force), so it'd be nice
if there were some way to do a similar override or make target.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH] kbuild: rust: add `rustupoverride` target

[Miguel Ojeda]

On Fri, Dec 15, 2023 at 8:38 AM David Gow <davidgow@google.com> wrote:
>
> Would having similar targets for bindgen help? What about having this
> install rust-src? Updating / installing those required a lot more
> looking up of documentation (and then adding --force), so it'd be nice
> if there were some way to do a similar override or make target.

Which docs did you need to check? i.e. we have the commands for those
steps in the Quick Start guide. I think you may be referring to the
case when switching between LTS and mainline, due to the `bindgen-cli`
vs. `bindgen` name change that the tool did (since that is where
`--force` is required, not for normal upgrading or downgrading). That
is definitely a bit painful :-( At least `cargo` mentions the need for
`--force` in that case. Or are you referring to something else?

I considered having a `rustupsetup` target (or script) instead that
does everything (with a `BUILDONLY=1` option or similar, given some
dependencies are not strictly needed for building), since having all
this "switching logic" is useful, but then:

  - I am not sure we should "hide" the details of the setup too much:
I thought `rustupoverride` would be OK-ish because the output
directory is needed (so it is justified) and the command is
straightforward, but the others do not "need" that information.

  - If we include `bindgen` there, it wouldn't be `rustup`-only
anymore, so perhaps we would need another name like `rustsetup`. But
that may mislead others (e.g. those looking at the Make help), because
it is just one way of setting things up and it is not required.
Perhaps this can be alleviated by not including it in the `make help`,
so that everybody comes from the Quick Start guide and thus hopefully
they have read the document at least diagonally :)

  - Given there are different ways to set different sub-steps, and the
fact that we don't have such a script for C does not have (please
correct me if I am wrong -- I am aware of Thorsten's recent guide,
which covers `apt` etc., but that is a Quick Start-like approach
rather than automated script), I am not sure it would be welcome as a
Make target (but perhaps it would be fine as a script?). Masahiro may
have some guidelines here.

  - In the future we may have to change how the setup works or add
steps, i.e. it is not 100% settled. Thus I am concerned about adding
complex Make targets that users may start to depend on (i.e. with
particular/complex semantics), vs. just having docs that are manual.
For `rustupoverride`, it thought it could be OK-ish because it is just
a single command and unlikely that it will change (and if we stop
using it, we can make it empty with a warning and then remove it
eventually; while it gets harder for more complex ones).

What do you think?

Cheers,
Miguel

Re: [PATCH] kbuild: rust: add `rustupoverride` target

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 4692 bytes --]

On Fri, 15 Dec 2023 at 19:27, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Fri, Dec 15, 2023 at 8:38 AM David Gow <davidgow@google.com> wrote:
> >
> > Would having similar targets for bindgen help? What about having this
> > install rust-src? Updating / installing those required a lot more
> > looking up of documentation (and then adding --force), so it'd be nice
> > if there were some way to do a similar override or make target.
>
> Which docs did you need to check? i.e. we have the commands for those
> steps in the Quick Start guide. I think you may be referring to the
> case when switching between LTS and mainline, due to the `bindgen-cli`
> vs. `bindgen` name change that the tool did (since that is where
> `--force` is required, not for normal upgrading or downgrading). That
> is definitely a bit painful :-( At least `cargo` mentions the need for
> `--force` in that case. Or are you referring to something else?

Yeah, I only needed to check the Quick Start guide, but had to look at
all three sections (rustc, rust-src, bindgen), which

And yes, it looks like the bindgen/bindgen-cli name change was what
was causing my issues. If that's only a one-off, then we should be
fine. (

> I considered having a `rustupsetup` target (or script) instead that
> does everything (with a `BUILDONLY=1` option or similar, given some
> dependencies are not strictly needed for building), since having all
> this "switching logic" is useful, but then:
>
>   - I am not sure we should "hide" the details of the setup too much:
> I thought `rustupoverride` would be OK-ish because the output
> directory is needed (so it is justified) and the command is
> straightforward, but the others do not "need" that information.

Yeah; I'm a bit conflicted here as well. A part of me thinks that, if
we're adding make targets, doing them for everything would be more
consistent, but it does add some 'unnecessary' targets, which is
probably not worth it just to look nice.

>   - If we include `bindgen` there, it wouldn't be `rustup`-only
> anymore, so perhaps we would need another name like `rustsetup`. But
> that may mislead others (e.g. those looking at the Make help), because
> it is just one way of setting things up and it is not required.
> Perhaps this can be alleviated by not including it in the `make help`,
> so that everybody comes from the Quick Start guide and thus hopefully
> they have read the document at least diagonally :)

Personally, I'd love a 'make rustsetup' or similar, but I do agree it
could be misleading, particularly long-term.

The real advantage to it is while we're depending on unstable things
and changing versions regularly. I don't think the one-off setup is
tricky (and the documentation is good), it's the need to upgrade
regularly (and, for per-directory overrides, possibly on several
different checkouts). Having a script to 'upgrade everything to the
required version' would be really convenient.

>   - Given there are different ways to set different sub-steps, and the
> fact that we don't have such a script for C does not have (please
> correct me if I am wrong -- I am aware of Thorsten's recent guide,
> which covers `apt` etc., but that is a Quick Start-like approach
> rather than automated script), I am not sure it would be welcome as a
> Make target (but perhaps it would be fine as a script?). Masahiro may
> have some guidelines here.
>
>   - In the future we may have to change how the setup works or add
> steps, i.e. it is not 100% settled. Thus I am concerned about adding
> complex Make targets that users may start to depend on (i.e. with
> particular/complex semantics), vs. just having docs that are manual.
> For `rustupoverride`, it thought it could be OK-ish because it is just
> a single command and unlikely that it will change (and if we stop
> using it, we can make it empty with a warning and then remove it
> eventually; while it gets harder for more complex ones).
>
> What do you think?

Yeah, on reflection it's definitely not a good long-term solution, as
ideally people will be able to just use whatever rustc version they
have lying around (be it via rustup, or distro packages).

Maybe what we need in the short term is an 'update-rust' script
(possibly living in scripts/, possibly hosted elsewhere, like the
rust-for-linux website), which just does all the steps from the Quick
Start guide automatically? (Even if that's not a good solution
overall, I'll probably throw one together myself just to keep my own
systems up-to-date.)

Still -- none of this is a blocker for this patch.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH] kbuild: rust: add `rustupoverride` target

[Masahiro Yamada]

On Fri, Dec 15, 2023 at 8:27 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Fri, Dec 15, 2023 at 8:38 AM David Gow <davidgow@google.com> wrote:
> >
> > Would having similar targets for bindgen help? What about having this
> > install rust-src? Updating / installing those required a lot more
> > looking up of documentation (and then adding --force), so it'd be nice
> > if there were some way to do a similar override or make target.
>
> Which docs did you need to check? i.e. we have the commands for those
> steps in the Quick Start guide. I think you may be referring to the
> case when switching between LTS and mainline, due to the `bindgen-cli`
> vs. `bindgen` name change that the tool did (since that is where
> `--force` is required, not for normal upgrading or downgrading). That
> is definitely a bit painful :-( At least `cargo` mentions the need for
> `--force` in that case. Or are you referring to something else?
>
> I considered having a `rustupsetup` target (or script) instead that
> does everything (with a `BUILDONLY=1` option or similar, given some
> dependencies are not strictly needed for building), since having all
> this "switching logic" is useful, but then:
>
>   - I am not sure we should "hide" the details of the setup too much:
> I thought `rustupoverride` would be OK-ish because the output
> directory is needed (so it is justified) and the command is
> straightforward, but the others do not "need" that information.
>
>   - If we include `bindgen` there, it wouldn't be `rustup`-only
> anymore, so perhaps we would need another name like `rustsetup`. But
> that may mislead others (e.g. those looking at the Make help), because
> it is just one way of setting things up and it is not required.
> Perhaps this can be alleviated by not including it in the `make help`,
> so that everybody comes from the Quick Start guide and thus hopefully
> they have read the document at least diagonally :)
>
>   - Given there are different ways to set different sub-steps, and the
> fact that we don't have such a script for C does not have (please
> correct me if I am wrong -- I am aware of Thorsten's recent guide,
> which covers `apt` etc., but that is a Quick Start-like approach
> rather than automated script), I am not sure it would be welcome as a
> Make target (but perhaps it would be fine as a script?). Masahiro may
> have some guidelines here.



'make rustupoverride' potentially requires internet connection
if the required rustc version is not yet installed on the system.

Even if it is already installed, it changes ~/.rustup/settings.toml.


If I do rustupoverride per-directory,

$ make O=~/kernel/build0 rustupoverride
$ make O=~/kernel/build1 rustupoverride
$ make O=~/kernel/build2 rustupoverride

it would accumulate the overrides entries in ~/.rustup/settings.toml


Rather, I will manually do this one time for the parent directory:

$ rustup override set --path=/~kernel $(scripts/min-tool-version.sh rustc)


and use ~/kernel/build0, ~/kernel/build1, ~/kernel/build2
as output directories.



In principle, Kbuild does not require internet connection,
or proactively change the system setting.

If you want to provide a way for automated settings,
you can do it in a script you maintain.








>   - In the future we may have to change how the setup works or add
> steps, i.e. it is not 100% settled. Thus I am concerned about adding
> complex Make targets that users may start to depend on (i.e. with
> particular/complex semantics), vs. just having docs that are manual.
> For `rustupoverride`, it thought it could be OK-ish because it is just
> a single command and unlikely that it will change (and if we stop
> using it, we can make it empty with a warning and then remove it
> eventually; while it gets harder for more complex ones).
>
> What do you think?
>
> Cheers,
> Miguel









--
Best Regards
Masahiro Yamada

Re: [PATCH] kbuild: rust: add `rustupoverride` target

[Miguel Ojeda]

On Mon, Dec 18, 2023 at 1:10 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> In principle, Kbuild does not require internet connection,
> or proactively change the system setting.

Yeah, that was what I thought. I agree it can be surprising to have
Make targets that modify environment/system-level bits (i.e. affecting
things outside the build).

> Rather, I will manually do this one time for the parent directory:

That can work for many people, yeah. Though I imagine some people may
want to keep builds (and sources) of different kernel versions in the
same parent folder (or even other projects). But one can use nested
overrides too.

> If you want to provide a way for automated settings,
> you can do it in a script you maintain.

Sounds good. In that case, we can send to the list your patch from the
`rust` branch if that is OK with you (i.e. I understand you would
prefer to avoid not just `rustsetup` but also `rustupoverride`).

Thanks Masahiro!

Cheers,
Miguel

Re: [PATCH] kbuild: rust: add `rustupoverride` target

[Masahiro Yamada]

On Mon, Dec 18, 2023 at 10:33 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Mon, Dec 18, 2023 at 1:10 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > In principle, Kbuild does not require internet connection,
> > or proactively change the system setting.
>
> Yeah, that was what I thought. I agree it can be surprising to have
> Make targets that modify environment/system-level bits (i.e. affecting
> things outside the build).
>
> > Rather, I will manually do this one time for the parent directory:
>
> That can work for many people, yeah. Though I imagine some people may
> want to keep builds (and sources) of different kernel versions in the
> same parent folder (or even other projects). But one can use nested
> overrides too.
>
> > If you want to provide a way for automated settings,
> > you can do it in a script you maintain.
>
> Sounds good. In that case, we can send to the list your patch from the
> `rust` branch if that is OK with you (i.e. I understand you would
> prefer to avoid not just `rustsetup` but also `rustupoverride`).
>
> Thanks Masahiro!
>
> Cheers,
> Miguel


Viresh's v2 was written without relying on this patch.

If that one is better described, that is OK too.





--
Best Regards
Masahiro Yamada

Re: [PATCH] kbuild: rust: add `rustupoverride` target

[Alice Ryhl]

On Thu, Dec 14, 2023 at 11:23 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> When setting up the Rust support via `rustup`, one may use an override
> in order to select the right version of the Rust toolchain.
>
> The current instructions at Documentation/rust/quick-start.rst assume
> one is using an in-tree kernel build (i.e. no `O=`) [1]. We would like
> to provide also the way to do so for `O=` builds, but ideally in a way
> that keeps the one-liner copy-pastable and without duplication [2].
>
> Thus provide a new Make target, `rustupoverride`, that sets it up for
> the user given their build options/variables.
>
> Link: https://lore.kernel.org/rust-for-linux/20231207084846.faset66xzuoyvdlg@vireshk-i7/ [1]
> Link: https://lore.kernel.org/rust-for-linux/CANiq72=mvca8PXoxwzSao+QFbAHDCecSKCDtV+ffd+YgZNFaww@mail.gmail.com/ [2]
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

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

Re: [PATCH] kbuild: rust: add `rustupoverride` target

[Tiago Lam]

On 14/12/2023 22:22, Miguel Ojeda wrote:
> When setting up the Rust support via `rustup`, one may use an override
> in order to select the right version of the Rust toolchain.
> 
> The current instructions at Documentation/rust/quick-start.rst assume
> one is using an in-tree kernel build (i.e. no `O=`) [1]. We would like
> to provide also the way to do so for `O=` builds, but ideally in a way
> that keeps the one-liner copy-pastable and without duplication [2].
> 
> Thus provide a new Make target, `rustupoverride`, that sets it up for
> the user given their build options/variables.
> 
> Link: https://lore.kernel.org/rust-for-linux/20231207084846.faset66xzuoyvdlg@vireshk-i7/ [1]
> Link: https://lore.kernel.org/rust-for-linux/CANiq72=mvca8PXoxwzSao+QFbAHDCecSKCDtV+ffd+YgZNFaww@mail.gmail.com/ [2]
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Reviewed-by: Tiago Lam <tiagolam@gmail.com>