[Buildroot] [PATCH 0/8] Detect and fix overridden variables in .mk files

[Buildroot] [PATCH 0/8] Detect and fix overridden variables in .mk files

[Ricardo Martincoski]

Hello,

This series makes check-package to warn when a variable is overridden in a .mk
file.

For the general case, appending values to variables is OK and also a good
practice, like this:
|PACKAGE_VAR = value1
|ifeq ...
|PACKAGE_VAR += value2

or this, when the above is not possible:
|PACKAGE_VAR = value1
|ifeq ...
|PACKAGE_VAR := $(PACKAGE_VAR), value2

But this override is an error:
|PACKAGE_VAR = value1
|PACKAGE_VAR = value2

as well this one:
|ifeq ...
|PACKAGE_VAR += value1
|endif
|PACKAGE_VAR = value2

And this override is error-prone:
|PACKAGE_VAR = value1
|ifeq ...
|PACKAGE_VAR = value2

Patches 1 to 3 fix packages that currently overridden variables in .mk files:
s6-networking, sdl_sound, avrdude.

Patch 4 removes a unicode character from usb_modeswitch.mk. When I was about to
send this series, git send-email warned about it.

Patch 5 standardize the code by avoiding conditionally overridden variables in
usb_modeswitch.

Patch 6 adds to check-package the ability to recognize a special comment and
ignore one or more warnings that would otherwise be issued for the next line of
code. This is a common feature for static-analysis tools.

Patch 7 teaches check-package about ifdef/ifndef, problem identified while
implementing patch 8.

Patch 8 adds a new check function to check-package to warn for overridden
variables.

With the entire series applied, a check-package job:
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/151812563

With only patches 6 to 8 applied, a check-package job:
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/151813273

NOTE: I used git format-patch -U16 to make the review of patches 1,2,3,5 easier.

Regards,
Ricardo

Ricardo Martincoski (8):
  package/s6-networking: fix dependency when libressl is enabled
  package/sdl_sound: actually use the optional CONF_OPTS
  Revert "avrdude: add license information"
  package/usb_modeswitch: drop unicode space in comment
  package/usb_modeswitch: avoid overriding variables
  utils/check-package: allow to disable warning for a line
  utils/check-package: handle ifdef/ifndef in .mk files
  utils/check-package: warn about overridden variables

 package/avrdude/avrdude.mk               |  2 -
 package/gcc/gcc-final/gcc-final.mk       |  2 +
 package/s6-networking/s6-networking.mk   |  2 +-
 package/sdl_sound/sdl_sound.mk           | 11 ++--
 package/usb_modeswitch/usb_modeswitch.mk |  8 +--
 package/zmqpp/zmqpp.mk                   |  1 +
 utils/check-package                      |  4 ++
 utils/checkpackagelib/base.py            |  2 +
 utils/checkpackagelib/lib_mk.py          | 70 +++++++++++++++++++++++-
 9 files changed, 86 insertions(+), 16 deletions(-)

-- 
2.17.1

[Buildroot] [PATCH 1/8] package/s6-networking: fix dependency when libressl is enabled

[Ricardo Martincoski]

Commit "c5b85231fb s6-networking: enable SSL if libressl is selected"
actually dropped the dependency on s6-dns and s6 when libressl is
enabled.
Fix this by using += inside the conditional code.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Eric Le Bihan <eric.le.bihan.dev@free.fr>
---
NOTE: I don't know why the autobuilders didn't caught this.

On current master (f6843a75fe), test-pkg with this config:
BR2_PACKAGE_OPENSSL=y
BR2_PACKAGE_LIBRESSL=y
BR2_PACKAGE_S6_NETWORKING=y
results in:
6 builds, 1 skipped, 5 build failed, 0 legal-info failed
After this patch:
6 builds, 1 skipped, 0 build failed, 0 legal-info failed
---
 package/s6-networking/s6-networking.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/s6-networking/s6-networking.mk b/package/s6-networking/s6-networking.mk
index 7c9f087051..b621eb225d 100644
--- a/package/s6-networking/s6-networking.mk
+++ b/package/s6-networking/s6-networking.mk
@@ -12,33 +12,33 @@ S6_NETWORKING_INSTALL_STAGING = YES
 S6_NETWORKING_DEPENDENCIES = s6-dns s6
 
 S6_NETWORKING_CONF_OPTS = \
 	--prefix=/usr \
 	--with-sysdeps=$(STAGING_DIR)/usr/lib/skalibs/sysdeps \
 	--with-include=$(STAGING_DIR)/usr/include \
 	--with-dynlib=$(STAGING_DIR)/usr/lib \
 	--with-lib=$(STAGING_DIR)/usr/lib/execline \
 	--with-lib=$(STAGING_DIR)/usr/lib/s6 \
 	--with-lib=$(STAGING_DIR)/usr/lib/s6-dns \
 	--with-lib=$(STAGING_DIR)/usr/lib/skalibs \
 	$(if $(BR2_STATIC_LIBS),,--disable-allstatic) \
 	$(SHARED_STATIC_LIBS_OPTS)
 
 ifeq ($(BR2_PACKAGE_LIBRESSL),y)
 S6_NETWORKING_CONF_OPTS += --enable-ssl=libressl
-S6_NETWORKING_DEPENDENCIES = libressl
+S6_NETWORKING_DEPENDENCIES += libressl
 endif
 
 define S6_NETWORKING_CONFIGURE_CMDS
 	(cd $(@D); $(TARGET_CONFIGURE_OPTS) ./configure $(S6_NETWORKING_CONF_OPTS))
 endef
 
 define S6_NETWORKING_BUILD_CMDS
 	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D)
 endef
 
 define S6_NETWORKING_REMOVE_STATIC_LIB_DIR
 	rm -rf $(TARGET_DIR)/usr/lib/s6-networking
 endef
 
 S6_NETWORKING_POST_INSTALL_TARGET_HOOKS += S6_NETWORKING_REMOVE_STATIC_LIB_DIR
 
-- 
2.17.1

[Buildroot] [PATCH 2/8] package/sdl_sound: actually use the optional CONF_OPTS

[Ricardo Martincoski]

Since "57ace26b6c package/sdl_sound: add optional support for
libmodplug" from 2016, optional CONF_OPTS are added but they do not
really take effect because there is an unconditional override below the
conditional append.

Currently this does not cause build failures, but it can lead to wrong
detection of dependencies because many explicit --enable/--disable are
not passed to configure.

Fix this by moving the unconditional code to the top.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
On current master (f6843a75fe), test-pkg with this config:
BR2_PACKAGE_SDL=y
BR2_PACKAGE_SDL_SOUND=y
results in:
6 builds, 0 skipped, 0 build failed, 0 legal-info failed
After this patch, the same result:
6 builds, 0 skipped, 0 build failed, 0 legal-info failed

After running test-pkg I diffed this file between the output of master
and the output after this patch:
br-arm-full/build/sdl_sound-1.0.3/config.status
Following flags are added, result of this patch:
'--disable-flac' '--disable-modplug' '--disable-ogg' '--disable-speex' '--disable-physfs'

On current master (f6843a75fe), test-pkg with this config:
BR2_PACKAGE_FLAC=y
BR2_PACKAGE_SDL=y
BR2_PACKAGE_SDL_SOUND=y
BR2_PACKAGE_SDL_SOUND_PLAYSOUND=y
BR2_PACKAGE_LIBMODPLUG=y
BR2_PACKAGE_LIBVORBIS=y
BR2_PACKAGE_SPEEX=y
BR2_PACKAGE_PHYSFS=y
results in:
6 builds, 0 skipped, 0 build failed, 0 legal-info failed
After this patch, the same result:
6 builds, 0 skipped, 0 build failed, 0 legal-info failed

After running test-pkg I diffed this file between the output of master
and the output after this patch:
br-arm-full/build/sdl_sound-1.0.3/config.status
Following flags are added, result of this patch:
'--enable-flac' '--enable-modplug' '--enable-ogg' '--enable-speex' '--enable-physfs'
---
 package/sdl_sound/sdl_sound.mk | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/package/sdl_sound/sdl_sound.mk b/package/sdl_sound/sdl_sound.mk
index 2544b598e9..c6028e7910 100644
--- a/package/sdl_sound/sdl_sound.mk
+++ b/package/sdl_sound/sdl_sound.mk
@@ -1,29 +1,34 @@
 ################################################################################
 #
 # sdl_sound
 #
 ################################################################################
 
 SDL_SOUND_VERSION = 1.0.3
 SDL_SOUND_SOURCE = SDL_sound-$(SDL_SOUND_VERSION).tar.gz
 SDL_SOUND_SITE = http://icculus.org/SDL_sound/downloads
 SDL_SOUND_LICENSE = LGPL-2.1+
 SDL_SOUND_LICENSE_FILES = COPYING
 SDL_SOUND_INSTALL_STAGING = YES
 SDL_SOUND_DEPENDENCIES = sdl
+SDL_SOUND_CONF_OPTS = \
+	--with-sdl-prefix=$(STAGING_DIR)/usr \
+	--with-sdl-exec-prefix=$(STAGING_DIR)/usr \
+	--disable-sdltest \
+	--enable-static
 
 ifneq ($(BR2_ENABLE_LOCALE),y)
 SDL_SOUND_DEPENDENCIES += libiconv
 endif
 
 # optional dependencies
 ifeq ($(BR2_PACKAGE_FLAC)$(BR2_PACKAGE_LIBOGG),yy)
 SDL_SOUND_CONF_OPTS += --enable-flac
 SDL_SOUND_DEPENDENCIES += flac libogg
 else
 SDL_SOUND_CONF_OPTS += --disable-flac
 endif
 
 ifeq ($(BR2_PACKAGE_LIBMODPLUG),y)
 SDL_SOUND_CONF_OPTS += --enable-modplug
 SDL_SOUND_DEPENDENCIES += libmodplug
@@ -39,37 +44,31 @@ SDL_SOUND_CONF_OPTS += --disable-ogg
 endif
 
 ifeq ($(BR2_PACKAGE_SPEEX),y)
 SDL_SOUND_CONF_OPTS += --enable-speex
 SDL_SOUND_DEPENDENCIES += speex
 else
 SDL_SOUND_CONF_OPTS += --disable-speex
 endif
 
 ifeq ($(BR2_PACKAGE_PHYSFS),y)
 SDL_SOUND_CONF_OPTS += --enable-physfs
 SDL_SOUND_DEPENDENCIES += physfs
 else
 SDL_SOUND_CONF_OPTS += --disable-physfs
 endif
 
-SDL_SOUND_CONF_OPTS = \
-	--with-sdl-prefix=$(STAGING_DIR)/usr \
-	--with-sdl-exec-prefix=$(STAGING_DIR)/usr \
-	--disable-sdltest \
-	--enable-static
-
 ifeq ($(BR2_X86_CPU_HAS_MMX),y)
 SDL_SOUND_CONF_OPTS += --enable-mmx
 else
 SDL_SOUND_CONF_OPTS += --disable-mmx
 endif
 
 define SDL_SOUND_REMOVE_PLAYSOUND
 	rm $(addprefix $(TARGET_DIR)/usr/bin/,playsound playsound_simple)
 endef
 
 ifneq ($(BR2_PACKAGE_SDL_SOUND_PLAYSOUND),y)
 SDL_SOUND_POST_INSTALL_TARGET_HOOKS += SDL_SOUND_REMOVE_PLAYSOUND
 endif
 
 $(eval $(autotools-package))
-- 
2.17.1

[Buildroot] [PATCH 3/8] Revert "avrdude: add license information"

[Ricardo Martincoski]

This reverts commit d1f545004bf0533064363d87c2d7c71e7acf7435 from 2014
because the added variables already existed. The real problem at the
time was that one of the pre-existent variables had a typo, fixed in a
later commit.

Currently AVRDUDE_LICENSE and AVRDUDE_LICENSE_FILES are declared twice
with the same values for each one. So remove one of them.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Wojciech M. Zabolotny <wzab01@gmail.com>
---
NOTE: When I first sent the series adding check-package, I reported the
typo. Unfortunately I failed to identify the duplicated variables.

The output for below command is the same in current master (f6843a75fe)
and after this patch:
$ make -s BR2_HAVE_DOT_CONFIG=y printvars VARS=AVRDUDE_LICENSE%
AVRDUDE_LICENSE=GPL-2.0+
AVRDUDE_LICENSE_FILES=avrdude/COPYING
---
 package/avrdude/avrdude.mk | 2 --
 1 file changed, 2 deletions(-)

diff --git a/package/avrdude/avrdude.mk b/package/avrdude/avrdude.mk
index 0e34076e38..ae378dfbac 100644
--- a/package/avrdude/avrdude.mk
+++ b/package/avrdude/avrdude.mk
@@ -2,34 +2,32 @@
 #
 # avrdude
 #
 ################################################################################
 
 AVRDUDE_VERSION = f8893ccf0aceeff67c45f95ae4e86bc7fbe342aa
 AVRDUDE_SITE = $(call github,kcuzner,avrdude,$(AVRDUDE_VERSION))
 AVRDUDE_LICENSE = GPL-2.0+
 AVRDUDE_LICENSE_FILES = avrdude/COPYING
 AVRDUDE_SUBDIR = avrdude
 # Sources coming from git, without generated configure and Makefile.in
 # files.
 AVRDUDE_AUTORECONF = YES
 AVRDUDE_CONF_OPTS = --enable-linuxgpio
 AVRDUDE_DEPENDENCIES = elfutils libusb libusb-compat ncurses \
 	host-flex host-bison
-AVRDUDE_LICENSE = GPL-2.0+
-AVRDUDE_LICENSE_FILES = avrdude/COPYING
 
 ifeq ($(BR2_PACKAGE_LIBFTDI1),y)
 AVRDUDE_DEPENDENCIES += libftdi1
 else ifeq ($(BR2_PACKAGE_LIBFTDI),y)
 AVRDUDE_DEPENDENCIES += libftdi
 endif
 
 # if /etc/avrdude.conf exists, the installation process creates a
 # backup file, which we do not want in the context of Buildroot.
 define AVRDUDE_REMOVE_BACKUP_FILE
 	$(RM) -f $(TARGET_DIR)/etc/avrdude.conf.bak
 endef
 
 AVRDUDE_POST_INSTALL_TARGET_HOOKS += AVRDUDE_REMOVE_BACKUP_FILE
 
 $(eval $(autotools-package))
-- 
2.17.1

[Buildroot] [PATCH 4/8] package/usb_modeswitch: drop unicode space in comment

[Ricardo Martincoski]

Commit "a554109af8 package/usb_modeswitch: disable parallel build" added
a unicode space in a comment. Replace it with a normal ASCII space for
consistency with elsewhere.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
Before this patch:
$ file package/usb_modeswitch/usb_modeswitch.mk
package/usb_modeswitch/usb_modeswitch.mk: UTF-8 Unicode text

After this patch:
$ file package/usb_modeswitch/usb_modeswitch.mk
package/usb_modeswitch/usb_modeswitch.mk: ASCII text
---
 package/usb_modeswitch/usb_modeswitch.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/usb_modeswitch/usb_modeswitch.mk b/package/usb_modeswitch/usb_modeswitch.mk
index bb0d6ab1bf..8b93e086e3 100644
--- a/package/usb_modeswitch/usb_modeswitch.mk
+++ b/package/usb_modeswitch/usb_modeswitch.mk
@@ -1,29 +1,29 @@
 ################################################################################
 #
 # usb_modeswitch
 #
 ################################################################################
 
 USB_MODESWITCH_VERSION = 2.5.2
 USB_MODESWITCH_SOURCE = usb-modeswitch-$(USB_MODESWITCH_VERSION).tar.bz2
 USB_MODESWITCH_SITE = http://www.draisberghof.de/usb_modeswitch
 USB_MODESWITCH_DEPENDENCIES = libusb
 USB_MODESWITCH_LICENSE = GPL-2.0+
 USB_MODESWITCH_LICENSE_FILES = COPYING
-#?Package does not build in parallel due to improper make rules
+# Package does not build in parallel due to improper make rules
 USB_MODESWITCH_MAKE = $(MAKE1)
 
 USB_MODESWITCH_BUILD_TARGETS = static
 USB_MODESWITCH_INSTALL_TARGETS = install-static
 
 ifeq ($(BR2_PACKAGE_TCL)$(BR2_PACKAGE_TCL_SHLIB_ONLY),y)
 USB_MODESWITCH_DEPENDENCIES += tcl
 USB_MODESWITCH_BUILD_TARGETS = script
 USB_MODESWITCH_INSTALL_TARGETS = install-script
 endif
 
 # build system of embedded jimtcl doesn't use autotools, but does use
 # an old version of gnuconfig which doesn't know all the architectures
 # supported by Buildroot, so update config.guess / config.sub like we
 # do in pkg-autotools.mk
 USB_MODESWITCH_POST_PATCH_HOOKS += UPDATE_CONFIG_HOOK
-- 
2.17.1

[Buildroot] [PATCH 5/8] package/usb_modeswitch: avoid overriding variables

[Ricardo Martincoski]

Overriding variables in packages recipes is an error-prone practice.

Current behavior of installing either only as a script or only as a
binary is intended, as describe in the commit log of "d3e4db4e34
usb_modeswitch: bump to version 1.2.6" from 2013.

Rewrite the code to keep the same behavior while replacing variable
override [1] by conditional assignments [2].

[1]
VAR = ...
if ...
VAR = ...

[2]
if ...
VAR = ...
else
VAR = ...

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
The output for below commands is the same in current master (f6843a75fe)
and after this patch:
$ make -s BR2_HAVE_DOT_CONFIG=y BR2_PACKAGE_TCL_SHLIB_ONLY=y printvars \
  VARS='USB_MODESWITCH_BUILD_TARGETS USB_MODESWITCH_INSTALL_TARGETS'
USB_MODESWITCH_BUILD_TARGETS=script
USB_MODESWITCH_INSTALL_TARGETS=install-script
$ make -s BR2_HAVE_DOT_CONFIG=y printvars \
  VARS='USB_MODESWITCH_BUILD_TARGETS USB_MODESWITCH_INSTALL_TARGETS'
USB_MODESWITCH_BUILD_TARGETS=static
USB_MODESWITCH_INSTALL_TARGETS=install-static
---
 package/usb_modeswitch/usb_modeswitch.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/package/usb_modeswitch/usb_modeswitch.mk b/package/usb_modeswitch/usb_modeswitch.mk
index 8b93e086e3..9832d3c808 100644
--- a/package/usb_modeswitch/usb_modeswitch.mk
+++ b/package/usb_modeswitch/usb_modeswitch.mk
@@ -1,38 +1,38 @@
 ################################################################################
 #
 # usb_modeswitch
 #
 ################################################################################
 
 USB_MODESWITCH_VERSION = 2.5.2
 USB_MODESWITCH_SOURCE = usb-modeswitch-$(USB_MODESWITCH_VERSION).tar.bz2
 USB_MODESWITCH_SITE = http://www.draisberghof.de/usb_modeswitch
 USB_MODESWITCH_DEPENDENCIES = libusb
 USB_MODESWITCH_LICENSE = GPL-2.0+
 USB_MODESWITCH_LICENSE_FILES = COPYING
 # Package does not build in parallel due to improper make rules
 USB_MODESWITCH_MAKE = $(MAKE1)
 
-USB_MODESWITCH_BUILD_TARGETS = static
-USB_MODESWITCH_INSTALL_TARGETS = install-static
-
 ifeq ($(BR2_PACKAGE_TCL)$(BR2_PACKAGE_TCL_SHLIB_ONLY),y)
 USB_MODESWITCH_DEPENDENCIES += tcl
 USB_MODESWITCH_BUILD_TARGETS = script
 USB_MODESWITCH_INSTALL_TARGETS = install-script
+else
+USB_MODESWITCH_BUILD_TARGETS = static
+USB_MODESWITCH_INSTALL_TARGETS = install-static
 endif
 
 # build system of embedded jimtcl doesn't use autotools, but does use
 # an old version of gnuconfig which doesn't know all the architectures
 # supported by Buildroot, so update config.guess / config.sub like we
 # do in pkg-autotools.mk
 USB_MODESWITCH_POST_PATCH_HOOKS += UPDATE_CONFIG_HOOK
 
 define USB_MODESWITCH_BUILD_CMDS
 	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
 		$(if $(BR2_INSTALL_LIBSTDCPP),,CXX=false) \
 		CFLAGS="$(TARGET_CFLAGS) -D_GNU_SOURCE -Wall -I." \
 		JIM_CONFIGURE_OPTS="--host=$(GNU_TARGET_NAME) --build=$(GNU_HOST_NAME)" \
 		-C $(@D) $(USB_MODESWITCH_BUILD_TARGETS)
 endef
 
-- 
2.17.1

[Buildroot] [PATCH 6/8] utils/check-package: allow to disable warning for a line

[Ricardo Martincoski]

Currently any exceptions for a check function need to be coded into the
check-package script itself.

Create a pattern that can be used in a comment to make check-package
ignore one or more warning types in the line immediately below:
 # check-package Indent, VariableWithBraces

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
 utils/check-package           | 4 ++++
 utils/checkpackagelib/base.py | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/utils/check-package b/utils/check-package
index 26439f08eb..ce1fe98d67 100755
--- a/utils/check-package
+++ b/utils/check-package
@@ -119,36 +119,40 @@ def check_file_using_lib(fname):
         return nwarnings, nlines
     classes = inspect.getmembers(lib, is_a_check_function)
 
     if flags.dry_run:
         functions_to_run = [c[0] for c in classes]
         print("{}: would run: {}".format(fname, functions_to_run))
         return nwarnings, nlines
 
     objects = [c[1](fname, flags.manual_url) for c in classes]
 
     for cf in objects:
         nwarnings += print_warnings(cf.before())
     if six.PY3:
         f = open(fname, "r", errors="surrogateescape")
     else:
         f = open(fname, "r")
+    lastline = ""
     for lineno, text in enumerate(f.readlines()):
         nlines += 1
         for cf in objects:
+            if cf.disable.search(lastline):
+                continue
             nwarnings += print_warnings(cf.check_line(lineno + 1, text))
+        lastline = text
     f.close()
     for cf in objects:
         nwarnings += print_warnings(cf.after())
 
     return nwarnings, nlines
 
 
 def __main__():
     global flags
     flags = parse_args()
 
     if flags.intree_only:
         # change all paths received to be relative to the base dir
         base_dir = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
         files_to_check = [os.path.relpath(os.path.abspath(f), base_dir) for f in flags.files]
         # move current dir so the script find the files
diff --git a/utils/checkpackagelib/base.py b/utils/checkpackagelib/base.py
index fc09bec9a2..9544a64e5a 100644
--- a/utils/checkpackagelib/base.py
+++ b/utils/checkpackagelib/base.py
@@ -1,16 +1,18 @@
 # See utils/checkpackagelib/readme.txt before editing this file.
+import re
 
 
 class _CheckFunction(object):
     def __init__(self, filename, url_to_manual):
         self.filename = filename
         self.url_to_manual = url_to_manual
+        self.disable = re.compile(r"^\s*# check-package .*\b{}\b".format(self.__class__.__name__))
 
     def before(self):
         pass
 
     def check_line(self, lineno, text):
         pass
 
     def after(self):
         pass
-- 
2.17.1

[Buildroot] [PATCH 7/8] utils/check-package: handle ifdef/ifndef in .mk files

[Ricardo Martincoski]

Currently check-package only knows about ifeq/ifneq.
Add code to handle ifdef/ifndef as well.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
 utils/checkpackagelib/lib_mk.py | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py
index 4387cf7167..51c6577d2c 100644
--- a/utils/checkpackagelib/lib_mk.py
+++ b/utils/checkpackagelib/lib_mk.py
@@ -1,34 +1,38 @@
 # See utils/checkpackagelib/readme.txt before editing this file.
 # There are already dependency checks during the build, so below check
 # functions don't need to check for things already checked by exploring the
 # menu options using "make menuconfig" and by running "make" with appropriate
 # packages enabled.
 
 import re
 
 from checkpackagelib.base import _CheckFunction
 from checkpackagelib.lib import ConsecutiveEmptyLines  # noqa: F401
 from checkpackagelib.lib import EmptyLastLine          # noqa: F401
 from checkpackagelib.lib import NewlineAtEof           # noqa: F401
 from checkpackagelib.lib import TrailingSpace          # noqa: F401
 
+# used in more than one check
+start_conditional = ["ifdef", "ifeq", "ifndef", "ifneq"]
+end_conditional = ["endif"]
+
 
 class Indent(_CheckFunction):
     COMMENT = re.compile("^\s*#")
-    CONDITIONAL = re.compile("^\s*(ifeq|ifneq|endif)\s")
+    CONDITIONAL = re.compile("^\s*({})\s".format("|".join(start_conditional + end_conditional)))
     ENDS_WITH_BACKSLASH = re.compile(r"^[^#].*\\$")
     END_DEFINE = re.compile("^\s*endef\s")
     MAKEFILE_TARGET = re.compile("^[^# \t]+:\s")
     START_DEFINE = re.compile("^\s*define\s")
 
     def before(self):
         self.define = False
         self.backslash = False
         self.makefile_target = False
 
     def check_line(self, lineno, text):
         if self.START_DEFINE.search(text):
             self.define = True
             return
         if self.END_DEFINE.search(text):
             self.define = False
@@ -209,34 +213,34 @@ class TypoInPackageVariable(_CheckFunction):
             return
         if self.REGEX.search(text) is None:
             return ["{}:{}: possible typo: {} -> *{}*"
                     .format(self.filename, lineno, variable, self.package),
                     text]
 
 
 class UselessFlag(_CheckFunction):
     DEFAULT_AUTOTOOLS_FLAG = re.compile("^.*{}".format("|".join([
         "_AUTORECONF\s*=\s*NO",
         "_LIBTOOL_PATCH\s*=\s*YES"])))
     DEFAULT_GENERIC_FLAG = re.compile("^.*{}".format("|".join([
         "_INSTALL_IMAGES\s*=\s*NO",
         "_INSTALL_REDISTRIBUTE\s*=\s*YES",
         "_INSTALL_STAGING\s*=\s*NO",
         "_INSTALL_TARGET\s*=\s*YES"])))
-    END_CONDITIONAL = re.compile("^\s*(endif)")
-    START_CONDITIONAL = re.compile("^\s*(ifeq|ifneq)")
+    END_CONDITIONAL = re.compile("^\s*({})".format("|".join(end_conditional)))
+    START_CONDITIONAL = re.compile("^\s*({})".format("|".join(start_conditional)))
 
     def before(self):
         self.conditional = 0
 
     def check_line(self, lineno, text):
         if self.START_CONDITIONAL.search(text):
             self.conditional += 1
             return
         if self.END_CONDITIONAL.search(text):
             self.conditional -= 1
             return
 
         # allow non-default conditionally overridden by default
         if self.conditional > 0:
             return
 
-- 
2.17.1

[Buildroot] [PATCH 8/8] utils/check-package: warn about overridden variables

[Ricardo Martincoski]

For the general case, appending values to variables is OK and also a
good practice, like this:
|PACKAGE_VAR = value1
|ifeq ...
|PACKAGE_VAR += value2

or this, when the above is not possible:
|PACKAGE_VAR = value1
|ifeq ...
|PACKAGE_VAR := $(PACKAGE_VAR), value2

But this override is an error:
|PACKAGE_VAR = value1
|PACKAGE_VAR = value2

as well this one:
|ifeq ...
|PACKAGE_VAR += value1
|endif
|PACKAGE_VAR = value2

And this override is error-prone:
|PACKAGE_VAR = value1
|ifeq ...
|PACKAGE_VAR = value2

Create a check function to warn about overridden variables.

Some variables are likely to have a default value that gets overridden
in a conditional, so ignore them. The name of such variables end in
_ARCH, _CPU, _SITE, _SOURCE or _VERSION.

After ignoring these variable names, there are a few exceptions to this
rule in the tree. For them use the comment that disables the check.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Simon Dawson <spdawson@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 package/gcc/gcc-final/gcc-final.mk |  2 +
 package/zmqpp/zmqpp.mk             |  1 +
 utils/checkpackagelib/lib_mk.py    | 60 ++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)

diff --git a/package/gcc/gcc-final/gcc-final.mk b/package/gcc/gcc-final/gcc-final.mk
index 37b645d252..49f16f699e 100644
--- a/package/gcc/gcc-final/gcc-final.mk
+++ b/package/gcc/gcc-final/gcc-final.mk
@@ -55,36 +55,38 @@ endef
 # Languages supported by the cross-compiler
 GCC_FINAL_CROSS_LANGUAGES-y = c
 GCC_FINAL_CROSS_LANGUAGES-$(BR2_INSTALL_LIBSTDCPP) += c++
 GCC_FINAL_CROSS_LANGUAGES-$(BR2_TOOLCHAIN_BUILDROOT_FORTRAN) += fortran
 GCC_FINAL_CROSS_LANGUAGES = $(subst $(space),$(comma),$(GCC_FINAL_CROSS_LANGUAGES-y))
 
 HOST_GCC_FINAL_CONF_OPTS = \
 	$(HOST_GCC_COMMON_CONF_OPTS) \
 	--enable-languages=$(GCC_FINAL_CROSS_LANGUAGES) \
 	--with-build-time-tools=$(HOST_DIR)/$(GNU_TARGET_NAME)/bin
 
 HOST_GCC_FINAL_GCC_LIB_DIR = $(HOST_DIR)/$(GNU_TARGET_NAME)/lib*
 # The kernel wants to use the -m4-nofpu option to make sure that it
 # doesn't use floating point operations.
 ifeq ($(BR2_sh4)$(BR2_sh4eb),y)
 HOST_GCC_FINAL_CONF_OPTS += "--with-multilib-list=m4,m4-nofpu"
+# check-package OverriddenVariable
 HOST_GCC_FINAL_GCC_LIB_DIR = $(HOST_DIR)/$(GNU_TARGET_NAME)/lib/!m4*
 endif
 ifeq ($(BR2_sh4a)$(BR2_sh4aeb),y)
 HOST_GCC_FINAL_CONF_OPTS += "--with-multilib-list=m4a,m4a-nofpu"
+# check-package OverriddenVariable
 HOST_GCC_FINAL_GCC_LIB_DIR = $(HOST_DIR)/$(GNU_TARGET_NAME)/lib/!m4*
 endif
 
 ifeq ($(BR2_GCC_SUPPORTS_LIBCILKRTS),y)
 
 # libcilkrts does not support v8
 ifeq ($(BR2_sparc),y)
 HOST_GCC_FINAL_CONF_OPTS += --disable-libcilkrts
 endif
 
 # Pthreads are required to build libcilkrts
 ifeq ($(BR2_PTHREADS_NONE),y)
 HOST_GCC_FINAL_CONF_OPTS += --disable-libcilkrts
 endif
 
 ifeq ($(BR2_STATIC_LIBS),y)
diff --git a/package/zmqpp/zmqpp.mk b/package/zmqpp/zmqpp.mk
index 801766a7d8..ea6b50e826 100644
--- a/package/zmqpp/zmqpp.mk
+++ b/package/zmqpp/zmqpp.mk
@@ -6,32 +6,33 @@
 
 ZMQPP_VERSION = 4.2.0
 ZMQPP_SITE = $(call github,zeromq,zmqpp,$(ZMQPP_VERSION))
 ZMQPP_INSTALL_STAGING = YES
 ZMQPP_DEPENDENCIES = zeromq
 ZMQPP_LICENSE = MPL-2.0
 ZMQPP_LICENSE_FILES = LICENSE
 ZMQPP_MAKE_OPTS = LD="$(TARGET_CXX)" BUILD_PATH=./build PREFIX=/usr
 ZMQPP_LDFLAGS = $(TARGET_LDFLAGS) -lpthread
 ZMQPP_CONFIG = $(if $(BR2_ENABLE_DEBUG),debug,release)
 
 # gcc bug internal compiler error: in merge_overlapping_regs, at
 # regrename.c:304. This bug is fixed since gcc 6.
 # By setting CONFIG to empty, all optimizations such as -funroll-loops
 # -ffast-math -finline-functions -fomit-frame-pointer are disabled
 ifeq ($(BR2_or1k):$(BR2_TOOLCHAIN_GCC_AT_LEAST_6),y:)
+# check-package OverriddenVariable
 ZMQPP_CONFIG =
 endif
 
 ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y)
 ZMQPP_LDFLAGS += -latomic
 endif
 
 ifeq ($(BR2_PACKAGE_ZMQPP_CLIENT),y)
 ZMQPP_DEPENDENCIES += boost
 endif
 
 ifeq ($(BR2_STATIC_LIBS),y)
 ZMQPP_MAKE_OPTS += BUILD_STATIC=yes BUILD_SHARED=no
 else ifeq ($(BR2_SHARED_STATIC_LIBS),y)
 ZMQPP_MAKE_OPTS += BUILD_STATIC=yes BUILD_SHARED=yes
 else ifeq ($(BR2_SHARED_LIBS),y)
diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py
index 51c6577d2c..00efeb7fb2 100644
--- a/utils/checkpackagelib/lib_mk.py
+++ b/utils/checkpackagelib/lib_mk.py
@@ -60,32 +60,92 @@ class Indent(_CheckFunction):
         # comment can be indented or not inside define ... endef, so ignore it
         if self.define and self.COMMENT.search(text):
             return
 
         if expect_tabs:
             if not text.startswith("\t"):
                 return ["{}:{}: expected indent with tabs"
                         .format(self.filename, lineno),
                         text]
         else:
             if text.startswith("\t"):
                 return ["{}:{}: unexpected indent with tabs"
                         .format(self.filename, lineno),
                         text]
 
 
+class OverriddenVariable(_CheckFunction):
+    CONCATENATING = re.compile("^([A-Z0-9_]+)\s*(\+|:|)=\s*\$\(\\1\)")
+    END_CONDITIONAL = re.compile("^\s*({})".format("|".join(end_conditional)))
+    OVERRIDING_ASSIGNMENTS = [':=', "="]
+    START_CONDITIONAL = re.compile("^\s*({})".format("|".join(start_conditional)))
+    VARIABLE = re.compile("^([A-Z0-9_]+)\s*((\+|:|)=)")
+    USUALLY_OVERRIDDEN = re.compile("^[A-Z0-9_]+({})".format("|".join([
+        "_ARCH\s*=\s*",
+        "_CPU\s*=\s*",
+        "_SITE\s*=\s*",
+        "_SOURCE\s*=\s*",
+        "_VERSION\s*=\s*"])))
+
+    def before(self):
+        self.conditional = 0
+        self.unconditionally_set = []
+        self.conditionally_set = []
+
+    def check_line(self, lineno, text):
+        if self.START_CONDITIONAL.search(text):
+            self.conditional += 1
+            return
+        if self.END_CONDITIONAL.search(text):
+            self.conditional -= 1
+            return
+
+        m = self.VARIABLE.search(text)
+        if m is None:
+            return
+        variable, assignment = m.group(1, 2)
+
+        if self.conditional == 0:
+            if variable in self.conditionally_set:
+                self.unconditionally_set.append(variable)
+                if assignment in self.OVERRIDING_ASSIGNMENTS:
+                    return ["{}:{}: unconditional override of variable {} previously conditionally set"
+                            .format(self.filename, lineno, variable),
+                            text]
+
+            if variable not in self.unconditionally_set:
+                self.unconditionally_set.append(variable)
+                return
+            if assignment in self.OVERRIDING_ASSIGNMENTS:
+                return ["{}:{}: unconditional override of variable {}"
+                        .format(self.filename, lineno, variable),
+                        text]
+        else:
+            if variable not in self.unconditionally_set:
+                self.conditionally_set.append(variable)
+                return
+            if self.CONCATENATING.search(text):
+                return
+            if self.USUALLY_OVERRIDDEN.search(text):
+                return
+            if assignment in self.OVERRIDING_ASSIGNMENTS:
+                return ["{}:{}: conditional override of variable {}"
+                        .format(self.filename, lineno, variable),
+                        text]
+
+
 class PackageHeader(_CheckFunction):
     def before(self):
         self.skip = False
 
     def check_line(self, lineno, text):
         if self.skip or lineno > 6:
             return
 
         if lineno in [1, 5]:
             if lineno == 1 and text.startswith("include "):
                 self.skip = True
                 return
             if text.rstrip() != "#" * 80:
                 return ["{}:{}: should be 80 hashes ({}#writing-rules-mk)"
                         .format(self.filename, lineno, self.url_to_manual),
                         text,
-- 
2.17.1

[Buildroot] [PATCH 1/8] package/s6-networking: fix dependency when libressl is enabled

[Thomas Petazzoni]

Hello Ricardo,

On Sun, 27 Jan 2019 16:59:36 -0200
Ricardo Martincoski <ricardo.martincoski@gmail.com> wrote:

> Commit "c5b85231fb s6-networking: enable SSL if libressl is selected"
> actually dropped the dependency on s6-dns and s6 when libressl is
> enabled.
> Fix this by using += inside the conditional code.
> 
> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Cc: Eric Le Bihan <eric.le.bihan.dev@free.fr>
> ---
> NOTE: I don't know why the autobuilders didn't caught this.

Because libressl is selected as part of a choice..endchoice, and such
choices are not randomized by "randpackageconfig". Therefore, only
OpenSSL is tested by the autobuilders, and LibreSSL is never tested.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/8] package/s6-networking: fix dependency when libressl is enabled

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > Commit "c5b85231fb s6-networking: enable SSL if libressl is selected"
 > actually dropped the dependency on s6-dns and s6 when libressl is
 > enabled.
 > Fix this by using += inside the conditional code.

 > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
 > Cc: Eric Le Bihan <eric.le.bihan.dev@free.fr>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 2/8] package/sdl_sound: actually use the optional CONF_OPTS

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > Since "57ace26b6c package/sdl_sound: add optional support for
 > libmodplug" from 2016, optional CONF_OPTS are added but they do not
 > really take effect because there is an unconditional override below the
 > conditional append.

 > Currently this does not cause build failures, but it can lead to wrong
 > detection of dependencies because many explicit --enable/--disable are
 > not passed to configure.

 > Fix this by moving the unconditional code to the top.

 > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 3/8] Revert "avrdude: add license information"

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > This reverts commit d1f545004bf0533064363d87c2d7c71e7acf7435 from 2014
 > because the added variables already existed. The real problem at the
 > time was that one of the pre-existent variables had a typo, fixed in a
 > later commit.

 > Currently AVRDUDE_LICENSE and AVRDUDE_LICENSE_FILES are declared twice
 > with the same values for each one. So remove one of them.

 > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
 > Cc: Wojciech M. Zabolotny <wzab01@gmail.com>
 > ---
 > NOTE: When I first sent the series adding check-package, I reported the
 > typo. Unfortunately I failed to identify the duplicated variables.

 > The output for below command is the same in current master (f6843a75fe)
 > and after this patch:
 > $ make -s BR2_HAVE_DOT_CONFIG=y printvars VARS=AVRDUDE_LICENSE%
 > AVRDUDE_LICENSE=GPL-2.0+
 > AVRDUDE_LICENSE_FILES=avrdude/COPYING

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 4/8] package/usb_modeswitch: drop unicode space in comment

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > Commit "a554109af8 package/usb_modeswitch: disable parallel build" added
 > a unicode space in a comment. Replace it with a normal ASCII space for
 > consistency with elsewhere.

 > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 5/8] package/usb_modeswitch: avoid overriding variables

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > Overriding variables in packages recipes is an error-prone practice.
 > Current behavior of installing either only as a script or only as a
 > binary is intended, as describe in the commit log of "d3e4db4e34
 > usb_modeswitch: bump to version 1.2.6" from 2013.

 > Rewrite the code to keep the same behavior while replacing variable
 > override [1] by conditional assignments [2].

 > [1]
 > VAR = ...
 > if ...
 > VAR = ...

 > [2]
 > if ...
 > VAR = ...
 > else
 > VAR = ...

 > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/8] package/s6-networking: fix dependency when libressl is enabled

[Ricardo Martincoski]

Hello,

On Sun, Jan 27, 2019 at 05:52 PM, Thomas Petazzoni wrote:

> On Sun, 27 Jan 2019 16:59:36 -0200
> Ricardo Martincoski <ricardo.martincoski@gmail.com> wrote:
> 
>> Commit "c5b85231fb s6-networking: enable SSL if libressl is selected"
>> actually dropped the dependency on s6-dns and s6 when libressl is
>> enabled.
>> Fix this by using += inside the conditional code.
>> 
>> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
>> Cc: Eric Le Bihan <eric.le.bihan.dev@free.fr>
>> ---
>> NOTE: I don't know why the autobuilders didn't caught this.
> 
> Because libressl is selected as part of a choice..endchoice, and such
> choices are not randomized by "randpackageconfig". Therefore, only
> OpenSSL is tested by the autobuilders, and LibreSSL is never tested.

Right. Thank you for the clarification.


Regards,
Ricardo

[Buildroot] [PATCH 7/8] utils/check-package: handle ifdef/ifndef in .mk files

[Arnout Vandecappelle]

On 27/01/2019 19:59, Ricardo Martincoski wrote:
> Currently check-package only knows about ifeq/ifneq.
> Add code to handle ifdef/ifndef as well.
> 
> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

 Regards,
 Arnout

[Buildroot] [PATCH 6/8] utils/check-package: allow to disable warning for a line

[Arnout Vandecappelle]

On 27/01/2019 19:59, Ricardo Martincoski wrote:
> Currently any exceptions for a check function need to be coded into the
> check-package script itself.
> 
> Create a pattern that can be used in a comment to make check-package
> ignore one or more warning types in the line immediately below:
>  # check-package Indent, VariableWithBraces
> 
> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

 It would be nice to have mentioned that an in-tree user of this feature will be
added in a later patch.

Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

 Regards,
 Arnout

> ---
>  utils/check-package           | 4 ++++
>  utils/checkpackagelib/base.py | 2 ++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/utils/check-package b/utils/check-package
> index 26439f08eb..ce1fe98d67 100755
> --- a/utils/check-package
> +++ b/utils/check-package
> @@ -119,36 +119,40 @@ def check_file_using_lib(fname):
>          return nwarnings, nlines
>      classes = inspect.getmembers(lib, is_a_check_function)
>  
>      if flags.dry_run:
>          functions_to_run = [c[0] for c in classes]
>          print("{}: would run: {}".format(fname, functions_to_run))
>          return nwarnings, nlines
>  
>      objects = [c[1](fname, flags.manual_url) for c in classes]
>  
>      for cf in objects:
>          nwarnings += print_warnings(cf.before())
>      if six.PY3:
>          f = open(fname, "r", errors="surrogateescape")
>      else:
>          f = open(fname, "r")
> +    lastline = ""
>      for lineno, text in enumerate(f.readlines()):
>          nlines += 1
>          for cf in objects:
> +            if cf.disable.search(lastline):
> +                continue
>              nwarnings += print_warnings(cf.check_line(lineno + 1, text))
> +        lastline = text
>      f.close()
>      for cf in objects:
>          nwarnings += print_warnings(cf.after())
>  
>      return nwarnings, nlines
>  
>  
>  def __main__():
>      global flags
>      flags = parse_args()
>  
>      if flags.intree_only:
>          # change all paths received to be relative to the base dir
>          base_dir = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
>          files_to_check = [os.path.relpath(os.path.abspath(f), base_dir) for f in flags.files]
>          # move current dir so the script find the files
> diff --git a/utils/checkpackagelib/base.py b/utils/checkpackagelib/base.py
> index fc09bec9a2..9544a64e5a 100644
> --- a/utils/checkpackagelib/base.py
> +++ b/utils/checkpackagelib/base.py
> @@ -1,16 +1,18 @@
>  # See utils/checkpackagelib/readme.txt before editing this file.
> +import re
>  
>  
>  class _CheckFunction(object):
>      def __init__(self, filename, url_to_manual):
>          self.filename = filename
>          self.url_to_manual = url_to_manual
> +        self.disable = re.compile(r"^\s*# check-package .*\b{}\b".format(self.__class__.__name__))
>  
>      def before(self):
>          pass
>  
>      def check_line(self, lineno, text):
>          pass
>  
>      def after(self):
>          pass
> 

[Buildroot] [PATCH 6/8] utils/check-package: allow to disable warning for a line

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > Currently any exceptions for a check function need to be coded into the
 > check-package script itself.

 > Create a pattern that can be used in a comment to make check-package
 > ignore one or more warning types in the line immediately below:
 >  # check-package Indent, VariableWithBraces

 > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 7/8] utils/check-package: handle ifdef/ifndef in .mk files

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > Currently check-package only knows about ifeq/ifneq.
 > Add code to handle ifdef/ifndef as well.

 > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/8] package/s6-networking: fix dependency when libressl is enabled

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > Commit "c5b85231fb s6-networking: enable SSL if libressl is selected"
 > actually dropped the dependency on s6-dns and s6 when libressl is
 > enabled.
 > Fix this by using += inside the conditional code.

 > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
 > Cc: Eric Le Bihan <eric.le.bihan.dev@free.fr>

Committed to 2018.02.x and 2018.11.x, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 2/8] package/sdl_sound: actually use the optional CONF_OPTS

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > Since "57ace26b6c package/sdl_sound: add optional support for
 > libmodplug" from 2016, optional CONF_OPTS are added but they do not
 > really take effect because there is an unconditional override below the
 > conditional append.

 > Currently this does not cause build failures, but it can lead to wrong
 > detection of dependencies because many explicit --enable/--disable are
 > not passed to configure.

 > Fix this by moving the unconditional code to the top.

 > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

Committed to 2018.02.x and 2018.11.x, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 3/8] Revert "avrdude: add license information"

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > This reverts commit d1f545004bf0533064363d87c2d7c71e7acf7435 from 2014
 > because the added variables already existed. The real problem at the
 > time was that one of the pre-existent variables had a typo, fixed in a
 > later commit.

 > Currently AVRDUDE_LICENSE and AVRDUDE_LICENSE_FILES are declared twice
 > with the same values for each one. So remove one of them.

 > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
 > Cc: Wojciech M. Zabolotny <wzab01@gmail.com>

Committed to 2018.02.x and 2018.11.x, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 4/8] package/usb_modeswitch: drop unicode space in comment

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > Commit "a554109af8 package/usb_modeswitch: disable parallel build" added
 > a unicode space in a comment. Replace it with a normal ASCII space for
 > consistency with elsewhere.

 > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

Committed to 2018.02.x and 2018.11.x, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 5/8] package/usb_modeswitch: avoid overriding variables

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > Overriding variables in packages recipes is an error-prone practice.
 > Current behavior of installing either only as a script or only as a
 > binary is intended, as describe in the commit log of "d3e4db4e34
 > usb_modeswitch: bump to version 1.2.6" from 2013.

 > Rewrite the code to keep the same behavior while replacing variable
 > override [1] by conditional assignments [2].

 > [1]
 > VAR = ...
 > if ...
 > VAR = ...

 > [2]
 > if ...
 > VAR = ...
 > else
 > VAR = ...

 > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

Committed to 2018.02.x and 2018.11.x, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 8/8] utils/check-package: warn about overridden variables

[Titouan Christophe]

Hi Ricardo and all,

On Sun, 2019-01-27 at 16:59 -0200, Ricardo Martincoski wrote:
> For the general case, appending values to variables is OK and also a
> good practice, like this:
> > PACKAGE_VAR = value1
> > ifeq ...
> > PACKAGE_VAR += value2
> 
> or this, when the above is not possible:
> > PACKAGE_VAR = value1
> > ifeq ...
> > PACKAGE_VAR := $(PACKAGE_VAR), value2
> 
> But this override is an error:
> > PACKAGE_VAR = value1
> > PACKAGE_VAR = value2
> 
> as well this one:
> > ifeq ...
> > PACKAGE_VAR += value1
> > endif
> > PACKAGE_VAR = value2
> 
> And this override is error-prone:
> > PACKAGE_VAR = value1
> > ifeq ...
> > PACKAGE_VAR = value2
> 
> Create a check function to warn about overridden variables.
> 
> Some variables are likely to have a default value that gets
> overridden
> in a conditional, so ignore them. The name of such variables end in
> _ARCH, _CPU, _SITE, _SOURCE or _VERSION.
> 
> After ignoring these variable names, there are a few exceptions to
> this
> rule in the tree. For them use the comment that disables the check.
> 
> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

Tested-by: Titouan Christophe <titouan.christophe@railnova.eu>
[Test: adding an offending line in mosquitto.mk to trigger warning]

> Cc: Simon Dawson <spdawson@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
>  package/gcc/gcc-final/gcc-final.mk |  2 +
>  package/zmqpp/zmqpp.mk             |  1 +
>  utils/checkpackagelib/lib_mk.py    | 60
> ++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+)
> 
> diff --git a/package/gcc/gcc-final/gcc-final.mk b/package/gcc/gcc-
> final/gcc-final.mk
> index 37b645d252..49f16f699e 100644
> --- a/package/gcc/gcc-final/gcc-final.mk
> +++ b/package/gcc/gcc-final/gcc-final.mk
> @@ -55,36 +55,38 @@ endef
>  # Languages supported by the cross-compiler
>  GCC_FINAL_CROSS_LANGUAGES-y = c
>  GCC_FINAL_CROSS_LANGUAGES-$(BR2_INSTALL_LIBSTDCPP) += c++
>  GCC_FINAL_CROSS_LANGUAGES-$(BR2_TOOLCHAIN_BUILDROOT_FORTRAN) +=
> fortran
>  GCC_FINAL_CROSS_LANGUAGES = $(subst
> $(space),$(comma),$(GCC_FINAL_CROSS_LANGUAGES-y))
>  
>  HOST_GCC_FINAL_CONF_OPTS = \
>  	$(HOST_GCC_COMMON_CONF_OPTS) \
>  	--enable-languages=$(GCC_FINAL_CROSS_LANGUAGES) \
>  	--with-build-time-tools=$(HOST_DIR)/$(GNU_TARGET_NAME)/bin
>  
>  HOST_GCC_FINAL_GCC_LIB_DIR = $(HOST_DIR)/$(GNU_TARGET_NAME)/lib*
>  # The kernel wants to use the -m4-nofpu option to make sure that it
>  # doesn't use floating point operations.
>  ifeq ($(BR2_sh4)$(BR2_sh4eb),y)
>  HOST_GCC_FINAL_CONF_OPTS += "--with-multilib-list=m4,m4-nofpu"
> +# check-package OverriddenVariable
>  HOST_GCC_FINAL_GCC_LIB_DIR = $(HOST_DIR)/$(GNU_TARGET_NAME)/lib/!m4*
>  endif
>  ifeq ($(BR2_sh4a)$(BR2_sh4aeb),y)
>  HOST_GCC_FINAL_CONF_OPTS += "--with-multilib-list=m4a,m4a-nofpu"
> +# check-package OverriddenVariable
>  HOST_GCC_FINAL_GCC_LIB_DIR = $(HOST_DIR)/$(GNU_TARGET_NAME)/lib/!m4*
>  endif
>  
>  ifeq ($(BR2_GCC_SUPPORTS_LIBCILKRTS),y)
>  
>  # libcilkrts does not support v8
>  ifeq ($(BR2_sparc),y)
>  HOST_GCC_FINAL_CONF_OPTS += --disable-libcilkrts
>  endif
>  
>  # Pthreads are required to build libcilkrts
>  ifeq ($(BR2_PTHREADS_NONE),y)
>  HOST_GCC_FINAL_CONF_OPTS += --disable-libcilkrts
>  endif
>  
>  ifeq ($(BR2_STATIC_LIBS),y)
> diff --git a/package/zmqpp/zmqpp.mk b/package/zmqpp/zmqpp.mk
> index 801766a7d8..ea6b50e826 100644
> --- a/package/zmqpp/zmqpp.mk
> +++ b/package/zmqpp/zmqpp.mk
> @@ -6,32 +6,33 @@
>  
>  ZMQPP_VERSION = 4.2.0
>  ZMQPP_SITE = $(call github,zeromq,zmqpp,$(ZMQPP_VERSION))
>  ZMQPP_INSTALL_STAGING = YES
>  ZMQPP_DEPENDENCIES = zeromq
>  ZMQPP_LICENSE = MPL-2.0
>  ZMQPP_LICENSE_FILES = LICENSE
>  ZMQPP_MAKE_OPTS = LD="$(TARGET_CXX)" BUILD_PATH=./build PREFIX=/usr
>  ZMQPP_LDFLAGS = $(TARGET_LDFLAGS) -lpthread
>  ZMQPP_CONFIG = $(if $(BR2_ENABLE_DEBUG),debug,release)
>  
>  # gcc bug internal compiler error: in merge_overlapping_regs, at
>  # regrename.c:304. This bug is fixed since gcc 6.
>  # By setting CONFIG to empty, all optimizations such as -funroll-
> loops
>  # -ffast-math -finline-functions -fomit-frame-pointer are disabled
>  ifeq ($(BR2_or1k):$(BR2_TOOLCHAIN_GCC_AT_LEAST_6),y:)
> +# check-package OverriddenVariable
>  ZMQPP_CONFIG =
>  endif
>  
>  ifeq ($(BR2_TOOLCHAIN_HAS_LIBATOMIC),y)
>  ZMQPP_LDFLAGS += -latomic
>  endif
>  
>  ifeq ($(BR2_PACKAGE_ZMQPP_CLIENT),y)
>  ZMQPP_DEPENDENCIES += boost
>  endif
>  
>  ifeq ($(BR2_STATIC_LIBS),y)
>  ZMQPP_MAKE_OPTS += BUILD_STATIC=yes BUILD_SHARED=no
>  else ifeq ($(BR2_SHARED_STATIC_LIBS),y)
>  ZMQPP_MAKE_OPTS += BUILD_STATIC=yes BUILD_SHARED=yes
>  else ifeq ($(BR2_SHARED_LIBS),y)
> diff --git a/utils/checkpackagelib/lib_mk.py
> b/utils/checkpackagelib/lib_mk.py
> index 51c6577d2c..00efeb7fb2 100644
> --- a/utils/checkpackagelib/lib_mk.py
> +++ b/utils/checkpackagelib/lib_mk.py
> @@ -60,32 +60,92 @@ class Indent(_CheckFunction):
>          # comment can be indented or not inside define ... endef, so
> ignore it
>          if self.define and self.COMMENT.search(text):
>              return
>  
>          if expect_tabs:
>              if not text.startswith("\t"):
>                  return ["{}:{}: expected indent with tabs"
>                          .format(self.filename, lineno),
>                          text]
>          else:
>              if text.startswith("\t"):
>                  return ["{}:{}: unexpected indent with tabs"
>                          .format(self.filename, lineno),
>                          text]
>  
>  
> +class OverriddenVariable(_CheckFunction):
> +    CONCATENATING = re.compile("^([A-Z0-
> 9_]+)\s*(\+|:|)=\s*\$\(\\1\)")
> +    END_CONDITIONAL =
> re.compile("^\s*({})".format("|".join(end_conditional)))
> +    OVERRIDING_ASSIGNMENTS = [':=', "="]
> +    START_CONDITIONAL =
> re.compile("^\s*({})".format("|".join(start_conditional)))
> +    VARIABLE = re.compile("^([A-Z0-9_]+)\s*((\+|:|)=)")
> +    USUALLY_OVERRIDDEN = re.compile("^[A-Z0-
> 9_]+({})".format("|".join([
> +        "_ARCH\s*=\s*",
> +        "_CPU\s*=\s*",
> +        "_SITE\s*=\s*",
> +        "_SOURCE\s*=\s*",
> +        "_VERSION\s*=\s*"])))
> +
> +    def before(self):
> +        self.conditional = 0
> +        self.unconditionally_set = []
> +        self.conditionally_set = []
> +
> +    def check_line(self, lineno, text):
> +        if self.START_CONDITIONAL.search(text):
> +            self.conditional += 1
> +            return
> +        if self.END_CONDITIONAL.search(text):
> +            self.conditional -= 1
> +            return
> +
> +        m = self.VARIABLE.search(text)
> +        if m is None:
> +            return
> +        variable, assignment = m.group(1, 2)
> +
> +        if self.conditional == 0:
> +            if variable in self.conditionally_set:
> +                self.unconditionally_set.append(variable)
> +                if assignment in self.OVERRIDING_ASSIGNMENTS:
> +                    return ["{}:{}: unconditional override of
> variable {} previously conditionally set"
> +                            .format(self.filename, lineno,
> variable),
> +                            text]
> +
> +            if variable not in self.unconditionally_set:
> +                self.unconditionally_set.append(variable)
> +                return
> +            if assignment in self.OVERRIDING_ASSIGNMENTS:
> +                return ["{}:{}: unconditional override of variable
> {}"
> +                        .format(self.filename, lineno, variable),
> +                        text]
> +        else:
> +            if variable not in self.unconditionally_set:
> +                self.conditionally_set.append(variable)
> +                return
> +            if self.CONCATENATING.search(text):
> +                return
> +            if self.USUALLY_OVERRIDDEN.search(text):
> +                return
> +            if assignment in self.OVERRIDING_ASSIGNMENTS:
> +                return ["{}:{}: conditional override of variable {}"
> +                        .format(self.filename, lineno, variable),
> +                        text]
> +
> +
>  class PackageHeader(_CheckFunction):
>      def before(self):
>          self.skip = False
>  
>      def check_line(self, lineno, text):
>          if self.skip or lineno > 6:
>              return
>  
>          if lineno in [1, 5]:
>              if lineno == 1 and text.startswith("include "):
>                  self.skip = True
>                  return
>              if text.rstrip() != "#" * 80:
>                  return ["{}:{}: should be 80 hashes ({}#writing-
> rules-mk)"
>                          .format(self.filename, lineno,
> self.url_to_manual),
>                          text,


Thanks !

Titouan

[Buildroot] [PATCH 8/8] utils/check-package: warn about overridden variables

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > For the general case, appending values to variables is OK and also a
 > good practice, like this:
 > |PACKAGE_VAR = value1
 > |ifeq ...
 > |PACKAGE_VAR += value2

 > or this, when the above is not possible:
 > |PACKAGE_VAR = value1
 > |ifeq ...
 > |PACKAGE_VAR := $(PACKAGE_VAR), value2

 > But this override is an error:
 > |PACKAGE_VAR = value1
 > |PACKAGE_VAR = value2

 > as well this one:
 > |ifeq ...
 > |PACKAGE_VAR += value1
 > |endif
 > |PACKAGE_VAR = value2

 > And this override is error-prone:
 > |PACKAGE_VAR = value1
 > |ifeq ...
 > |PACKAGE_VAR = value2

 > Create a check function to warn about overridden variables.

 > Some variables are likely to have a default value that gets overridden
 > in a conditional, so ignore them. The name of such variables end in
 > _ARCH, _CPU, _SITE, _SOURCE or _VERSION.

 > After ignoring these variable names, there are a few exceptions to this
 > rule in the tree. For them use the comment that disables the check.

 > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
 > Cc: Simon Dawson <spdawson@gmail.com>
 > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 8/8] utils/check-package: warn about overridden variables

[Yann E. MORIN]

Ricardo, All,

On 2019-01-27 16:59 -0200, Ricardo Martincoski spake thusly:
> For the general case, appending values to variables is OK and also a
> good practice, like this:
> |PACKAGE_VAR = value1
> |ifeq ...
> |PACKAGE_VAR += value2
> 
> or this, when the above is not possible:
> |PACKAGE_VAR = value1
> |ifeq ...
> |PACKAGE_VAR := $(PACKAGE_VAR), value2
> 
> But this override is an error:
> |PACKAGE_VAR = value1
> |PACKAGE_VAR = value2
> 
> as well this one:
> |ifeq ...
> |PACKAGE_VAR += value1
> |endif
> |PACKAGE_VAR = value2
> 
> And this override is error-prone:
> |PACKAGE_VAR = value1
> |ifeq ...
> |PACKAGE_VAR = value2
> 
> Create a check function to warn about overridden variables.
> 
> Some variables are likely to have a default value that gets overridden
> in a conditional, so ignore them. The name of such variables end in
> _ARCH, _CPU, _SITE, _SOURCE or _VERSION.

Since I updated my host distribution to Ubuntu 19.04, flake8 now reports
tons of errors:

    ./utils/checkpackagelib/lib_mk.py:22:7: W605 invalid escape sequence '\s'

This is because Ubuntu has update pycodestyle to 2.4.0, which introduced
this new W605 check. It mandates that regexps be prefixed by an 'r'
(like a binary string be prefixed by a 'b'), like:

    r"^\s*#"

However, when I fix all these 82 new errors, and run check-package, I
now get a warning about overriden license lists in the few packages
where we do override the license, like in dtc:

    ifeq ($(BR2_PACKAGE_DTC_PROGRAMS),y)
    DTC_LICENSE := $(DTC_LICENSE), GPL-2.0+ (programs)
    [...]
    endif

However, as far as I understand the code, there is an explicit condition
that allows concatenating to a variable (the CONCATENATING regexp), now
line 127.

Do you have an idea what's going on now?

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 8/8] utils/check-package: warn about overridden variables

[Arnout Vandecappelle]

On 26/05/2019 11:20, Yann E. MORIN wrote:
[snip]
> However, when I fix all these 82 new errors, and run check-package, I
> now get a warning about overriden license lists in the few packages
> where we do override the license, like in dtc:
> 
>     ifeq ($(BR2_PACKAGE_DTC_PROGRAMS),y)
>     DTC_LICENSE := $(DTC_LICENSE), GPL-2.0+ (programs)
>     [...]
>     endif

 This should not trigger an error (which is actually a bug in check-package, but
that's a different issue). check-package code is:

        if self.conditional == 0:
            ...
        else:
            if variable not in self.unconditionally_set:
                self.conditionally_set.append(variable)
                return

i.e. because we're now in a conditional, and the variable was unconditionally
assigned before, it's just added to conditionally_set without further checking.

 I don't see how replacing \s by \\s (or putting r in front of the string, which
is equivalent) could change that behaviour. Except maybe the
START_CONDITIONAL/END_CONDITIONAL REs are no longer correct?


> However, as far as I understand the code, there is an explicit condition
> that allows concatenating to a variable (the CONCATENATING regexp), now
> line 127.

 Note that if you put an r in front, you have to replace \\ by \ again.
CONCATENATING has \\1...

 Regards,
 Arnout

> 
> Do you have an idea what's going on now?
> 
> Regards,
> Yann E. MORIN.
>