| Message ID | 20240519211235.589325-2-ojeda@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] kbuild: rust: move `-Dwarnings` handling to `Makefile.extrawarn` | expand |
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>
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
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
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
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
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.
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
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
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
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
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(-) -- 2.45.1