Fix warnings reported by check-package

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" <[email protected]>

Cc Philippe Proulx <[email protected]>

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

Reported by utils/check-package.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Adam Duskett <[email protected]>

Cc Clayton Shotwell <[email protected]>

Cc Matt Weber <[email protected]>

---
 package/checkpolicy/checkpolicy.mk | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

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

Reported by utils/check-package.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Frank Hunleth <[email protected]>

---
 package/am335x-pru-package/am335x-pru-package.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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

Reported by utils/check-package.

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 package/lockfile-progs/lockfile-progs.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

... as reported by utils/check-package

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Adam Duskett <[email protected]>

Cc Yegor Yefremov<[email protected]>

Cc Matt Weber <[email protected]>

Cc Thomas Petazzoni <[email protected]>

---
 package/python3/python3.mk | 2 --
 package/setools/setools.mk | 1 -
 2 files changed, 3 deletions(-)

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

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Adam Duskett <[email protected]>

---
 package/busybox/busybox.mk | 2 +-
 package/ntp/ntp.mk         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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" <[email protected]>

Cc Ricardo Martincoski <[email protected]>

Cc Thomas Petazzoni <[email protected]>

---
 package/asterisk/asterisk.mk | 1 +
 1 file changed, 1 insertion(+)

package/asterisk/asterisk.mk +0 -0

index 50512c0b3a..21dca549a0 100644

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

package/asterisk/asterisk.mk HOST_ASTERISK_LICENSE_FILES = COPYING · line 289

Quoted 7 lines

+++ b/package/asterisk/asterisk.mk

package/asterisk/asterisk.mk +1 -0

@@ -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 ?

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

+++ utils/checkpackagelib/lib_mk.py

(quoted diff) +1 -1

@@ -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).

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" <[email protected]>

Cc Ricardo Martincoski <[email protected]>

Cc Thomas Petazzoni <[email protected]>

---
 package/asterisk/asterisk.mk | 1 +
 1 file changed, 1 insertion(+)

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 <[email protected]>

Cc Yann E. MORIN <[email protected]>

Reported-by Yann E. MORIN <[email protected]>

Signed-off-by Ricardo Martincoski <[email protected]>

---
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(-)

utils/checkpackagelib/lib_mk.py +1 -1

index 6ed6011..817e809 100644
@@ -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),

Signature · 1 line

2.7.4

Quoted 4 lines

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.

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...

Quoted 18 lines

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

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

Quoted 6 lines

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

Signature · 3 lines

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

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...

Hello,

Quoted 10 lines · Yann E. MORIN

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.

Quoted 1 line

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

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 <[email protected]>

Cc Yann E. MORIN <[email protected]>

Reported-by Yann E. MORIN <[email protected]>

Signed-off-by Ricardo Martincoski <[email protected]>

---
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(-)

.gitlab-ci.yml check-DEVELOPERS: · line 35

Quoted 3 lines

.gitlab-ci.yml +3 -0

+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

Signature · 3 lines

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

On 2017-11-30 08:59 +0100, Thomas Petazzoni spake thusly:

Quoted 5 lines · Yann E. MORIN

Quoted 3 lines

.gitlab-ci.yml +3 -0

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

Quoted 1 line

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.

Quoted 2 lines

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?

Quoted 2 lines

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.

Quoted 7 lines

On 2017-11-30 08:59 +0100, Thomas Petazzoni spake thusly:

Quoted 6 lines · Yann E. MORIN

.gitlab-ci.yml check-DEVELOPERS: · line 35

Quoted 3 lines

.gitlab-ci.yml +3 -0

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

Does it run without warning on all .mk files?

Quoted 2 lines

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.

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

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Arnout Vandecappelle <[email protected]>

---
 .gitlab-ci.yml    | 4 ++++
 .gitlab-ci.yml.in | 4 ++++
 2 files changed, 8 insertions(+)