| Message ID | 1513599266-6954-1-git-send-email-jerzy.m.grzegorek@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] utils/checkpackagelib: add function to check of the default package source variable | expand |
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)\\$") >
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
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
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
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
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)\\$")
Signed-off-by: Jerzy Grzegorek <jerzy.m.grzegorek@gmail.com> --- utils/checkpackagelib/lib_mk.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)