[1/1] utils/checkpackagelib: add function to check of the default package source variable

Message ID 1513599266-6954-1-git-send-email-jerzy.m.grzegorek@gmail.com
State New
Headers show
Series
  • [1/1] utils/checkpackagelib: add function to check of the default package source variable
Related show

Commit Message

Jerzy Grzegorek Dec. 18, 2017, 12:14 p.m.
Signed-off-by: Jerzy Grzegorek <jerzy.m.grzegorek@gmail.com>
---
 utils/checkpackagelib/lib_mk.py | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Thomas Petazzoni Jan. 8, 2018, 10:48 p.m. | #1
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)\\$")
>
Ricardo Martincoski Jan. 9, 2018, 2:06 a.m. | #2
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
Thomas Petazzoni Jan. 9, 2018, 8:50 a.m. | #3
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
Jerzy Grzegorek Jan. 9, 2018, 12:51 p.m. | #4
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
Jerzy Grzegorek Jan. 9, 2018, 12:54 p.m. | #5
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

Patch

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)\\$")