Hi Simon
On Wed, Feb 1, 2023 at 12:21 PM Simon Glass <s...@chromium.org> wrote:
> Hi Troy,
>
> On Tue, 31 Jan 2023 at 12:57, Troy Kisky <troykiskybound...@gmail.com>
> wrote:
> >
> > Add script usage_of_is_enabled_check to print any configs that
> > use CONFIG_IS_ENABLED instead of IS_ENABLED and vice versa.
> >
> > Add usage_of_is_enabled_commit.sh to generate commits to fix the above
> > issues.
> >
> > Signed-off-by: Troy Kisky <troykiskybound...@gmail.com>
> > ---
> > .azure-pipelines.yml | 11 ++++++++
> > .gitlab-ci.yml | 5 ++++
> > test/usage_of_is_enabled_check.sh | 41 ++++++++++++++++++++++++++++++
> > test/usage_of_is_enabled_commit.sh | 38 +++++++++++++++++++++++++++
> > 4 files changed, 95 insertions(+)
> > create mode 100755 test/usage_of_is_enabled_check.sh
> > create mode 100755 test/usage_of_is_enabled_commit.sh
>
> This looks reasonable to me. I would of course prefer to just fix all
> these problems, since they make it impossible to move to split config.
>
So, this might help your effort ?
Do I need to have a body in these 200 commits or is only a subject enough ?
Are you happy with the subject, or do I need to edit each individually ?
Currently I have
CONFIG_ZSTD: change IS_ENABLED to CONFIG_IS_ENABLED
Is there an easy way for "git send-email" to send each commit only to the
proper maintainer ?
>
> But we need this sort of check in CI to stop it happening again.
>
> >
> > diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> > index 5673bb76afb..8e227512765 100644
> > --- a/.azure-pipelines.yml
> > +++ b/.azure-pipelines.yml
> > @@ -67,6 +67,17 @@ stages:
> > :^doc/ :^arch/arm/dts/ :^scripts/kconfig/lkc.h
> > :^include/linux/kconfig.h :^tools/ && exit 1 || exit 0
> >
> > + - job: check_usage_of_is_enabled
> > + displayName: 'Check usage of CONFIG_IS_ENABLED vs IS_ENABLED'
> > + pool:
> > + vmImage: $(ubuntu_vm)
> > + container:
> > + image: $(ci_runner_image)
> > + options: $(container_option)
> > + steps:
> > + # generate list of SPL configs
> > + - script: test/usage_of_is_enabled_check.sh
> > +
> > - job: cppcheck
> > displayName: 'Static code analysis with cppcheck'
> > pool:
> > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > index aaf9d25abfc..6bb8efef258 100644
> > --- a/.gitlab-ci.yml
> > +++ b/.gitlab-ci.yml
> > @@ -134,6 +134,11 @@ check for new CONFIG symbols outside Kconfig:
> > :^doc/ :^arch/arm/dts/ :^scripts/kconfig/lkc.h
> > :^include/linux/kconfig.h :^tools/ && exit 1 || exit 0
> >
> > +check usage of CONFIG_IS_ENABLED vs IS_ENABLED:
> > + stage: testsuites
> > + script:
> > + - ./test/usage_of_is_enabled_check.sh
> > +
> > # QA jobs for code analytics
> > # static code analysis with cppcheck (we can add --enable=all later)
> > cppcheck:
> > diff --git a/test/usage_of_is_enabled_check.sh
> b/test/usage_of_is_enabled_check.sh
> > new file mode 100755
> > index 00000000000..0bc9bff8bd1
> > --- /dev/null
> > +++ b/test/usage_of_is_enabled_check.sh
> > @@ -0,0 +1,41 @@
> > +#!/bin/bash
> > +# generate list of CONFIGs that should use CONFIG_IS_ENABLED
>
> Could you add a few comments about the steps in here?
>
+# 1. all obj-$(CONFIG_$(SPL_)xxx in Makefiles
+# 2. all SPL_xxx in Kconfig files
+# 3. all CONFIG_CMD_xxx which already use CONFIG_IS_ENABLED
+# The Makefile for most if these use ifndef CONFIG_SPL_BUILD
+# instead of obj-$(CONFIG_$(SPL_)xxx
+# 4. A list of other configs that should use CONFIG_IS_ENABLED
+# Note: CONFIG_CLK was included to prevent a change in
test_checkpatch.py
+# which is checking for an error.
+# This list could be reduced if obj-$(CONFIG_$(SPL_)xxx was used
instead of
+# ifndef CONFIG_SPL_BUILD in Makefiles
>
> > +{ { git grep 'obj-$(CONFIG_$(SPL_'|grep Makefile|sed -e
> "s/SPL_TPL_/SPL_/"| \
> > +sed -n -r
> 's/obj\-\$\(CONFIG_\$\(SPL_\)([0-9a-zA-Z_]+)\)/\n\{\1\}\n/gp'| \
> > +sed -n -r 's/\{([0-9a-zA-Z_]+)\}/\1/p'; } ;\
> > +{ git grep -E 'config [ST]PL_'|grep Kconfig| \
> > +sed -n -r "s/config [ST]PL_([0-9a-zA-Z_]+)/\n\{\1\}\n/p" |
> > +sed -n -r 's/\{([0-9a-zA-Z_]+)\}/\1/p'; } ; \
> > +git grep -E 'CONFIG_IS_ENABLED\(CMD_'|sed -n -e
> "s/\(CONFIG_IS_ENABLED(CMD_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > +sed -n -r "s/CONFIG_IS_ENABLED\((CMD_[0-9a-zA-Z_]+)\)/\1/p"; \
> > +echo -e "\
> > +BZIP2\n\
> > +CONFIG_CLK\n\
>
> CLK ?
>
>
Addressed above
> > +DM_EVENT\n\
> > +EFI_DEVICE_PATH_TO_TEXT\n\
> > +EFI_LOADER\n\
> > +ERRNO_STR\n\
> > +GENERATE_SMBIOS_TABLE\n\
> > +";\
>
> What is the above list for? Can you add a comment?
>
>
Addressed above
> > +} | sort -u >splcfg.tmp
> > +
> > +{
> > +# generate list of CONFIGs that incorrectly use CONFIG_IS_ENABLED
> > +git grep CONFIG_IS_ENABLED|sed -n -e
> "s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > +sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u| comm
> -23 - splcfg.tmp ;
> > +
> > +# generate list of CONFIGs that incorrectly use IS_ENABLED
> > +git grep -w IS_ENABLED|sed -n -e
> "s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > +sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u| join
> - splcfg.tmp;
> > +} | grep -vw FOO;
> > +if [ $? -eq 0 ] ; then
> > + echo "The above may have incorrect usage of
> IS_ENABLED/CONFIG_IS_ENABLED"
> > + echo "Run test/usage_of_is_enabled_commit.sh and squash with
> appropriate commit"
> > + ret=1;
> > +else
> > + ret=0;
> > +fi
> > +
> > +rm splcfg.tmp
> > +exit ${ret}
> > +
> > diff --git a/test/usage_of_is_enabled_commit.sh
> b/test/usage_of_is_enabled_commit.sh
> > new file mode 100755
> > index 00000000000..f776d0c69ab
> > --- /dev/null
> > +++ b/test/usage_of_is_enabled_commit.sh
> > @@ -0,0 +1,38 @@
> > +#!/bin/bash
> > +# generate list of CONFIGs that should use CONFIG_IS_ENABLED
>
> Same thing, could use some comments
>
Same added here
Thanks
Troy
> > +{ { git grep 'obj-$(CONFIG_$(SPL_'|grep Makefile|sed -e
> "s/SPL_TPL_/SPL_/"| \
> > +sed -n -r
> 's/obj\-\$\(CONFIG_\$\(SPL_\)([0-9a-zA-Z_]+)\)/\n\{\1\}\n/gp'| \
> > +sed -n -r 's/\{([0-9a-zA-Z_]+)\}/\1/p'; } ;\
> > +{ git grep -E 'config [ST]PL_'|grep Kconfig| \
> > +sed -n -r "s/config [ST]PL_([0-9a-zA-Z_]+)/\n\{\1\}\n/p" |
> > +sed -n -r 's/\{([0-9a-zA-Z_]+)\}/\1/p'; } ; \
> > +git grep -E 'CONFIG_IS_ENABLED\(CMD_'|sed -n -e
> "s/\(CONFIG_IS_ENABLED(CMD_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > +sed -n -r "s/CONFIG_IS_ENABLED\((CMD_[0-9a-zA-Z_]+)\)/\1/p"; \
> > +echo -e "\
> > +BZIP2\n\
> > +CONFIG_CLK\n\
> > +DM_EVENT\n\
> > +EFI_DEVICE_PATH_TO_TEXT\n\
> > +EFI_LOADER\n\
> > +ERRNO_STR\n\
> > +GENERATE_SMBIOS_TABLE\n\
> > +";\
> > +} | sort -u >splcfg.tmp
> > +
> > +
> > +git grep CONFIG_IS_ENABLED|sed -n -e
> "s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > +sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|grep
> -vw FOO| \
> > +comm -23 - splcfg.tmp|xargs -I {} \
> > +sh -c "git grep -l 'CONFIG_IS_ENABLED({})' | \
> > +xargs -IFile sh -c \"sed -i -e
> \\\"s/CONFIG_IS_ENABLED({})/IS_ENABLED(CONFIG_{})/g\\\" File\" ; \
> > +git commit -a -m\"CONFIG_{}: change CONFIG_IS_ENABLED to IS_ENABLED\";"
> > +
> > +git grep -w IS_ENABLED|sed -n -e
> "s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > +sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|grep
> -vw FOO| \
> > +join - splcfg.tmp |xargs -I {} \
> > +sh -c "git grep -l 'IS_ENABLED(CONFIG_{})' | \
> > +xargs -IFile sh -c \"sed -i -e
> \\\"s/\([^_]\)IS_ENABLED(CONFIG_{})/\1CONFIG_IS_ENABLED({})/g\\\" File\" ; \
> > +git commit -a -m\"CONFIG_{}: change IS_ENABLED to CONFIG_IS_ENABLED\";"
> > +
> > +rm splcfg.tmp
> > +
> > --
> > 2.34.1
> >
>
> Regards,
> SImon
>
