[PATCH 1/3] kbuild: rust: move `-Dwarnings` handling to `Makefile.extrawarn`

[PATCH 1/3] kbuild: rust: move `-Dwarnings` handling to `Makefile.extrawarn`

[Miguel Ojeda]

Following commit e88ca24319e4 ("kbuild: consolidate warning flags
in scripts/Makefile.extrawarn"), move `-Dwarnings` handling into
`Makefile.extrawarn` like C's `-Werror`.

No functional change intended.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 Makefile                   | 3 ---
 scripts/Makefile.extrawarn | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 763b6792d3d5..fba567a55607 100644
--- a/Makefile
+++ b/Makefile
@@ -842,9 +842,6 @@ stackp-flags-$(CONFIG_STACKPROTECTOR_STRONG)      := -fstack-protector-strong
 
 KBUILD_CFLAGS += $(stackp-flags-y)
 
-KBUILD_RUSTFLAGS-$(CONFIG_WERROR) += -Dwarnings
-KBUILD_RUSTFLAGS += $(KBUILD_RUSTFLAGS-y)
-
 ifdef CONFIG_FRAME_POINTER
 KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
 KBUILD_RUSTFLAGS += -Cforce-frame-pointers=y
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 3ce5d503a6da..48114e91c386 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -26,6 +26,9 @@ endif
 
 KBUILD_CPPFLAGS-$(CONFIG_WERROR) += -Werror
 KBUILD_CPPFLAGS += $(KBUILD_CPPFLAGS-y)
+KBUILD_RUSTFLAGS-$(CONFIG_WERROR) += -Dwarnings
+KBUILD_RUSTFLAGS += $(KBUILD_RUSTFLAGS-y)
+
 KBUILD_CFLAGS-$(CONFIG_CC_NO_ARRAY_BOUNDS) += -Wno-array-bounds
 
 ifdef CONFIG_CC_IS_CLANG
-- 
2.45.1

[PATCH 2/3] kbuild: rust: apply `CONFIG_WERROR` to all Rust targets

[Miguel Ojeda]

Make all Rust code (i.e. not just kernel code) respect `CONFIG_WERROR`,
so that we keep everything warning clean.

In particular, this affects targets in `rust/` (`RUSTDOC H`, `RUSTC TL`,
`RUSTDOC T`, `RUSTC T` and `RUSTC P`), plus host programs and any others
we may add later.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
This one requires the `rusttest` warning cleanup at
https://lore.kernel.org/rust-for-linux/20240519210735.587323-1-ojeda@kernel.org/

 Makefile                   | 4 ++--
 scripts/Makefile.extrawarn | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index fba567a55607..2d0ea441cb9c 100644
--- a/Makefile
+++ b/Makefile
@@ -469,7 +469,7 @@ export rust_common_flags := --edition=2021 \

 KBUILD_HOSTCFLAGS   := $(KBUILD_USERHOSTCFLAGS) $(HOST_LFS_CFLAGS) $(HOSTCFLAGS)
 KBUILD_HOSTCXXFLAGS := -Wall -O2 $(HOST_LFS_CFLAGS) $(HOSTCXXFLAGS)
-KBUILD_HOSTRUSTFLAGS := $(rust_common_flags) -O -Cstrip=debuginfo \
+KBUILD_HOSTRUSTFLAGS = $(rust_common_flags) -O -Cstrip=debuginfo \
 			-Zallow-features= $(HOSTRUSTFLAGS)
 KBUILD_HOSTLDFLAGS  := $(HOST_LFS_LDFLAGS) $(HOSTLDFLAGS)
 KBUILD_HOSTLDLIBS   := $(HOST_LFS_LIBS) $(HOSTLDLIBS)
@@ -560,7 +560,7 @@ KBUILD_CFLAGS += -fno-PIE
 KBUILD_CFLAGS += -fno-strict-aliasing

 KBUILD_CPPFLAGS := -D__KERNEL__
-KBUILD_RUSTFLAGS := $(rust_common_flags) \
+KBUILD_RUSTFLAGS = $(rust_common_flags) \
 		    -Cpanic=abort -Cembed-bitcode=n -Clto=n \
 		    -Cforce-unwind-tables=n -Ccodegen-units=1 \
 		    -Csymbol-mangling-version=v0 \
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 48114e91c386..990890821889 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -26,8 +26,8 @@ endif

 KBUILD_CPPFLAGS-$(CONFIG_WERROR) += -Werror
 KBUILD_CPPFLAGS += $(KBUILD_CPPFLAGS-y)
-KBUILD_RUSTFLAGS-$(CONFIG_WERROR) += -Dwarnings
-KBUILD_RUSTFLAGS += $(KBUILD_RUSTFLAGS-y)
+rust_common_flags-$(CONFIG_WERROR) += -Dwarnings
+rust_common_flags += $(rust_common_flags-y)

 KBUILD_CFLAGS-$(CONFIG_CC_NO_ARRAY_BOUNDS) += -Wno-array-bounds

--
2.45.1

[PATCH 3/3] kbuild: rust: support `W=e` for Rust

[Miguel Ojeda]

With `W=e`, kernel C targets error out on warnings.

Add support for the same feature for Rust code, but take the opportunity
to apply it for every Rust target (i.e. not just kernel code), so that
it behaves like having set `CONFIG_WERROR`.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 scripts/Makefile.extrawarn | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 990890821889..214b5edce4f2 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -206,5 +206,6 @@ endif
 ifneq ($(findstring e, $(KBUILD_EXTRA_WARN)),)
 
 KBUILD_CFLAGS += -Werror
+rust_common_flags += -Dwarnings
 
 endif
-- 
2.45.1

Re: [PATCH 1/3] kbuild: rust: move `-Dwarnings` handling to `Makefile.extrawarn`

[Alice Ryhl]

On Sun, May 19, 2024 at 11:12 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> Following commit e88ca24319e4 ("kbuild: consolidate warning flags
> in scripts/Makefile.extrawarn"), move `-Dwarnings` handling into
> `Makefile.extrawarn` like C's `-Werror`.
>
> No functional change intended.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

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

Re: [PATCH 2/3] kbuild: rust: apply `CONFIG_WERROR` to all Rust targets

[Alice Ryhl]

On Sun, May 19, 2024 at 11:12 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> Make all Rust code (i.e. not just kernel code) respect `CONFIG_WERROR`,
> so that we keep everything warning clean.
>
> In particular, this affects targets in `rust/` (`RUSTDOC H`, `RUSTC TL`,
> `RUSTDOC T`, `RUSTC T` and `RUSTC P`), plus host programs and any others
> we may add later.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

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

Re: [PATCH 3/3] kbuild: rust: support `W=e` for Rust

[Alice Ryhl]

On Sun, May 19, 2024 at 11:13 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> With `W=e`, kernel C targets error out on warnings.
>
> Add support for the same feature for Rust code, but take the opportunity
> to apply it for every Rust target (i.e. not just kernel code), so that
> it behaves like having set `CONFIG_WERROR`.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

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

Re: [PATCH 2/3] kbuild: rust: apply `CONFIG_WERROR` to all Rust targets

[Masahiro Yamada]

On Mon, May 20, 2024 at 6:12 AM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> Make all Rust code (i.e. not just kernel code) respect `CONFIG_WERROR`,
> so that we keep everything warning clean.


Rust started to do something different from C.


KBUILD_HOSTCFLAGS is not affected by any CONFIG option.

The reason is because HOSTCC is needed for building Kconfig.
If the flags for HOSTCC is changed by a CONFIG option,
it would be a chicken-egg problem.
Also, some host programs might be compiled even
without .config at all. (e.g. scripts/unifdef)


I know Rust will not become a part of the core infrastructure
of the build system, but IMHO, host programs should not be
affected by any CONFIG option.

I do not like this patch.






>
> In particular, this affects targets in `rust/` (`RUSTDOC H`, `RUSTC TL`,
> `RUSTDOC T`, `RUSTC T` and `RUSTC P`), plus host programs and any others
> we may add later.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
> This one requires the `rusttest` warning cleanup at
> https://lore.kernel.org/rust-for-linux/20240519210735.587323-1-ojeda@kernel.org/
>
>  Makefile                   | 4 ++--
>  scripts/Makefile.extrawarn | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index fba567a55607..2d0ea441cb9c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -469,7 +469,7 @@ export rust_common_flags := --edition=2021 \
>
>  KBUILD_HOSTCFLAGS   := $(KBUILD_USERHOSTCFLAGS) $(HOST_LFS_CFLAGS) $(HOSTCFLAGS)
>  KBUILD_HOSTCXXFLAGS := -Wall -O2 $(HOST_LFS_CFLAGS) $(HOSTCXXFLAGS)
> -KBUILD_HOSTRUSTFLAGS := $(rust_common_flags) -O -Cstrip=debuginfo \
> +KBUILD_HOSTRUSTFLAGS = $(rust_common_flags) -O -Cstrip=debuginfo \
>                         -Zallow-features= $(HOSTRUSTFLAGS)
>  KBUILD_HOSTLDFLAGS  := $(HOST_LFS_LDFLAGS) $(HOSTLDFLAGS)
>  KBUILD_HOSTLDLIBS   := $(HOST_LFS_LIBS) $(HOSTLDLIBS)
> @@ -560,7 +560,7 @@ KBUILD_CFLAGS += -fno-PIE
>  KBUILD_CFLAGS += -fno-strict-aliasing
>
>  KBUILD_CPPFLAGS := -D__KERNEL__
> -KBUILD_RUSTFLAGS := $(rust_common_flags) \
> +KBUILD_RUSTFLAGS = $(rust_common_flags) \
>                     -Cpanic=abort -Cembed-bitcode=n -Clto=n \
>                     -Cforce-unwind-tables=n -Ccodegen-units=1 \
>                     -Csymbol-mangling-version=v0 \
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index 48114e91c386..990890821889 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -26,8 +26,8 @@ endif
>
>  KBUILD_CPPFLAGS-$(CONFIG_WERROR) += -Werror
>  KBUILD_CPPFLAGS += $(KBUILD_CPPFLAGS-y)
> -KBUILD_RUSTFLAGS-$(CONFIG_WERROR) += -Dwarnings
> -KBUILD_RUSTFLAGS += $(KBUILD_RUSTFLAGS-y)
> +rust_common_flags-$(CONFIG_WERROR) += -Dwarnings
> +rust_common_flags += $(rust_common_flags-y)
>
>  KBUILD_CFLAGS-$(CONFIG_CC_NO_ARRAY_BOUNDS) += -Wno-array-bounds
>
> --
> 2.45.1



--
Best Regards

Masahiro Yamada

Re: [PATCH 2/3] kbuild: rust: apply `CONFIG_WERROR` to all Rust targets

[Miguel Ojeda]

On Tue, May 21, 2024 at 6:15 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Rust started to do something different from C.
>
> KBUILD_HOSTCFLAGS is not affected by any CONFIG option.
>
> The reason is because HOSTCC is needed for building Kconfig.
> If the flags for HOSTCC is changed by a CONFIG option,
> it would be a chicken-egg problem.
> Also, some host programs might be compiled even
> without .config at all. (e.g. scripts/unifdef)
>
> I know Rust will not become a part of the core infrastructure
> of the build system, but IMHO, host programs should not be
> affected by any CONFIG option.
>
> I do not like this patch.

Thanks Masahiro -- yeah, I can see how it makes sense for C host
programs, and consistency with those may be best, even if it is not
used for core infrastructure in the case of Rust.

Do you think it would be OK if we do it only for everything else, i.e.
no host programs? That covers most of the Rust things so far and would
have helped with the warning I linked above (which is why I would like
to change this -- one can pass the flag manually, of course, but
having more targets affected by `CONFIG_WERROR` means more developers
will have it enabled and thus notice earlier).

(I am also thinking whether it could make sense to eventually
explicitly mark the C host programs that would be exempt from
`CONFIG_WERROR` so that we could apply it to everything else -- I
guess it could be easy to get wrong and/or forget when new ones are
added.)

Cheers,
Miguel

Re: [PATCH 2/3] kbuild: rust: apply `CONFIG_WERROR` to all Rust targets

[Masahiro Yamada]

On Tue, May 21, 2024 at 5:05 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Tue, May 21, 2024 at 6:15 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > Rust started to do something different from C.
> >
> > KBUILD_HOSTCFLAGS is not affected by any CONFIG option.
> >
> > The reason is because HOSTCC is needed for building Kconfig.
> > If the flags for HOSTCC is changed by a CONFIG option,
> > it would be a chicken-egg problem.
> > Also, some host programs might be compiled even
> > without .config at all. (e.g. scripts/unifdef)
> >
> > I know Rust will not become a part of the core infrastructure
> > of the build system, but IMHO, host programs should not be
> > affected by any CONFIG option.
> >
> > I do not like this patch.
>
> Thanks Masahiro -- yeah, I can see how it makes sense for C host
> programs, and consistency with those may be best, even if it is not
> used for core infrastructure in the case of Rust.
>
> Do you think it would be OK if we do it only for everything else, i.e.
> no host programs?


What does "everything else" mean exactly?


I just noticed that $(rust_common_flags) is passed
not only to RUSTC but also to RUSTDOC.


It seems to be intentional because it explicitly says
"requires kernel .config".


@echo  '  rustdoc   - Generate Rust documentation'
@echo  '     (requires kernel .config)'


This is different from how other "make htmldocs" etc. work.

Why is the .config required for generating documentation?









> That covers most of the Rust things so far and would
> have helped with the warning I linked above (which is why I would like
> to change this -- one can pass the flag manually, of course, but
> having more targets affected by `CONFIG_WERROR` means more developers
> will have it enabled and thus notice earlier).
>
> (I am also thinking whether it could make sense to eventually
> explicitly mark the C host programs that would be exempt from
> `CONFIG_WERROR` so that we could apply it to everything else -- I
> guess it could be easy to get wrong and/or forget when new ones are
> added.)
>
> Cheers,
> Miguel



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/3] kbuild: rust: apply `CONFIG_WERROR` to all Rust targets

[Miguel Ojeda]

On Wed, May 22, 2024 at 12:14 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> What does "everything else" mean exactly?

Everything but the host programs. Or as many targets as possible, if
you think there are other cases that we should avoid.

> Why is the .config required for generating documentation?

`rustdoc` sees the code in a similar way as the compiler (it uses
parts of the compiler); in particular, it processes conditional
compilation like the compiler. So we need a given configuration to
generate it.

Cheers,
Miguel

Re: [PATCH 2/3] kbuild: rust: apply `CONFIG_WERROR` to all Rust targets

[Masahiro Yamada]

On Wed, May 22, 2024 at 7:52 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Wed, May 22, 2024 at 12:14 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > What does "everything else" mean exactly?
>
> Everything but the host programs. Or as many targets as possible, if
> you think there are other cases that we should avoid.


You can do this if rebuilding makes sense
when any CONFIG option is changed.



> > Why is the .config required for generating documentation?
>
> `rustdoc` sees the code in a similar way as the compiler (it uses
> parts of the compiler); in particular, it processes conditional
> compilation like the compiler. So we need a given configuration to
> generate it.

Surprising.

It potentially generates different documentations
depending on the .config file.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/3] kbuild: rust: move `-Dwarnings` handling to `Makefile.extrawarn`

[Andreas Hindborg]

Miguel Ojeda <ojeda@kernel.org> writes:

> Following commit e88ca24319e4 ("kbuild: consolidate warning flags
> in scripts/Makefile.extrawarn"), move `-Dwarnings` handling into
> `Makefile.extrawarn` like C's `-Werror`.
>
> No functional change intended.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>


Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

Re: [PATCH 2/3] kbuild: rust: apply `CONFIG_WERROR` to all Rust targets

[Andreas Hindborg]

Miguel Ojeda <ojeda@kernel.org> writes:

> Make all Rust code (i.e. not just kernel code) respect `CONFIG_WERROR`,
> so that we keep everything warning clean.
>
> In particular, this affects targets in `rust/` (`RUSTDOC H`, `RUSTC TL`,
> `RUSTDOC T`, `RUSTC T` and `RUSTC P`), plus host programs and any others
> we may add later.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Tested-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

Re: [PATCH 3/3] kbuild: rust: support `W=e` for Rust

[Andreas Hindborg]

Miguel Ojeda <ojeda@kernel.org> writes:

> With `W=e`, kernel C targets error out on warnings.
>
> Add support for the same feature for Rust code, but take the opportunity
> to apply it for every Rust target (i.e. not just kernel code), so that
> it behaves like having set `CONFIG_WERROR`.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Tested-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

Re: [PATCH 2/3] kbuild: rust: apply `CONFIG_WERROR` to all Rust targets

[Andreas Hindborg]

Masahiro Yamada <masahiroy@kernel.org> writes:

> On Wed, May 22, 2024 at 7:52 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> On Wed, May 22, 2024 at 12:14 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>> >
>> > What does "everything else" mean exactly?
>>
>> Everything but the host programs. Or as many targets as possible, if
>> you think there are other cases that we should avoid.
>
>
> You can do this if rebuilding makes sense
> when any CONFIG option is changed.

Not having the ability to make builds fail when clippy or rustdoc is
issuing warnings is a big problem. I would very much like to have the
option to make builds fail when these targets give warnings.

@Miguel: Should I look into exempting host programs, or do you already
have an idea of how to implement?


Regarding host programs, it would be nice to set werror for those. How
does C do this? Is W=e enough?


Best regards,
Andreas Hindborg

Re: [PATCH 2/3] kbuild: rust: apply `CONFIG_WERROR` to all Rust targets

[Miguel Ojeda]

On Fri, Jan 10, 2025 at 2:33 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> @Miguel: Should I look into exempting host programs, or do you already
> have an idea of how to implement?

I will send a v2. I may take the chance to already apply the first
patch into `rust-next`, since that could go in already.

> Regarding host programs, it would be nice to set werror for those. How
> does C do this? Is W=e enough?

Neither `CONFIG_WERROR` nor `W=e` apply to C host programs.

However, you may already do:

    make ... HOSTCFLAGS=-Werror

And for Rust:

    make ... HOSTRUSTFLAGS=-Dwarnings

I could perhaps add that into the docs.

I hope that helps, and thanks for the tags!

Cheers,
Miguel