| Message ID | 20241123-resolve_btfids-v1-3-927700b641d1@weissschuh.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kbuild: propagate CONFIG_WERROR to resolve_btfids | expand |
On Sat, Nov 23, 2024 at 5:33 AM Thomas Weißschuh <linux@weissschuh.net> wrote: > > Use CONFIG_WERROR to also fail on warnings emitted by resolve_btfids. > Allow the CI bots to prevent the introduction of new warnings. > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > scripts/link-vmlinux.sh | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh > index a9b3f34a78d2cd4514e73a728f1a784eee891768..61f1f670291351a276221153146d66001eca556c 100755 > --- a/scripts/link-vmlinux.sh > +++ b/scripts/link-vmlinux.sh > @@ -274,7 +274,11 @@ vmlinux_link vmlinux > # fill in BTF IDs > if is_enabled CONFIG_DEBUG_INFO_BTF; then > info BTFIDS vmlinux > - ${RESOLVE_BTFIDS} vmlinux > + RESOLVE_BTFIDS_ARGS="" > + if is_enabled CONFIG_WERROR; then > + RESOLVE_BTFIDS_ARGS=" --fatal-warnings " > + fi > + ${RESOLVE_BTFIDS} ${RESOLVE_BTFIDS_ARGS} vmlinux I'm not convinced we need to fail the build when functions are renamed. These warns are eventually found and fixed.
On 2024-11-24 15:38:40-0800, Alexei Starovoitov wrote: > On Sat, Nov 23, 2024 at 5:33 AM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > Use CONFIG_WERROR to also fail on warnings emitted by resolve_btfids. > > Allow the CI bots to prevent the introduction of new warnings. > > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > > --- > > scripts/link-vmlinux.sh | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh > > index a9b3f34a78d2cd4514e73a728f1a784eee891768..61f1f670291351a276221153146d66001eca556c 100755 > > --- a/scripts/link-vmlinux.sh > > +++ b/scripts/link-vmlinux.sh > > @@ -274,7 +274,11 @@ vmlinux_link vmlinux > > # fill in BTF IDs > > if is_enabled CONFIG_DEBUG_INFO_BTF; then > > info BTFIDS vmlinux > > - ${RESOLVE_BTFIDS} vmlinux > > + RESOLVE_BTFIDS_ARGS="" > > + if is_enabled CONFIG_WERROR; then > > + RESOLVE_BTFIDS_ARGS=" --fatal-warnings " > > + fi > > + ${RESOLVE_BTFIDS} ${RESOLVE_BTFIDS_ARGS} vmlinux > > I'm not convinced we need to fail the build when functions are renamed. > These warns are eventually found and fixed. The same could be said for most other build warnings. CONFIG_WERROR is a well-known opt-in switch for exactly this behavior. Fixing these warnings before they hit mainline has various advantages. The author introducing the warning knows about the full impact of their change, discussions can be had when everybody still has the topic fresh on their mind and other unrelated people don't get confused, like me or [0]. The "eventually fixed" part seems to have been me the last two times :-) Given the fairly simple implementation, in my opinion this is worth doing. Please note that I have two fairly trivial changes for a v2 and would also like to get some feedback from Masahiro, especially for patch 1. Thomas [0] https://lore.kernel.org/lkml/20241113093703.9936-1-laura.nao@collabora.com/
On Mon, Nov 25, 2024 at 09:20:37AM +0100, Thomas Weißschuh wrote: > On 2024-11-24 15:38:40-0800, Alexei Starovoitov wrote: > > On Sat, Nov 23, 2024 at 5:33 AM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > > > Use CONFIG_WERROR to also fail on warnings emitted by resolve_btfids. > > > Allow the CI bots to prevent the introduction of new warnings. > > > > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > > > --- > > > scripts/link-vmlinux.sh | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh > > > index a9b3f34a78d2cd4514e73a728f1a784eee891768..61f1f670291351a276221153146d66001eca556c 100755 > > > --- a/scripts/link-vmlinux.sh > > > +++ b/scripts/link-vmlinux.sh > > > @@ -274,7 +274,11 @@ vmlinux_link vmlinux > > > # fill in BTF IDs > > > if is_enabled CONFIG_DEBUG_INFO_BTF; then > > > info BTFIDS vmlinux > > > - ${RESOLVE_BTFIDS} vmlinux > > > + RESOLVE_BTFIDS_ARGS="" > > > + if is_enabled CONFIG_WERROR; then > > > + RESOLVE_BTFIDS_ARGS=" --fatal-warnings " > > > + fi > > > + ${RESOLVE_BTFIDS} ${RESOLVE_BTFIDS_ARGS} vmlinux > > > > I'm not convinced we need to fail the build when functions are renamed. > > These warns are eventually found and fixed. > > The same could be said for most other build warnings. > CONFIG_WERROR is a well-known opt-in switch for exactly this behavior. > > Fixing these warnings before they hit mainline has various > advantages. The author introducing the warning knows about the full > impact of their change, discussions can be had when everybody still > has the topic fresh on their mind and other unrelated people don't get > confused, like me or [0]. > > The "eventually fixed" part seems to have been me the last two times :-) > > Given the fairly simple implementation, in my opinion this is worth doing. > > Please note that I have two fairly trivial changes for a v2 and would > also like to get some feedback from Masahiro, especially for patch 1. ok, I think it's fine to fail for CONFIG_WERROR option, for patchset: Acked-by: Jiri Olsa <jolsa@kernel.org> thanks, jirka > > > Thomas > > [0] https://lore.kernel.org/lkml/20241113093703.9936-1-laura.nao@collabora.com/
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh index a9b3f34a78d2cd4514e73a728f1a784eee891768..61f1f670291351a276221153146d66001eca556c 100755 --- a/scripts/link-vmlinux.sh +++ b/scripts/link-vmlinux.sh @@ -274,7 +274,11 @@ vmlinux_link vmlinux # fill in BTF IDs if is_enabled CONFIG_DEBUG_INFO_BTF; then info BTFIDS vmlinux - ${RESOLVE_BTFIDS} vmlinux + RESOLVE_BTFIDS_ARGS="" + if is_enabled CONFIG_WERROR; then + RESOLVE_BTFIDS_ARGS=" --fatal-warnings " + fi + ${RESOLVE_BTFIDS} ${RESOLVE_BTFIDS_ARGS} vmlinux fi mksysmap vmlinux System.map
Use CONFIG_WERROR to also fail on warnings emitted by resolve_btfids. Allow the CI bots to prevent the introduction of new warnings. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- scripts/link-vmlinux.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)