* [Buildroot] [PATCH 1/1] utils/checkpackagelib: add function to check of the default package source variable @ 2017-12-18 12:14 Jerzy Grzegorek 2018-01-08 22:48 ` Thomas Petazzoni 0 siblings, 1 reply; 6+ messages in thread From: Jerzy Grzegorek @ 2017-12-18 12:14 UTC (permalink / raw) To: buildroot Signed-off-by: Jerzy Grzegorek <jerzy.m.grzegorek@gmail.com> --- utils/checkpackagelib/lib_mk.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py index 817e809..5ae565c 100644 --- a/utils/checkpackagelib/lib_mk.py +++ b/utils/checkpackagelib/lib_mk.py @@ -99,6 +99,25 @@ class PackageHeader(_CheckFunction): text] +class RemoveDefaultPackageSourceVariable(_CheckFunction): + PACKAGE_NAME = re.compile("/([^/]+)\.mk") + + def before(self): + package = self.PACKAGE_NAME.search(self.filename).group(1) + package_upper = package.replace("-", "_").upper() + self.package = package + self.package_upper = package_upper + self.FIND_SOURCE = re.compile( + "^{}_SOURCE\s*=\s*{}-\$\({}_VERSION\)\.tar\.gz" + .format(package_upper, package, package_upper)) + + def check_line(self, lineno, text): + if self.FIND_SOURCE.search(text): + return ["{}:{}: remove default value of _SOURCE variable ({}#writing-rules-mk)" + .format(self.filename, lineno, self.url_to_manual), + text] + + class SpaceBeforeBackslash(_CheckFunction): TAB_OR_MULTIPLE_SPACES_BEFORE_BACKSLASH = re.compile(r"^.*( |\t)\\$") -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] utils/checkpackagelib: add function to check of the default package source variable 2017-12-18 12:14 [Buildroot] [PATCH 1/1] utils/checkpackagelib: add function to check of the default package source variable Jerzy Grzegorek @ 2018-01-08 22:48 ` Thomas Petazzoni 2018-01-09 2:06 ` Ricardo Martincoski 0 siblings, 1 reply; 6+ messages in thread From: Thomas Petazzoni @ 2018-01-08 22:48 UTC (permalink / raw) To: buildroot Ricardo, Do you think you could have a look at the below patch touching checkpackagelib ? It looks OK to me, so unless you complain in the next days, I'll apply. Thanks a lot for your feedback! Thomas On Mon, 18 Dec 2017 13:14:26 +0100, Jerzy Grzegorek wrote: > Signed-off-by: Jerzy Grzegorek <jerzy.m.grzegorek@gmail.com> > --- > utils/checkpackagelib/lib_mk.py | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py > index 817e809..5ae565c 100644 > --- a/utils/checkpackagelib/lib_mk.py > +++ b/utils/checkpackagelib/lib_mk.py > @@ -99,6 +99,25 @@ class PackageHeader(_CheckFunction): > text] > > > +class RemoveDefaultPackageSourceVariable(_CheckFunction): > + PACKAGE_NAME = re.compile("/([^/]+)\.mk") > + > + def before(self): > + package = self.PACKAGE_NAME.search(self.filename).group(1) > + package_upper = package.replace("-", "_").upper() > + self.package = package > + self.package_upper = package_upper > + self.FIND_SOURCE = re.compile( > + "^{}_SOURCE\s*=\s*{}-\$\({}_VERSION\)\.tar\.gz" > + .format(package_upper, package, package_upper)) > + > + def check_line(self, lineno, text): > + if self.FIND_SOURCE.search(text): > + return ["{}:{}: remove default value of _SOURCE variable ({}#writing-rules-mk)" > + .format(self.filename, lineno, self.url_to_manual), > + text] > + > + > class SpaceBeforeBackslash(_CheckFunction): > TAB_OR_MULTIPLE_SPACES_BEFORE_BACKSLASH = re.compile(r"^.*( |\t)\\$") > -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] utils/checkpackagelib: add function to check of the default package source variable 2018-01-08 22:48 ` Thomas Petazzoni @ 2018-01-09 2:06 ` Ricardo Martincoski 2018-01-09 8:50 ` Thomas Petazzoni 2018-01-09 12:51 ` Jerzy Grzegorek 0 siblings, 2 replies; 6+ messages in thread From: Ricardo Martincoski @ 2018-01-09 2:06 UTC (permalink / raw) To: buildroot Hello, On Mon, Jan 08, 2018 at 08:48 PM, Thomas Petazzoni wrote: > Ricardo, > > Do you think you could have a look at the below patch touching > checkpackagelib ? Thomas, Thank you for adding me in CC. > > It looks OK to me, so unless you complain in the next days, I'll apply. Don't apply just yet. See at the end. [snip] > On Mon, 18 Dec 2017 13:14:26 +0100, Jerzy Grzegorek wrote: Jerzy, The code is almost good. Below see some nits about the code. And at the end see my main concerns. I think we will need a whitelist and some patches fixing packages. [snip] >> >> +class RemoveDefaultPackageSourceVariable(_CheckFunction): >> + PACKAGE_NAME = re.compile("/([^/]+)\.mk") >> + >> + def before(self): >> + package = self.PACKAGE_NAME.search(self.filename).group(1) >> + package_upper = package.replace("-", "_").upper() >> + self.package = package >> + self.package_upper = package_upper These 2 lines are not needed because the variables are not used in other methods of this class. >> + self.FIND_SOURCE = re.compile( >> + "^{}_SOURCE\s*=\s*{}-\$\({}_VERSION\)\.tar\.gz" >> + .format(package_upper, package, package_upper)) >> + >> + def check_line(self, lineno, text): >> + if self.FIND_SOURCE.search(text): >> + return ["{}:{}: remove default value of _SOURCE variable ({}#writing-rules-mk)" Instead of #writing-rules-mk maybe a better url would be #generic-package-reference It contains this text for LIBFOO_SOURCE: "If none are specified, then the value is assumed to be libfoo-$(LIBFOO_VERSION).tar.gz" >> + .format(self.filename, lineno, self.url_to_manual), >> + text] >> + >> + >> class SpaceBeforeBackslash(_CheckFunction): >> TAB_OR_MULTIPLE_SPACES_BEFORE_BACKSLASH = re.compile(r"^.*( |\t)\\$") >> Running the new check function in the tree we get warnings from 6 files. https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/47157096 Unrelated... but I see there are few more (other) warnings in the tree. 1) daq A patch fixing this (removing the unneeded variable) ideally should be added to the series because it is tested in gitlab. 2) glibc It's a special package, but the removal of the variable seems fine to me (needs testing of course). 3) python-networkmanager I guess the variable can be removed. Could it interact with scanpypi? Do we care if it does interact? By 'interact' I mean: when someone uses scanpypi to create a package should he/she use check-package after it? For these 2 I am not sure which one is the best solution: fix or whitelist. 4) gdb The variable is overwritten for ARC. Would removing the variable make the code worst in this case? The 'if' would need to be negated, and the non-default value be assigned for not-ARC, I guess. 5) binutils It has '?=' later for the same variable. I am not sure the first assignment can be removed. 6) gcc Maybe we want it to be explicit to ease the maintenance? Not sure. These 3 are good candidates for a whitelist. Regards, Ricardo ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] utils/checkpackagelib: add function to check of the default package source variable 2018-01-09 2:06 ` Ricardo Martincoski @ 2018-01-09 8:50 ` Thomas Petazzoni 2018-01-09 12:54 ` Jerzy Grzegorek 2018-01-09 12:51 ` Jerzy Grzegorek 1 sibling, 1 reply; 6+ messages in thread From: Thomas Petazzoni @ 2018-01-09 8:50 UTC (permalink / raw) To: buildroot Hello, +Yegor in Cc, since there is some scanpypi discussion below. On Tue, 09 Jan 2018 00:06:48 -0200, Ricardo Martincoski wrote: > Unrelated... but I see there are few more (other) warnings in the tree. > > > 1) daq > A patch fixing this (removing the unneeded variable) ideally should be added to > the series because it is tested in gitlab. I fixed this one. > 2) glibc > It's a special package, but the removal of the variable seems fine to me (needs > testing of course). I think it can be changed indeed, but I haven't tested it. > 3) python-networkmanager > I guess the variable can be removed. Could it interact with scanpypi? Do we > care if it does interact? > By 'interact' I mean: when someone uses scanpypi to create a package should > he/she use check-package after it? I fixed this one as well. I guess scanpypi could be improved to not emit the <pkg>_SOURCE line when its value is the default one. > For these 2 I am not sure which one is the best solution: fix or whitelist. Fix :-) > 4) gdb > The variable is overwritten for ARC. Would removing the variable make the code > worst in this case? The 'if' would need to be negated, and the non-default > value be assigned for not-ARC, I guess. > > 5) binutils > It has '?=' later for the same variable. I am not sure the first assignment can > be removed. > > 6) gcc > Maybe we want it to be explicit to ease the maintenance? Not sure. > > These 3 are good candidates for a whitelist. Yes, agreed. Jerzy, could you send an updated patch that takes into account Ricardo's comments, including whitelisting gdb/binutils/gcc ? Thanks! Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] utils/checkpackagelib: add function to check of the default package source variable 2018-01-09 8:50 ` Thomas Petazzoni @ 2018-01-09 12:54 ` Jerzy Grzegorek 0 siblings, 0 replies; 6+ messages in thread From: Jerzy Grzegorek @ 2018-01-09 12:54 UTC (permalink / raw) To: buildroot Hi Thomas, > Hello, > > +Yegor in Cc, since there is some scanpypi discussion below. > > On Tue, 09 Jan 2018 00:06:48 -0200, Ricardo Martincoski wrote: > >> Unrelated... but I see there are few more (other) warnings in the tree. >> >> >> 1) daq >> A patch fixing this (removing the unneeded variable) ideally should be added to >> the series because it is tested in gitlab. > I fixed this one. > >> 2) glibc >> It's a special package, but the removal of the variable seems fine to me (needs >> testing of course). > I think it can be changed indeed, but I haven't tested it. > >> 3) python-networkmanager >> I guess the variable can be removed. Could it interact with scanpypi? Do we >> care if it does interact? >> By 'interact' I mean: when someone uses scanpypi to create a package should >> he/she use check-package after it? > I fixed this one as well. I guess scanpypi could be improved to not > emit the <pkg>_SOURCE line when its value is the default one. > >> For these 2 I am not sure which one is the best solution: fix or whitelist. > Fix :-) > >> 4) gdb >> The variable is overwritten for ARC. Would removing the variable make the code >> worst in this case? The 'if' would need to be negated, and the non-default >> value be assigned for not-ARC, I guess. >> >> 5) binutils >> It has '?=' later for the same variable. I am not sure the first assignment can >> be removed. >> >> 6) gcc >> Maybe we want it to be explicit to ease the maintenance? Not sure. >> >> These 3 are good candidates for a whitelist. > Yes, agreed. Jerzy, could you send an updated patch that takes into > account Ricardo's comments, including whitelisting gdb/binutils/gcc ? Sure, I send it in the next few days. Regards, Jerzy > > Thanks! > > Thomas ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH 1/1] utils/checkpackagelib: add function to check of the default package source variable 2018-01-09 2:06 ` Ricardo Martincoski 2018-01-09 8:50 ` Thomas Petazzoni @ 2018-01-09 12:51 ` Jerzy Grzegorek 1 sibling, 0 replies; 6+ messages in thread From: Jerzy Grzegorek @ 2018-01-09 12:51 UTC (permalink / raw) To: buildroot Hi Ricardo, > Hello, > > On Mon, Jan 08, 2018 at 08:48 PM, Thomas Petazzoni wrote: > >> Ricardo, >> >> Do you think you could have a look at the below patch touching >> checkpackagelib ? > Thomas, > Thank you for adding me in CC. > >> It looks OK to me, so unless you complain in the next days, I'll apply. > Don't apply just yet. See at the end. > > [snip] >> On Mon, 18 Dec 2017 13:14:26 +0100, Jerzy Grzegorek wrote: > Jerzy, > The code is almost good. > Below see some nits about the code. > > And at the end see my main concerns. I think we will need a whitelist and some > patches fixing packages. Thanks for your feedback. > > [snip] >>> >>> +class RemoveDefaultPackageSourceVariable(_CheckFunction): >>> + PACKAGE_NAME = re.compile("/([^/]+)\.mk") >>> + >>> + def before(self): >>> + package = self.PACKAGE_NAME.search(self.filename).group(1) >>> + package_upper = package.replace("-", "_").upper() >>> + self.package = package >>> + self.package_upper = package_upper > These 2 lines are not needed because the variables are not used in other > methods of this class. In fact, you're right. > >>> + self.FIND_SOURCE = re.compile( >>> + "^{}_SOURCE\s*=\s*{}-\$\({}_VERSION\)\.tar\.gz" >>> + .format(package_upper, package, package_upper)) >>> + >>> + def check_line(self, lineno, text): >>> + if self.FIND_SOURCE.search(text): >>> + return ["{}:{}: remove default value of _SOURCE variable ({}#writing-rules-mk)" > Instead of #writing-rules-mk maybe a better url would be > #generic-package-reference > It contains this text for LIBFOO_SOURCE: "If none are specified, then the value > is assumed to be libfoo-$(LIBFOO_VERSION).tar.gz" Will do. > >>> + .format(self.filename, lineno, self.url_to_manual), >>> + text] >>> + >>> + >>> class SpaceBeforeBackslash(_CheckFunction): >>> TAB_OR_MULTIPLE_SPACES_BEFORE_BACKSLASH = re.compile(r"^.*( |\t)\\$") >>> Regards, Jerzy > Running the new check function in the tree we get warnings from 6 files. > https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/47157096 > > Unrelated... but I see there are few more (other) warnings in the tree. > > > 1) daq > A patch fixing this (removing the unneeded variable) ideally should be added to > the series because it is tested in gitlab. > > > 2) glibc > It's a special package, but the removal of the variable seems fine to me (needs > testing of course). > > 3) python-networkmanager > I guess the variable can be removed. Could it interact with scanpypi? Do we > care if it does interact? > By 'interact' I mean: when someone uses scanpypi to create a package should > he/she use check-package after it? > > For these 2 I am not sure which one is the best solution: fix or whitelist. > > > 4) gdb > The variable is overwritten for ARC. Would removing the variable make the code > worst in this case? The 'if' would need to be negated, and the non-default > value be assigned for not-ARC, I guess. > > 5) binutils > It has '?=' later for the same variable. I am not sure the first assignment can > be removed. > > 6) gcc > Maybe we want it to be explicit to ease the maintenance? Not sure. > > These 3 are good candidates for a whitelist. > > > Regards, > Ricardo ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-09 12:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-12-18 12:14 [Buildroot] [PATCH 1/1] utils/checkpackagelib: add function to check of the default package source variable Jerzy Grzegorek 2018-01-08 22:48 ` Thomas Petazzoni 2018-01-09 2:06 ` Ricardo Martincoski 2018-01-09 8:50 ` Thomas Petazzoni 2018-01-09 12:54 ` Jerzy Grzegorek 2018-01-09 12:51 ` Jerzy Grzegorek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox