| Message ID | 1474862702-16580-6-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Rejected |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Masahiro, On 25 September 2016 at 22:05, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Currently, the check-config.sh terminates the build when unknown > ad-hoc options are detected. I think it is too much because we may > want to patch config headers locally in a build/deployment project. > > So, let's relax check-config.sh to just warn even if it detects > options that are not in the whitelist. Instead, this check can be > done at the end of build, along with other checks. It will catch > more attention. > > Even with this change, the Buildman tool catches new warnings, > so Tom can give NACK to new ad-hoc options. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > scripts/check-config.sh | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) I am not sure this is a good idea. We need buildman to fail the build; otherwise patches will make it to mainline with new CONFIGs. Perhaps we should have an ENV variable to skip the check? Regards, Simon
Hi Simon, 2016-09-27 9:34 GMT+09:00 Simon Glass <sjg@chromium.org>: > Hi Masahiro, > > On 25 September 2016 at 22:05, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: >> Currently, the check-config.sh terminates the build when unknown >> ad-hoc options are detected. I think it is too much because we may >> want to patch config headers locally in a build/deployment project. >> >> So, let's relax check-config.sh to just warn even if it detects >> options that are not in the whitelist. Instead, this check can be >> done at the end of build, along with other checks. It will catch >> more attention. >> >> Even with this change, the Buildman tool catches new warnings, >> so Tom can give NACK to new ad-hoc options. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> --- >> >> scripts/check-config.sh | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) > > I am not sure this is a good idea. We need buildman to fail the build; > otherwise patches will make it to mainline with new CONFIGs. Perhaps > we should have an ENV variable to skip the check? It is OK for me as long as there is a way to work-around this check, ENV or whatever.
On Mon, Sep 26, 2016 at 01:05:02PM +0900, Masahiro Yamada wrote: > Currently, the check-config.sh terminates the build when unknown > ad-hoc options are detected. I think it is too much because we may > want to patch config headers locally in a build/deployment project. I'm not yet convinced this is a good use case. In the not-so-long-now term include/configs/ should go away. So you wouldn't be able to do an ad-hoc thing like this for testing. And tossing stuff ad-hoc into $(BOARDDIR)/Kconfig should be easy enough too, yes?
Hi Tom, Simon, 2016-09-28 2:44 GMT+09:00 Tom Rini <trini@konsulko.com>: > On Mon, Sep 26, 2016 at 01:05:02PM +0900, Masahiro Yamada wrote: > >> Currently, the check-config.sh terminates the build when unknown >> ad-hoc options are detected. I think it is too much because we may >> want to patch config headers locally in a build/deployment project. > > I'm not yet convinced this is a good use case. In the not-so-long-now > term include/configs/ should go away. So you wouldn't be able to do an > ad-hoc thing like this for testing. And tossing stuff ad-hoc into > $(BOARDDIR)/Kconfig should be easy enough too, yes? > You are right. I tend to add local-hack options into my header file just because it is easier than creating Kconfig entries and enabling them from a defconfig, but it is possible to do so with a little more effort. So, I retract this patch and marked the Patchwork one as Rejected.
diff --git a/scripts/check-config.sh b/scripts/check-config.sh index 6618dfb..9d2cfc6 100755 --- a/scripts/check-config.sh +++ b/scripts/check-config.sh @@ -37,14 +37,13 @@ cat `find ${srctree} -name "Kconfig*"` |sed -n \ -e 's/^menuconfig \([A-Za-z0-9_]*\).*$/CONFIG_\1/p' |sort |uniq > ${ok} comm -23 ${suspects} ${ok} >${new_adhoc} if [ -s ${new_adhoc} ]; then - echo "Error: You must add new CONFIG options using Kconfig" + echo "Warning: You must add new CONFIG options using Kconfig" echo "The following new ad-hoc CONFIG options were detected:" cat ${new_adhoc} echo echo "Please add these via Kconfig instead. Find a suitable Kconfig" echo "file and add a 'config' or 'menuconfig' option." # Don't delete the temporary files in case they are useful - exit 1 else rm ${suspects} ${ok} ${new_adhoc} fi
Currently, the check-config.sh terminates the build when unknown ad-hoc options are detected. I think it is too much because we may want to patch config headers locally in a build/deployment project. So, let's relax check-config.sh to just warn even if it detects options that are not in the whitelist. Instead, this check can be done at the end of build, along with other checks. It will catch more attention. Even with this change, the Buildman tool catches new warnings, so Tom can give NACK to new ad-hoc options. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- scripts/check-config.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)