| Message ID | 20240519211235.589325-2-ojeda@kernel.org (mailing list archive) |
|---|---|
| State | New |
| 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.
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