Date: Fri, 15 Dec 2023 12:27:19 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: David Gow <davidgow@...gle.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Masahiro Yamada <masahiroy@...nel.org>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, Alex Gaynor <alex.gaynor@...il.com>, 
	Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>, 
	Nicolas Schier <nicolas@...sle.eu>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...sung.com>, 
	Alice Ryhl <aliceryhl@...gle.com>, linux-kbuild@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, 
	patches@...ts.linux.dev
Subject: Re: [PATCH] kbuild: rust: add `rustupoverride` target

On Fri, Dec 15, 2023 at 8:38 AM David Gow <davidgow@...gle.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

Powered by blists - more mailing lists