kernel: introduce KERNEL_WERROR config option

[ynezz]

In commit b2d1eb7 ("generic: 5.15: enable Werror by default for kernel compile") CONFIG_WERROR=y was enabled and all warnings/errors reported with GCC 12 were fixed.

Keeping this in sync with past/future GCC versions is going to be uphill battle, so lets introduce new KERNEL_WERROR config option, enable it by default only for tested/known working combinations and on buildbots.

References: #12687 #12622 #12734

[neheb]

OTOH it being painful was the reason for upstream's introduction of Werror.

[robimarko]

Enabling it on GCC12 now only delays the same warnings appearing once GCC13 is the default, and as @neheb pointed out its supposed to be painful.

[ynezz]

Enabling it on GCC12 now only delays the same warnings appearing once GCC13 is the default.

It would be spotted on buildbots as I've tied it to CONFIG_BUILDBOT option as well. I've suggested to enable CONFIG_BUILDBOT=y in the CI as well, but it was rejected. I'm not sure if we want to add CONFIG_CI=y to enable checks for the CI only.

[robimarko]

My comment was more in a sense that option of disabling WERROR should not even exist.
Also, to me it makes more sense to have CI catch the warning first before buildbots even get a chance to build it.

[neheb]

@Ansuel has the most say as he introduced Werror and fixed most of the warnings. I guess I shouldn't mention GCC11 and 13 :)

[Ansuel]

Well my 2 cents are that it's ok to make this configurable... but set it to y by default and fix everything that is broken by new gcc version...

Never happen that an old gcc version caused different warning quite the opposite since new gcc version enforce new warning for a better code...

I still see making things fail and not compile the correct way to push people fixing their stuff... (and this was the idea of WERROR in the kernel from the start)
Upstream, these kind of error are quickly fixed and backported so it's not a problem for us (just additional patch that will be dropped later on kernel bump)
It's a problem fixing error with our downstream kernel modules and gcc13 but again gcc13 is experimental, buildbot doesn't use it so I don't see problem in having it currently broken. Having gcc13 not working is actually a good reason to fix each compilation error. And for sure errors can't be that big I already fixed the majority of one so it's really improving the code with the new warning and make it even better... Also 6.1 added some new macro with fortify memset/memcpy and those are also warning that should have been fixed right from the start (due to how sensible these kind of function are)

I know these stuff are time consuming and they are just warning but for real... it's a thing to do once and be done with it... Nothing to maintain...

I've suggested to enable CONFIG_BUILDBOT=y in the CI as wel

Do we have some link on why it was rejected?

[ynezz]

That bugreport #12687 triggered this change, I'm anticipating much more of such bug reports as this is enabled in upcoming stable 23.05 release.

I still see making things fail and not compile the correct way to push people fixing their stuff

Expected behaviour

• Kernel should successfully build

so might be worth for someone to make a PR here with the patch.

IMO it says it all. Folks expect, that its going to work, they're not going to fix our stuff, even if you point them at the fix.

buildbot doesn't use it so I don't see a problem in having it currently broken

I understand, that its a huge amount of work to keep gcc-11, gcc-12 and gcc-13 in working state with such pedantic configuration, so that's why I've introduced that option to relax it a bit. IMO it doesn't need to be broken for end users, its bad user experience.

Do we have some link on why it was rejected?

Rejected is probably not a proper word, lets rephrase it to not considered

[timocapa]

even if you point them at the fix

ive not had time yet to properly look into how to make patches that could/would be accepted in openwrt, sorry.

i'll look at https://openwrt.org/docs/guide-developer/toolchain/use-patches-with-buildsystem in some time.

[ynezz]

I'm anticipating much more of such bug reports as this is enabled in upcoming stable 23.05 release.

Slowly incoming #12734

[mcprat]

I know that the intention is to make user experience better for a stable branch, but that in turn would make the stable branch "less stable" (provided that the specific warning that WERROR turns into an error is not useless).

Just throwing an idea out there, what if stable branches had it enabled by default, and master disabled by default? or maybe the reverse if we really think user experience is more important, master enabled by default, and stable branch disabled by default?

[robimarko]

Considering that master will eventually become next stable that just kicks the can down the road.

[mcprat]

I'm more or less imagining a way for Make to know whether a stable branch is checked out or not and use an if statement. I don't know if it's possible.

(well I'm sure it's possible but maybe not reasonable)

[ynezz]

I know that the intention is to make user experience better for a stable branch

Its bad for use experience, regardless of the branch or the environment. IIUC then original intention is QA purpose, which should be part of internal workflows, we shouldn't impose QA tasks on every single user out there.

The rule of thumb for this project is dead simple, if it breaks, fix or revert it. From the linked bug reports you can clearly see, that its broken for some users, so we should fix it.

Those bug reports always represent just a fraction of the ongoing issues, interpolating it reveals a lot more of such WTF moments.

Fixing the linked issues is not going to cut it, extending the CI matrix with another GCC version is not going to help either, because folks are simply allowed to use different combinations of kernel config options, use out of tree targets, kernel modules etc. so we can't simply CI QA all of this combinations and we're always going to spot something.

so lets introduce new KERNEL_WERROR config option, enable it by default only for tested/known working combinations and on buildbots.

In retrospect this was wrong as well, there is no such tested/known working combination, so KERNEL_WERROR option should be only enabled in our internal QA workflows, such as CI and buildbots.

[Ansuel]

@ynezz I totally understand the reason and you are absolutely right! What I see as a problem is the gcc thing (I would drop it, I get the idea that gcc12 works and every warning is fixed but I feel like that is a hint that we will have to had gcc13 when we will have things fixed, some kind of validation config? I don't like that part since we will have to remember adding stuff to it)

I like the idea of enabling it only in CI and buildbots. For CI we would just enable this new config option but I wonder if a good idea might be introduce a CI config that would enable all of this options? (another config that would be added is the AUTOREMOVE and also CCACHE?) I remember there was an idea so this might be a good time to implement it?

[Ansuel]

I just read the problem with using buildbot config... maybe we can find some common config to improve reproducibility between CI and buildbot?