[Buildroot] [PATCH 1/8] package/lttng-tools: fix typo in variable name

[Buildroot] [PATCH 1/8] package/lttng-tools: fix typo in variable name

[Yann E. MORIN]

It's lttng-tools, not lttng-libust. ;-)

Fixes numerous build failures caused by a late autoreconf:
    http://autobuild.buildroot.org/results/b84/b84a6e39fcca70c56bfe49d54c385dfe6da82422/
    etc...

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Philippe Proulx <eeppeliteloop@gmail.com>
---
 package/lttng-tools/lttng-tools.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/lttng-tools/lttng-tools.mk b/package/lttng-tools/lttng-tools.mk
index f0c5febafd..451f652cdc 100644
--- a/package/lttng-tools/lttng-tools.mk
+++ b/package/lttng-tools/lttng-tools.mk
@@ -11,7 +11,7 @@ LTTNG_TOOLS_LICENSE = GPL-2.0+, LGPL-2.1+ (include/lttng/*, src/lib/lttng-ctl/*)
 LTTNG_TOOLS_LICENSE_FILES = gpl-2.0.txt lgpl-2.1.txt LICENSE
 LTTNG_TOOLS_CONF_OPTS += --disable-man-pages
 # 0001-Fix-detect-dlmopen-and-disable-corresponding-tests-i.patch
-LTTNG_LIBUST_AUTORECONF = YES
+LTTNG_TOOLS_AUTORECONF = YES
 LTTNG_TOOLS_DEPENDENCIES = liburcu libxml2 popt util-linux
 
 ifeq ($(BR2_PACKAGE_LTTNG_LIBUST),y)
-- 
2.11.0

[Buildroot] [PATCH 2/8] package/checkpolicy: rename variable

[Yann E. MORIN]

We use package names as poor-man's namespace, so fix that.

Reported by utils/check-package.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Adam Duskett <aduskett@gmail.com>
Cc: Clayton Shotwell <clayton.shotwell@rockwellcollins.com>
Cc: Matt Weber <matthew.weber@rockwellcollins.com>
---
 package/checkpolicy/checkpolicy.mk | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/package/checkpolicy/checkpolicy.mk b/package/checkpolicy/checkpolicy.mk
index c1fb43da4d..536a7d69ca 100644
--- a/package/checkpolicy/checkpolicy.mk
+++ b/package/checkpolicy/checkpolicy.mk
@@ -11,22 +11,22 @@ CHECKPOLICY_LICENSE_FILES = COPYING
 
 CHECKPOLICY_DEPENDENCIES = libselinux flex host-flex host-bison
 
-TARGET_CHECKPOLICY_MAKE_OPTS = $(TARGET_CONFIGURE_OPTS) \
+CHECKPOLICY_TARGET_MAKE_OPTS = $(TARGET_CONFIGURE_OPTS) \
 	LEX="$(HOST_DIR)/bin/flex" \
 	YACC="$(HOST_DIR)/bin/bison -y"
 
 # DESTDIR is used at build time to find libselinux
 define CHECKPOLICY_BUILD_CMDS
-	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) $(TARGET_CHECKPOLICY_MAKE_OPTS) DESTDIR=$(STAGING_DIR)
+	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) $(CHECKPOLICY_TARGET_MAKE_OPTS) DESTDIR=$(STAGING_DIR)
 endef
 
 define CHECKPOLICY_STAGING_CMDS
-	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) $(TARGET_CHECKPOLICY_MAKE_OPTS) DESTDIR=$(STAGING_DIR) install
+	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) $(CHECKPOLICY_TARGET_MAKE_OPTS) DESTDIR=$(STAGING_DIR) install
 
 endef
 
 define CHECKPOLICY_INSTALL_TARGET_CMDS
-	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) $(TARGET_CHECKPOLICY_MAKE_OPTS) DESTDIR=$(TARGET_DIR) install
+	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) $(CHECKPOLICY_TARGET_MAKE_OPTS) DESTDIR=$(TARGET_DIR) install
 endef
 
 HOST_CHECKPOLICY_DEPENDENCIES = host-libselinux host-flex host-bison
-- 
2.11.0

[Buildroot] [PATCH 0/8] Fix warnings reported by check-package

[Yann E. MORIN]

Hello All!

This series fixes variables names as reported by check-package, to apply
our porr-man's namespace in packages.

Also fix extra lines and trailing white-spaces.

It eventually adds a gitlab-ci job to automatically run check-package,
but I suggest it is not applied because there is one false-positive
reported by check-package, on the asterisk package:

    package/asterisk/asterisk.mk:289: useless default value
    (http://nightly.buildroot.org/#_infrastructure_for_autotools_based_packages)

However, this one is about _AUTORECONF, which is inherited from target
to host variants, but for asterisk, we explcitly do not want to
autoreconf the host variant (not the same _SUBDIR tree!).


Regards,
Yann E. MORIN.


The following changes since commit 15d1962cf8b70ebcf52614301f0db242894e28a2

  ndisc6: fix bogus <pkg>_DEPENDENCIES names (2017-11-29 23:16:22 +0100)


are available in the git repository at:

  git://git.buildroot.org/~ymorin/git/buildroot.git

for you to fetch changes up to b9a9596c19817cf5a70c17acaa23657ed3597f01

  gitlab-ci: run check-package (2017-11-30 00:07:50 +0100)


----------------------------------------------------------------
Yann E. MORIN (8):
      package/lttng-tools: fix typo in variable name
      package/checkpolicy: rename variable
      package/am335x-pru-package: rename variable
      package/lockfile-progs: rename variable
      packages: remove "consecutive empty lines"
      packages: fix trailing spaces and slash
      package/asterisk: add comment about a check-package false positive
      gitlab-ci: run check-package

 .gitlab-ci.yml                                   | 4 ++++
 .gitlab-ci.yml.in                                | 4 ++++
 package/am335x-pru-package/am335x-pru-package.mk | 6 +++---
 package/asterisk/asterisk.mk                     | 1 +
 package/busybox/busybox.mk                       | 2 +-
 package/checkpolicy/checkpolicy.mk               | 8 ++++----
 package/lockfile-progs/lockfile-progs.mk         | 4 ++--
 package/lttng-tools/lttng-tools.mk               | 2 +-
 package/ntp/ntp.mk                               | 2 +-
 package/python3/python3.mk                       | 2 --
 package/setools/setools.mk                       | 1 -
 11 files changed, 21 insertions(+), 15 deletions(-)

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

[Buildroot] [PATCH 3/8] package/am335x-pru-package: rename variable

[Yann E. MORIN]

We use package names as poor-man's namespace, so fix that.

Reported by utils/check-package.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Frank Hunleth <fhunleth@troodon-software.com>
---
 package/am335x-pru-package/am335x-pru-package.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/package/am335x-pru-package/am335x-pru-package.mk b/package/am335x-pru-package/am335x-pru-package.mk
index b2e89d3710..24a5df4815 100644
--- a/package/am335x-pru-package/am335x-pru-package.mk
+++ b/package/am335x-pru-package/am335x-pru-package.mk
@@ -13,14 +13,14 @@ AM335X_PRU_PACKAGE_INSTALL_STAGING = YES
 
 # The default 'all' rule builds everything, when we just need the library
 ifeq ($(BR2_ENABLE_DEBUG),y)
-AM335X_MAKE_TARGET = debug $(if $(BR2_STATIC_LIBS),,sodebug)
+AM335X_PRU_PACKAGE_MAKE_TARGET = debug $(if $(BR2_STATIC_LIBS),,sodebug)
 else
-AM335X_MAKE_TARGET = release $(if $(BR2_STATIC_LIBS),,sorelease)
+AM335X_PRU_PACKAGE_MAKE_TARGET = release $(if $(BR2_STATIC_LIBS),,sorelease)
 endif
 
 define AM335X_PRU_PACKAGE_BUILD_CMDS
 	$(TARGET_MAKE_ENV) $(MAKE) CROSS_COMPILE="$(TARGET_CROSS)" \
-		-C $(@D)/pru_sw/app_loader/interface $(AM335X_MAKE_TARGET)
+		-C $(@D)/pru_sw/app_loader/interface $(AM335X_PRU_PACKAGE_MAKE_TARGET)
 endef
 
 # 'install' installs whatever was built, and our patch removes the dependency
-- 
2.11.0

[Buildroot] [PATCH 4/8] package/lockfile-progs: rename variable

[Yann E. MORIN]

We use package names as poor-man's namespace, so fix that.

Reported by utils/check-package.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 package/lockfile-progs/lockfile-progs.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/package/lockfile-progs/lockfile-progs.mk b/package/lockfile-progs/lockfile-progs.mk
index 57df573d4a..ed192f1028 100644
--- a/package/lockfile-progs/lockfile-progs.mk
+++ b/package/lockfile-progs/lockfile-progs.mk
@@ -11,7 +11,7 @@ LOCKFILE_PROGS_DEPENDENCIES = liblockfile
 LOCKFILE_PROGS_LICENSE = GPL-2.0
 LOCKFILE_PROGS_LICENSE_FILES = COPYING
 
-LOCKFILE_BINS = \
+LOCKFILE_PROGS_BINS = \
 	$(addprefix lockfile-,check create remove touch) \
 	$(addprefix mail-,lock touchlock unlock)
 
@@ -20,7 +20,7 @@ define LOCKFILE_PROGS_BUILD_CMDS
 endef
 
 define LOCKFILE_PROGS_INSTALL_TARGET_CMDS
-	for i in $(LOCKFILE_BINS); do \
+	for i in $(LOCKFILE_PROGS_BINS); do \
 		$(INSTALL) -D -m 755 $(@D)/bin/$$i $(TARGET_DIR)/usr/bin/$$i || exit 1; \
 	done
 endef
-- 
2.11.0

[Buildroot] [PATCH 5/8] packages: remove "consecutive empty lines"

[Yann E. MORIN]

... as reported by utils/check-package

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Adam Duskett <aduskett@gmail.com>
Cc: Yegor Yefremov<yegorslists@googlemail.com>
Cc: Matt Weber <matthew.weber@rockwellcollins.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 package/python3/python3.mk | 2 --
 package/setools/setools.mk | 1 -
 2 files changed, 3 deletions(-)

diff --git a/package/python3/python3.mk b/package/python3/python3.mk
index 1d2201eec7..6e61108661 100644
--- a/package/python3/python3.mk
+++ b/package/python3/python3.mk
@@ -159,7 +159,6 @@ PYTHON3_CONF_OPTS += \
 	--disable-idle3 \
 	--disable-pyc-build
 
-
 #
 # Some of CPython's source code is generated using Python interpreter
 # and some helper tools such as "Programs/_freeze_importlib" or
@@ -201,7 +200,6 @@ endef
 
 PYTHON3_PRE_BUILD_HOOKS += PYTHON3_MAKE_REGEN_IMPORTLIB
 
-
 #
 # Remove useless files. In the config/ directory, only the Makefile
 # and the pyconfig.h files are needed at runtime.
diff --git a/package/setools/setools.mk b/package/setools/setools.mk
index 8b93ccf45e..6748c95c23 100644
--- a/package/setools/setools.mk
+++ b/package/setools/setools.mk
@@ -13,7 +13,6 @@ SETOOLS_LICENSE_FILES = COPYING COPYING.GPL COPYING.LGPL
 SETOOLS_SETUP_TYPE = setuptools
 HOST_SETOOLS_DEPENDENCIES = host-libselinux host-libsepol
 
-
 ifeq ($(BR2_PACKAGE_PYTHON3),y)
 SETOOLS_PYLIBVER = python$(PYTHON3_VERSION_MAJOR)
 else
-- 
2.11.0

[Buildroot] [PATCH 6/8] packages: fix trailing spaces and slash

[Yann E. MORIN]

... as reported by utils/check-package.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Adam Duskett <aduskett@gmail.com>
---
 package/busybox/busybox.mk | 2 +-
 package/ntp/ntp.mk         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/package/busybox/busybox.mk b/package/busybox/busybox.mk
index 1aeeeef4bf..8b720b30b8 100644
--- a/package/busybox/busybox.mk
+++ b/package/busybox/busybox.mk
@@ -65,7 +65,7 @@ define BUSYBOX_PERMISSIONS
 # Set permissions on all applets with BB_SUID_REQUIRE and BB_SUID_MAYBE.
 # 12 Applets are pulled from applets.h using grep command :
 #  grep -r -e "APPLET.*BB_SUID_REQUIRE\|APPLET.*BB_SUID_MAYBE" \
-#  $(@D)/include/applets.h 
+#  $(@D)/include/applets.h
 # These applets are added to the device table and the makedev file
 # ignores the files with type 'F' ( optional files).
 	/usr/bin/wall 			 F 4755 0  0 - - - - -
diff --git a/package/ntp/ntp.mk b/package/ntp/ntp.mk
index 6605c3b85b..05619aef7c 100644
--- a/package/ntp/ntp.mk
+++ b/package/ntp/ntp.mk
@@ -17,7 +17,7 @@ NTP_CONF_OPTS = \
 	--disable-tickadj \
 	--disable-debugging \
 	--with-yielding-select=yes \
-	--disable-local-libevent \
+	--disable-local-libevent
 
 # 0002-ntp-syscalls-fallback.patch
 # 0003-ntpq-fpic.patch
-- 
2.11.0

[Buildroot] [PATCH 7/8] package/asterisk: add comment about a check-package false positive

[Yann E. MORIN]

This variable is inherited from the target variant, which is
autoreconfed. But the host variant is only the menucselect
sub-directory, which we do not want to autoreconf, so the explcit
NO is required.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 package/asterisk/asterisk.mk | 1 +
 1 file changed, 1 insertion(+)

diff --git a/package/asterisk/asterisk.mk b/package/asterisk/asterisk.mk
index 50512c0b3a..21dca549a0 100644
--- a/package/asterisk/asterisk.mk
+++ b/package/asterisk/asterisk.mk
@@ -285,6 +285,7 @@ HOST_ASTERISK_LICENSE_FILES = COPYING
 
 # No need to autoreconf for the host variant,
 # so do not inherit the target setup.
+# check-package reports an issue here, but that's a false positive. Ignore.
 HOST_ASTERISK_AUTORECONF = NO
 
 HOST_ASTERISK_CONF_ENV = CONFIG_LIBXML2=$(HOST_DIR)/usr/bin/xml2-config
-- 
2.11.0

[Buildroot] [PATCH 8/8] gitlab-ci: run check-package

[Yann E. MORIN]

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
---
 .gitlab-ci.yml    | 4 ++++
 .gitlab-ci.yml.in | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 0dddb22f12..c80a6b129b 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -30,6 +30,10 @@ check-DEVELOPERS:
     script:
         - "! utils/get-developers | grep -v 'No action specified'"
 
+check-package:
+    script:
+        - find . -type f -name '*.mk' -exec ./utils/check-package {} +
+
 .defconfig: &defconfig
     # Running the defconfigs for every push is too much, so limit to
     # explicit triggers through the API.
diff --git a/.gitlab-ci.yml.in b/.gitlab-ci.yml.in
index 33c7b13725..0d2e239414 100644
--- a/.gitlab-ci.yml.in
+++ b/.gitlab-ci.yml.in
@@ -30,6 +30,10 @@ check-DEVELOPERS:
     script:
         - "! utils/get-developers | grep -v 'No action specified'"
 
+check-package:
+    script:
+        - find . -type f -name '*.mk' -exec ./utils/check-package {} +
+
 .defconfig: &defconfig
     # Running the defconfigs for every push is too much, so limit to
     # explicit triggers through the API.
-- 
2.11.0

[Buildroot] [PATCH 1/8] package/lttng-tools: fix typo in variable name

[Thomas Petazzoni]

Hello,

On Thu, 30 Nov 2017 00:08:38 +0100, Yann E. MORIN wrote:
> It's lttng-tools, not lttng-libust. ;-)
> 
> Fixes numerous build failures caused by a late autoreconf:
>     http://autobuild.buildroot.org/results/b84/b84a6e39fcca70c56bfe49d54c385dfe6da82422/
>     etc...
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Philippe Proulx <eeppeliteloop@gmail.com>

You had already sent this one, and I already merged it. So I'll mark
that one as Rejected in patchwork.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 8/8] gitlab-ci: run check-package

[Thomas Petazzoni]

Hello,

On Thu, 30 Nov 2017 00:08:45 +0100, Yann E. MORIN wrote:

> +check-package:
> +    script:
> +        - find . -type f -name '*.mk' -exec ./utils/check-package {} +

Does it run without warning on all .mk files?

In fact, I think we shouldn't limit it to .mk files, because
check-package can also verify Config.in files and .hash files.

Perhaps:

  find package/ boot/ linux/ -type f -exec ./utils/check-package {}

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 7/8] package/asterisk: add comment about a check-package false positive

[Thomas Petazzoni]

Hello,

On Thu, 30 Nov 2017 00:08:44 +0100, Yann E. MORIN wrote:
> This variable is inherited from the target variant, which is
> autoreconfed. But the host variant is only the menucselect

menuselect

> sub-directory, which we do not want to autoreconf, so the explcit
> NO is required.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  package/asterisk/asterisk.mk | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/package/asterisk/asterisk.mk b/package/asterisk/asterisk.mk
> index 50512c0b3a..21dca549a0 100644
> --- a/package/asterisk/asterisk.mk
> +++ b/package/asterisk/asterisk.mk
> @@ -285,6 +285,7 @@ HOST_ASTERISK_LICENSE_FILES = COPYING
>  
>  # No need to autoreconf for the host variant,
>  # so do not inherit the target setup.
> +# check-package reports an issue here, but that's a false positive. Ignore.
>  HOST_ASTERISK_AUTORECONF = NO

I'm not sure we should have a comment here. Instead we should fix
check-package: it is not normal for check-package to emit a warning
here, since it is legal (currently) for a package to autoreconf its
target variant, and not autoreconf its host variant.

Ricardo, do you think you could fix this problem in check-package ?

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 7/8] package/asterisk: add comment about a check-package false positive

[Ricardo Martincoski]

Hello,

On Thursday, November 30, 2017 6:00:57 AM, Thomas Petazzoni wrote:
> On Thu, 30 Nov 2017 00:08:44 +0100, Yann E. MORIN wrote:
[snip]
>> +++ b/package/asterisk/asterisk.mk
>> @@ -285,6 +285,7 @@ HOST_ASTERISK_LICENSE_FILES = COPYING
>>  
>>  # No need to autoreconf for the host variant,
>>  # so do not inherit the target setup.
>> +# check-package reports an issue here, but that's a false positive. Ignore.
>>  HOST_ASTERISK_AUTORECONF = NO
> 
> I'm not sure we should have a comment here. Instead we should fix
> check-package: it is not normal for check-package to emit a warning
> here, since it is legal (currently) for a package to autoreconf its
> target variant, and not autoreconf its host variant.

I agree, it's better to not issue a false warning.

> 
> Ricardo, do you think you could fix this problem in check-package ?

We can use something like this (not fully tested yet!):

+++ utils/checkpackagelib/lib_mk.py
@@ -219 +219 @@ class UselessFlag(_CheckFunction):
-        if self.DEFAULT_AUTOTOOLS_FLAG.search(text):
+        if self.DEFAULT_AUTOTOOLS_FLAG.search(text) and not text.lstrip().startswith("HOST_"):

Avoiding false warnings is more important than testing for all corner
cases IMO. Using the code above we don't issue a valid warning for
rare cases, we stop issuing a false warning and we also keep O(n).

I will send it separately (after proper commit message and testing)
if Yann doesn't include something similar (feel free to do so!) to
the series before.

Regards,
--
Ricardo Martincoski
DATACOM 
Ethernet Switches 
Rua Am?rica, 1000 - Eldorado do Sul, RS - 92990-000 - Brasil
+55 51 3933 3000 - Ramal 3307
ricardo.martincoski at datacom.ind.br
www.datacom.ind.br

[Buildroot] [PATCH 7/8] package/asterisk: add comment about a check-package false positive

[Thomas Petazzoni]

Hello,

Thanks for looking into it so quickly!

On Thu, 30 Nov 2017 09:19:41 -0200 (BRST), Ricardo Martincoski wrote:

> We can use something like this (not fully tested yet!):
> 
> +++ utils/checkpackagelib/lib_mk.py
> @@ -219 +219 @@ class UselessFlag(_CheckFunction):
> -        if self.DEFAULT_AUTOTOOLS_FLAG.search(text):
> +        if self.DEFAULT_AUTOTOOLS_FLAG.search(text) and not text.lstrip().startswith("HOST_"):
> 
> Avoiding false warnings is more important than testing for all corner
> cases IMO. Using the code above we don't issue a valid warning for
> rare cases, we stop issuing a false warning and we also keep O(n).

Yeah, if you want to test this properly, it's more difficult. Just
AUTORECONF = NO is redundant. Just HOST_AUTORECONF = NO is redundant.
But the combination of AUTORECONF = YES + HOST_AUTORECONF = NO is valid.

So basically for all variables that have inheritance between target
and host, having the host variant of the variable set the variable
value back to its default is correct if the target variable is set.

So, your approach at least works, and will not generate false warnings.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 8/8] gitlab-ci: run check-package

[Yann E. MORIN]

Thomas, All,

On 2017-11-30 08:59 +0100, Thomas Petazzoni spake thusly:
> On Thu, 30 Nov 2017 00:08:45 +0100, Yann E. MORIN wrote:
> > +check-package:
> > +    script:
> > +        - find . -type f -name '*.mk' -exec ./utils/check-package {} +
> 
> Does it run without warning on all .mk files?

As I explained in thecover letter, no. There is a false-positive in
asterisk.

That's why I suggested in the cover-letter not to applu it for now,
until check-pacakge learns about that case.

> In fact, I think we shouldn't limit it to .mk files, because
> check-package can also verify Config.in files and .hash files.

This can be refined later on, probably?

> Perhaps:
>   find package/ boot/ linux/ -type f -exec ./utils/check-package {}

But then it would also catch the patches, the init scripts, and any
other data file that is present in packages directories.

For now, this catches 305+539+264 = 1108 warnings...

So I would at least limit it to Config.in, .mk, .hash, .patch files.

And even that generates 305+566+230 = 1101 warnings.

Regards,
Yann E. MORIN.

PS. Not sure what the split in three numbers is:

  $ find package/ boot/ linux/ -type f \
        \( -name '*.mk' -o -name '*.hash' \
           -o -name '*.patch' -o -name Config.in \
        \) -exec ./utils/check-package {} + 2>&1 \
    |grep 'warnings generated'
    305 warnings generated
    566 warnings generated
    230 warnings generated

> Best regards,
> 
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

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

[Buildroot] [PATCH 8/8] gitlab-ci: run check-package

[Thomas Petazzoni]

Hello,

On Thu, 30 Nov 2017 16:34:58 +0100, Yann E. MORIN wrote:

> On 2017-11-30 08:59 +0100, Thomas Petazzoni spake thusly:
> > On Thu, 30 Nov 2017 00:08:45 +0100, Yann E. MORIN wrote:  
> > > +check-package:
> > > +    script:
> > > +        - find . -type f -name '*.mk' -exec ./utils/check-package {} +  
> > 
> > Does it run without warning on all .mk files?  
> 
> As I explained in thecover letter, no. There is a false-positive in
> asterisk.
> 
> That's why I suggested in the cover-letter not to applu it for now,
> until check-pacakge learns about that case.

Ricardo already proposed a fix for check-package to avoid the asterisk
case :-)

> > In fact, I think we shouldn't limit it to .mk files, because
> > check-package can also verify Config.in files and .hash files.  
> 
> This can be refined later on, probably?

True.

> > Perhaps:
> >   find package/ boot/ linux/ -type f -exec ./utils/check-package {}  
> 
> But then it would also catch the patches, the init scripts, and any
> other data file that is present in packages directories.
> 
> For now, this catches 305+539+264 = 1108 warnings...

Doh.

> So I would at least limit it to Config.in, .mk, .hash, .patch files.
> 
> And even that generates 305+566+230 = 1101 warnings.

That's indeed a lot. In addition, I want the thing to "fail" if there
are some warnings, otherwise Gitlab CI will not report the job as
failed.

So, let's do this:

 1. Fix check-package to avoid the false warning on asterisk

 2. Add the Gitlab CI job testing only .mk files, but making sure that
    if there is a single warning, it returns with a non-zero error code
    so that the job is considered as failed if we have a warning

 3. Over time, fix warnings in Config.in and .hash files, until the
    point where we can enable checking them in Gitlab CI as well.

Thoughts?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 8/8] gitlab-ci: run check-package

[Yann E. MORIN]

Thomas, All,

On 2017-12-01 10:54 +0100, Thomas Petazzoni spake thusly:
> On Thu, 30 Nov 2017 16:34:58 +0100, Yann E. MORIN wrote:
> > On 2017-11-30 08:59 +0100, Thomas Petazzoni spake thusly:
> > > On Thu, 30 Nov 2017 00:08:45 +0100, Yann E. MORIN wrote:  
> > > > +check-package:
> > > > +    script:
> > > > +        - find . -type f -name '*.mk' -exec ./utils/check-package {} +  
> > > 
> > > Does it run without warning on all .mk files?  
[--SNIP--]
> > So I would at least limit it to Config.in, .mk, .hash, .patch files.
> > And even that generates 305+566+230 = 1101 warnings.
> 
> That's indeed a lot. In addition, I want the thing to "fail" if there
> are some warnings, otherwise Gitlab CI will not report the job as
> failed.

Agreed.

> So, let's do this:
> 
>  1. Fix check-package to avoid the false warning on asterisk

Waiting for Ricardo's change to trickle in. Ricardo, can you send an
official patch, please?

>  2. Add the Gitlab CI job testing only .mk files, but making sure that
>     if there is a single warning, it returns with a non-zero error code
>     so that the job is considered as failed if we have a warning

Then this series is ready as it is, minus the comment in asterisk.mk.

>  3. Over time, fix warnings in Config.in and .hash files, until the
>     point where we can enable checking them in Gitlab CI as well.

Agreed.

Regards,
Yann E. MORIN.

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

[Buildroot] [PATCH 2/8] package/checkpolicy: rename variable

[Thomas Petazzoni]

Hello,

On Thu, 30 Nov 2017 00:08:39 +0100, Yann E. MORIN wrote:
> We use package names as poor-man's namespace, so fix that.
> 
> Reported by utils/check-package.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Adam Duskett <aduskett@gmail.com>
> Cc: Clayton Shotwell <clayton.shotwell@rockwellcollins.com>
> Cc: Matt Weber <matthew.weber@rockwellcollins.com>
> ---
>  package/checkpolicy/checkpolicy.mk | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Applied to master, after doing the following change:

    [Thomas: use CHECKPOLICY_MAKE_OPTS instead of
    CHECKPOLICY_TARGET_MAKE_OPTS, as it is more consistent with
    HOST_CHECKPOLICY_MAKE_OPTS being used for the host variant.]

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 3/8] package/am335x-pru-package: rename variable

[Thomas Petazzoni]

Hello,

On Thu, 30 Nov 2017 00:08:40 +0100, Yann E. MORIN wrote:
> We use package names as poor-man's namespace, so fix that.
> 
> Reported by utils/check-package.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Frank Hunleth <fhunleth@troodon-software.com>
> ---
>  package/am335x-pru-package/am335x-pru-package.mk | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Applied to master, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 4/8] package/lockfile-progs: rename variable

[Thomas Petazzoni]

Hello,

On Thu, 30 Nov 2017 00:08:41 +0100, Yann E. MORIN wrote:
> We use package names as poor-man's namespace, so fix that.
> 
> Reported by utils/check-package.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> ---
>  package/lockfile-progs/lockfile-progs.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied to master, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 5/8] packages: remove "consecutive empty lines"

[Thomas Petazzoni]

Hello,

On Thu, 30 Nov 2017 00:08:42 +0100, Yann E. MORIN wrote:
> ... as reported by utils/check-package
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Adam Duskett <aduskett@gmail.com>
> Cc: Yegor Yefremov<yegorslists@googlemail.com>
> Cc: Matt Weber <matthew.weber@rockwellcollins.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  package/python3/python3.mk | 2 --
>  package/setools/setools.mk | 1 -
>  2 files changed, 3 deletions(-)

Applied to master, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 6/8] packages: fix trailing spaces and slash

[Thomas Petazzoni]

Hello,

On Thu, 30 Nov 2017 00:08:43 +0100, Yann E. MORIN wrote:
> ... as reported by utils/check-package.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Adam Duskett <aduskett@gmail.com>
> ---
>  package/busybox/busybox.mk | 2 +-
>  package/ntp/ntp.mk         | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Applied to master, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 7/8] package/asterisk: add comment about a check-package false positive

[Thomas Petazzoni]

Hello,

On Thu, 30 Nov 2017 00:08:44 +0100, Yann E. MORIN wrote:
> This variable is inherited from the target variant, which is
> autoreconfed. But the host variant is only the menucselect
> sub-directory, which we do not want to autoreconf, so the explcit
> NO is required.
> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  package/asterisk/asterisk.mk | 1 +
>  1 file changed, 1 insertion(+)

As we discussed, I marked this one as Rejected in patchwork. Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 8/8] gitlab-ci: run check-package

[Thomas Petazzoni]

Hello,

On Thu, 30 Nov 2017 00:08:45 +0100, Yann E. MORIN wrote:
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> ---
>  .gitlab-ci.yml    | 4 ++++
>  .gitlab-ci.yml.in | 4 ++++
>  2 files changed, 8 insertions(+)

Applied to master, after doing two other commits fixing .mk files
warning introduced in the next branch. We're down to just the false
warning from asterisk.mk.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] check-package: avoid false warning of useless flag

[Ricardo Martincoski]

Just AUTORECONF = NO is redundant.
Just HOST_AUTORECONF = NO is redundant.
But the combination of AUTORECONF = YES + HOST_AUTORECONF = NO is valid.

So basically for all variables that have inheritance between target and
host, having the host variant of the variable set the variable value
back to its default is correct if the target variable is set.

Instead of increasing complexity of the script to fully detect this
case, ignore the host flag set to its default value as it can be
overriding a non-default value inherited from the equivalent target
flag.

Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Yann E. MORIN <yann.morin.1998@free.fr>
Reported-by: Yann E. MORIN <yann.morin.1998@free.fr>
Signed-off-by: Ricardo Martincoski <ricardo.martincoski@datacom.ind.br>
---
Part of commit log copied from Thomas' e-mail. I hope you don't mind.
---
 utils/checkpackagelib/lib_mk.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py
index 6ed6011..817e809 100644
--- a/utils/checkpackagelib/lib_mk.py
+++ b/utils/checkpackagelib/lib_mk.py
@@ -216,7 +216,7 @@ class UselessFlag(_CheckFunction):
                     .format(self.filename, lineno, self.url_to_manual),
                     text]
 
-        if self.DEFAULT_AUTOTOOLS_FLAG.search(text):
+        if self.DEFAULT_AUTOTOOLS_FLAG.search(text) and not text.lstrip().startswith("HOST_"):
             return ["{}:{}: useless default value "
                     "({}#_infrastructure_for_autotools_based_packages)"
                     .format(self.filename, lineno, self.url_to_manual),
-- 
2.7.4

[Buildroot] [PATCH 8/8] gitlab-ci: run check-package

[Ricardo Martincoski]

Hello,

On Thu, Nov 30, 2017 at 01:34 PM, Yann E. MORIN wrote:
[snip]
> And even that generates 305+566+230 = 1101 warnings.
> 
> Regards,
> Yann E. MORIN.
> 
> PS. Not sure what the split in three numbers is:
> 
>   $ find package/ boot/ linux/ -type f \
>         \( -name '*.mk' -o -name '*.hash' \
>            -o -name '*.patch' -o -name Config.in \
>         \) -exec ./utils/check-package {} + 2>&1 \
>     |grep 'warnings generated'
>     305 warnings generated
>     566 warnings generated
>     230 warnings generated

find -exec splits it into 3 lines:
  $ find package/ boot/ linux/ -type f \
        \( -name '*.mk' -o -name '*.hash' \
           -o -name '*.patch' -o -name Config.in \
        \) -exec echo {} + | wc -l
  3
Each one of the first two lines is 128KiB long.
  $ xargs --show-limits
  ...
  Size of command buffer we are actually using: 131072

Regards,
Ricardo

[Buildroot] [PATCH 1/1] check-package: avoid false warning of useless flag

[Yann E. MORIN]

Ricardo, All,

On 2017-12-02 02:28 -0200, Ricardo Martincoski spake thusly:
> Just AUTORECONF = NO is redundant.
> Just HOST_AUTORECONF = NO is redundant.
> But the combination of AUTORECONF = YES + HOST_AUTORECONF = NO is valid.
> 
> So basically for all variables that have inheritance between target and
> host, having the host variant of the variable set the variable value
> back to its default is correct if the target variable is set.
> 
> Instead of increasing complexity of the script to fully detect this
> case, ignore the host flag set to its default value as it can be
> overriding a non-default value inherited from the equivalent target
> flag.

But then we miss the cases where:
  - both are set to their default values,
  - the target one is not set and the host one is set to the default
    value.

But as you said, detecting the real false-positive would require
maintaining some state, which is currently not possible in the script.

Yet, htere is one issue I see: the script currently names issues mere
'warnings' but exits with a non-zero error code as soon as at least one
such 'warning' is seen.

However, what we so far detected were really 'errors', not 'warnings',
while the case we are speaking about now really is a warning: it should
be reported, biut should not exit in error.

So I must say I'm still not really fond of this change... :-(

Also I noticed that the comments do match the regexps:

    288: # We need HOST_ASTERISK_AUTORECONF = NO because the
    289: # target variant has _AUTORECONF = YES
    290: HOST_ASTERISK_AUTORECONF = NO

will report the issue twice (man URL removed):

    package/asterisk/asterisk.mk:288: useless default value [...]
    package/asterisk/asterisk.mk:290: useless default value [...]

Since this is the .mk parser, it could ignore the comments, I believe...

Regards,
Yann E. MORIN.

> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Yann E. MORIN <yann.morin.1998@free.fr>
> Reported-by: Yann E. MORIN <yann.morin.1998@free.fr>
> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@datacom.ind.br>
> ---
> Part of commit log copied from Thomas' e-mail. I hope you don't mind.
> ---
>  utils/checkpackagelib/lib_mk.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py
> index 6ed6011..817e809 100644
> --- a/utils/checkpackagelib/lib_mk.py
> +++ b/utils/checkpackagelib/lib_mk.py
> @@ -216,7 +216,7 @@ class UselessFlag(_CheckFunction):
>                      .format(self.filename, lineno, self.url_to_manual),
>                      text]
>  
> -        if self.DEFAULT_AUTOTOOLS_FLAG.search(text):
> +        if self.DEFAULT_AUTOTOOLS_FLAG.search(text) and not text.lstrip().startswith("HOST_"):
>              return ["{}:{}: useless default value "
>                      "({}#_infrastructure_for_autotools_based_packages)"
>                      .format(self.filename, lineno, self.url_to_manual),
> -- 
> 2.7.4
> 

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

[Buildroot] [PATCH 1/1] check-package: avoid false warning of useless flag

[Thomas Petazzoni]

Hello,

On Sat, 2 Dec 2017 12:03:20 +0100, Yann E. MORIN wrote:

> > Instead of increasing complexity of the script to fully detect this
> > case, ignore the host flag set to its default value as it can be
> > overriding a non-default value inherited from the equivalent target
> > flag.  
> 
> But then we miss the cases where:
>   - both are set to their default values,
>   - the target one is not set and the host one is set to the default
>     value.
> 
> But as you said, detecting the real false-positive would require
> maintaining some state, which is currently not possible in the script.
> 
> Yet, htere is one issue I see: the script currently names issues mere
> 'warnings' but exits with a non-zero error code as soon as at least one
> such 'warning' is seen.
> 
> However, what we so far detected were really 'errors', not 'warnings',
> while the case we are speaking about now really is a warning: it should
> be reported, biut should not exit in error.

I'm not sure we want to go down this road, and distinguish warnings and
errors. Either something is correct, or it's not. I prefer to have a
clean and readable check-package output, with only things that really
need to be fixed.

> Also I noticed that the comments do match the regexps:
> 
>     288: # We need HOST_ASTERISK_AUTORECONF = NO because the
>     289: # target variant has _AUTORECONF = YES
>     290: HOST_ASTERISK_AUTORECONF = NO
> 
> will report the issue twice (man URL removed):
> 
>     package/asterisk/asterisk.mk:288: useless default value [...]
>     package/asterisk/asterisk.mk:290: useless default value [...]
> 
> Since this is the .mk parser, it could ignore the comments, I believe...

Agreed, but that's a separate issue :)

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] check-package: avoid false warning of useless flag

[Thomas Petazzoni]

Hello,

On Sat,  2 Dec 2017 02:28:24 -0200, Ricardo Martincoski wrote:
> Just AUTORECONF = NO is redundant.
> Just HOST_AUTORECONF = NO is redundant.
> But the combination of AUTORECONF = YES + HOST_AUTORECONF = NO is valid.
> 
> So basically for all variables that have inheritance between target and
> host, having the host variant of the variable set the variable value
> back to its default is correct if the target variable is set.
> 
> Instead of increasing complexity of the script to fully detect this
> case, ignore the host flag set to its default value as it can be
> overriding a non-default value inherited from the equivalent target
> flag.
> 
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Yann E. MORIN <yann.morin.1998@free.fr>
> Reported-by: Yann E. MORIN <yann.morin.1998@free.fr>
> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@datacom.ind.br>
> ---
> Part of commit log copied from Thomas' e-mail. I hope you don't mind.
> ---
>  utils/checkpackagelib/lib_mk.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to master, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] check-package: avoid false warning of useless flag

[Yann E. MORIN]

Thomas, All,

On 2017-12-02 14:10 +0100, Thomas Petazzoni spake thusly:
> On Sat, 2 Dec 2017 12:03:20 +0100, Yann E. MORIN wrote:
> 
> > > Instead of increasing complexity of the script to fully detect this
> > > case, ignore the host flag set to its default value as it can be
> > > overriding a non-default value inherited from the equivalent target
> > > flag.  
> > 
> > But then we miss the cases where:
> >   - both are set to their default values,
> >   - the target one is not set and the host one is set to the default
> >     value.
> > 
> > But as you said, detecting the real false-positive would require
> > maintaining some state, which is currently not possible in the script.
> > 
> > Yet, htere is one issue I see: the script currently names issues mere
> > 'warnings' but exits with a non-zero error code as soon as at least one
> > such 'warning' is seen.
> > 
> > However, what we so far detected were really 'errors', not 'warnings',
> > while the case we are speaking about now really is a warning: it should
> > be reported, biut should not exit in error.
> 
> I'm not sure we want to go down this road, and distinguish warnings and
> errors. Either something is correct, or it's not. I prefer to have a
> clean and readable check-package output, with only things that really
> need to be fixed.

OK, but then with this change, we will now be missing actual errors,
e.g.:

    FOO_AUTORECONF = NO
    HOST_FOO_AUTORECONF = NO

Will not report the host vairant as bogus, even after the target variant
gets removed.

So we lose a false positive for a false negative.

I have to admit I am not sure which is worse/best...

Regards,
Yann E. MORIN.

> > Also I noticed that the comments do match the regexps:
> > 
> >     288: # We need HOST_ASTERISK_AUTORECONF = NO because the
> >     289: # target variant has _AUTORECONF = YES
> >     290: HOST_ASTERISK_AUTORECONF = NO
> > 
> > will report the issue twice (man URL removed):
> > 
> >     package/asterisk/asterisk.mk:288: useless default value [...]
> >     package/asterisk/asterisk.mk:290: useless default value [...]
> > 
> > Since this is the .mk parser, it could ignore the comments, I believe...
> 
> Agreed, but that's a separate issue :)
> 
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

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

[Buildroot] [PATCH 1/1] check-package: avoid false warning of useless flag

[Thomas Petazzoni]

Hello,

On Sat, 2 Dec 2017 14:54:43 +0100, Yann E. MORIN wrote:

> OK, but then with this change, we will now be missing actual errors,
> e.g.:
> 
>     FOO_AUTORECONF = NO
>     HOST_FOO_AUTORECONF = NO
> 
> Will not report the host vairant as bogus, even after the target variant
> gets removed.
> 
> So we lose a false positive for a false negative.

Yes, it was already mentioned by Ricardo in the discussion.

> I have to admit I am not sure which is worse/best...

Checking tools that have false positives are unusable, because they
create some noise that always make you think "gaah, it must be the tool
again complaining about something that doesn't make sense".

So I very much prefer a tool that doesn't report false positives, and
for which the results can be trusted. This way, we are much more likely
to pay attention to the tool results, and keep in a good shape what the
tool is checking today.

And that doesn't prevent, in parallel, from improving the tool to
detect other problems.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] check-package: avoid false warning of useless flag

[Arnout Vandecappelle]

On 02-12-17 15:08, Thomas Petazzoni wrote:
> Hello,
> 
> On Sat, 2 Dec 2017 14:54:43 +0100, Yann E. MORIN wrote:
> 
>> OK, but then with this change, we will now be missing actual errors,
>> e.g.:
>>
>>     FOO_AUTORECONF = NO
>>     HOST_FOO_AUTORECONF = NO
>>
>> Will not report the host vairant as bogus, even after the target variant
>> gets removed.
>>
>> So we lose a false positive for a false negative.
> 
> Yes, it was already mentioned by Ricardo in the discussion.
> 
>> I have to admit I am not sure which is worse/best...
> 
> Checking tools that have false positives are unusable, because they
> create some noise that always make you think "gaah, it must be the tool
> again complaining about something that doesn't make sense".
> 
> So I very much prefer a tool that doesn't report false positives, and
> for which the results can be trusted. This way, we are much more likely
> to pay attention to the tool results, and keep in a good shape what the
> tool is checking today.

 +1 to that.

 Regards,
 Arnout

> 
> And that doesn't prevent, in parallel, from improving the tool to
> detect other problems.
> 
> Thomas
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF