| Message ID | d2844704851e4e8ff93c3007e9e67e2a339e11c7.1511996903.git.yann.morin.1998@free.fr |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/8] package/lttng-tools: fix typo in variable name | expand |
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
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@datacom.ind.br www.datacom.ind.br
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
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
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
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(+)