| Message ID | 20221127130739.1862398-13-ricardo.martincoski@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | check-symbols v2 | expand |
>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes: > There are two legitimate cases to prefer ifdef over ifeq in package > recipes: command-line overrides are allowed for busybox and uclibc > configs. > Except for that, all package in tree already use ifeq, so warn the > developer adding/changing a package to use ifeq instead of ifdef, in > order to keep consistence across packages. > file.mk:2: use ifeq ($(SYMBOL),y) instead of ifdef SYMBOL > file.mk:5: use ifneq ($(SYMBOL),y) instead of ifndef SYMBOL > The difference between ifeq and ifdef is that ifdef doesn't expand > recursively. > Add comments to busybox and uclibc packages to avoid a warning in such > special cases. > Cc: Arnout Vandecappelle <arnout@mind.be> > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com> > --- > NOTE 1: > I only state "all package in tree already use ifeq" because earlier in > the series other 3 (2 ifdef and 1 ifndef) uses were removed. Committed, thanks.
>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes: > There are two legitimate cases to prefer ifdef over ifeq in package > recipes: command-line overrides are allowed for busybox and uclibc > configs. > Except for that, all package in tree already use ifeq, so warn the > developer adding/changing a package to use ifeq instead of ifdef, in > order to keep consistence across packages. > file.mk:2: use ifeq ($(SYMBOL),y) instead of ifdef SYMBOL > file.mk:5: use ifneq ($(SYMBOL),y) instead of ifndef SYMBOL > The difference between ifeq and ifdef is that ifdef doesn't expand > recursively. > Add comments to busybox and uclibc packages to avoid a warning in such > special cases. > Cc: Arnout Vandecappelle <arnout@mind.be> > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com> > --- > NOTE 1: > I only state "all package in tree already use ifeq" because earlier in > the series other 3 (2 ifdef and 1 ifndef) uses were removed. Committed to 2022.11.x and 2022.02.x, thanks.
diff --git a/package/busybox/busybox.mk b/package/busybox/busybox.mk index 0f14bf999d..f8f9cb5616 100644 --- a/package/busybox/busybox.mk +++ b/package/busybox/busybox.mk @@ -110,6 +110,7 @@ BUSYBOX_MAKE_OPTS = \ # specifying BUSYBOX_CONFIG_FILE on the command-line overrides the .config # setting. +# check-package disable Ifdef ifndef BUSYBOX_CONFIG_FILE BUSYBOX_CONFIG_FILE = $(call qstrip,$(BR2_PACKAGE_BUSYBOX_CONFIG)) endif diff --git a/package/uclibc/uclibc.mk b/package/uclibc/uclibc.mk index 0ddf7dfa6d..125aa4cdcf 100644 --- a/package/uclibc/uclibc.mk +++ b/package/uclibc/uclibc.mk @@ -22,6 +22,7 @@ UCLIBC_DEPENDENCIES = host-gcc-initial linux-headers # specifying UCLIBC_CONFIG_FILE on the command-line overrides the .config # setting. +# check-package disable Ifdef ifndef UCLIBC_CONFIG_FILE UCLIBC_CONFIG_FILE = $(call qstrip,$(BR2_UCLIBC_CONFIG)) endif diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py index b50a19ac62..8adf844e9a 100644 --- a/utils/checkpackagelib/lib_mk.py +++ b/utils/checkpackagelib/lib_mk.py @@ -21,6 +21,24 @@ continue_conditional = ["elif", "else"] end_conditional = ["endif"] +class Ifdef(_CheckFunction): + IFDEF = re.compile(r"^\s*(else\s+|)(ifdef|ifndef)\s") + + def check_line(self, lineno, text): + m = self.IFDEF.search(text) + if m is None: + return + word = m.group(2) + if word == 'ifdef': + return ["{}:{}: use ifeq ($(SYMBOL),y) instead of ifdef SYMBOL" + .format(self.filename, lineno), + text] + else: + return ["{}:{}: use ifneq ($(SYMBOL),y) instead of ifndef SYMBOL" + .format(self.filename, lineno), + text] + + class Indent(_CheckFunction): COMMENT = re.compile(r"^\s*#") CONDITIONAL = re.compile(r"^\s*({})\s".format("|".join(start_conditional + end_conditional + continue_conditional))) diff --git a/utils/checkpackagelib/test_lib_mk.py b/utils/checkpackagelib/test_lib_mk.py index 49fa216fcd..80a1736b4e 100644 --- a/utils/checkpackagelib/test_lib_mk.py +++ b/utils/checkpackagelib/test_lib_mk.py @@ -3,6 +3,54 @@ import checkpackagelib.test_util as util import checkpackagelib.lib_mk as m +Ifdef = [ + ('ignore commented line', + 'any', + '# ifdef\n', + []), + ('simple', + 'any', + '\n' + 'ifdef BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE\n' + 'endif\n', + [['any:2: use ifeq ($(SYMBOL),y) instead of ifdef SYMBOL', + 'ifdef BR2_PACKAGE_FWTS_EFI_RUNTIME_MODULE\n']]), + ('ignore indentation', + 'any', + ' ifdef FOO\n' + ' endif\n' + '\tifdef BAR\n' + 'endif\n', + [['any:1: use ifeq ($(SYMBOL),y) instead of ifdef SYMBOL', + ' ifdef FOO\n'], + ['any:3: use ifeq ($(SYMBOL),y) instead of ifdef SYMBOL', + '\tifdef BAR\n']]), + ('typo', + 'any', + '\n' + 'ifndef ($(BR2_ENABLE_LOCALE),y)\n' + 'endif\n', + [['any:2: use ifneq ($(SYMBOL),y) instead of ifndef SYMBOL', + 'ifndef ($(BR2_ENABLE_LOCALE),y)\n']]), + ('else ifdef', + 'any', + 'else ifdef SYMBOL # comment\n', + [['any:1: use ifeq ($(SYMBOL),y) instead of ifdef SYMBOL', + 'else ifdef SYMBOL # comment\n']]), + ('else ifndef', + 'any', + '\t else ifndef\t($(SYMBOL),y) # comment\n', + [['any:1: use ifneq ($(SYMBOL),y) instead of ifndef SYMBOL', + '\t else ifndef\t($(SYMBOL),y) # comment\n']]), + ] + + +@pytest.mark.parametrize('testname,filename,string,expected', Ifdef) +def test_Ifdef(testname, filename, string, expected): + warnings = util.check_file(m.Ifdef, filename, string) + assert warnings == expected + + Indent = [ ('ignore comment at beginning of line', 'any',
There are two legitimate cases to prefer ifdef over ifeq in package recipes: command-line overrides are allowed for busybox and uclibc configs. Except for that, all package in tree already use ifeq, so warn the developer adding/changing a package to use ifeq instead of ifdef, in order to keep consistence across packages. file.mk:2: use ifeq ($(SYMBOL),y) instead of ifdef SYMBOL file.mk:5: use ifneq ($(SYMBOL),y) instead of ifndef SYMBOL The difference between ifeq and ifdef is that ifdef doesn't expand recursively. Add comments to busybox and uclibc packages to avoid a warning in such special cases. Cc: Arnout Vandecappelle <arnout@mind.be> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com> --- NOTE 1: I only state "all package in tree already use ifeq" because earlier in the series other 3 (2 ifdef and 1 ifndef) uses were removed. NOTE 2: We have both ifneq ($(BR2_ENABLE_LOCALE),y) and ifeq ($(BR2_ENABLE_LOCALE),) in the tree. I assume both do the same. So I choose one for the check-package warning message, for no particular reason. package/dialog/dialog.mk:ifneq ($(BR2_ENABLE_LOCALE),y) package/dosfstools/dosfstools.mk:ifneq ($(BR2_ENABLE_LOCALE),y) package/dvb-apps/dvb-apps.mk:ifeq ($(BR2_ENABLE_LOCALE),) package/gettext-gnu/gettext-gnu.mk:ifeq ($(BR2_ENABLE_LOCALE),) package/guile/guile.mk:ifeq ($(BR2_ENABLE_LOCALE),) package/kodi-audioencoder-lame/kodi-audioencoder-lame.mk:ifeq ($(BR2_ENABLE_LOCALE),) package/kodi/kodi.mk:ifeq ($(BR2_ENABLE_LOCALE),) package/libcddb/libcddb.mk:ifeq ($(BR2_ENABLE_LOCALE),) package/libcdio/libcdio.mk:ifeq ($(BR2_ENABLE_LOCALE),) package/libglib2/libglib2.mk:ifneq ($(BR2_ENABLE_LOCALE),y) package/libpsl/libpsl.mk:ifeq ($(BR2_ENABLE_LOCALE),) package/lsof/lsof.mk:ifeq ($(BR2_ENABLE_LOCALE),) package/sdl_sound/sdl_sound.mk:ifneq ($(BR2_ENABLE_LOCALE),y) package/softether/softether.mk:ifeq ($(BR2_ENABLE_LOCALE),) package/uclibc-ng-test/uclibc-ng-test.mk:ifeq ($(BR2_ENABLE_LOCALE),) NOTE 3: Applying only this patch on current master, check-package returns: package/pugixml/pugixml.mk:32: use ifeq ($(SYMBOL),y) instead of ifdef SYMBOL package/live555/live555.mk:42: use ifneq ($(SYMBOL),y) instead of ifndef SYMBOL package/fwts/fwts.mk:18: use ifeq ($(SYMBOL),y) instead of ifdef SYMBOL --- package/busybox/busybox.mk | 1 + package/uclibc/uclibc.mk | 1 + utils/checkpackagelib/lib_mk.py | 18 +++++++++++ utils/checkpackagelib/test_lib_mk.py | 48 ++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+)