* [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 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
* [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
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