| Message ID | 1512188904-31366-1-git-send-email-ricardo.martincoski@datacom.ind.br |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/1] check-package: avoid false warning of useless flag | expand |
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 >
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
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, 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
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
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 >
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),
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(-)