[PATCH 0/2] Minor changes to checkpatches

[PATCH 0/2] Minor changes to checkpatches

[Michael Santana]

Fixed a minor bug with variable assignment, as well as added an
option for checkpatches

Michael Santana (2):
  Enable codespell from config file.
  Fix variable assignment.

 devtools/checkpatches.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletions(-)

-- 
2.20.1

[PATCH 1/2] Enable codespell from config file.

[Michael Santana]

Enable turning on codespell from any of the config files for
checkpatches.sh. codespell is a feature by checkpatch.pl that
checks for common spelling mistakes in comments in patches.

This feature is disabled by default. To enable it one must add
the '--codespell' flag to the $options variable in
checkpatches.sh. With this change the user can decide to turn
on codespell from a config file rather than directly modifying
checkpatches.sh

Signed-off-by: Michael Santana <msantana@redhat.com>
---
 devtools/checkpatches.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 3b03b7ef2..cfe01223b 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -5,6 +5,7 @@
 # Load config options:
 # - DPDK_CHECKPATCH_PATH
 # - DPDK_CHECKPATCH_LINE_LENGTH
+# - DPDK_CHECKPATCH_CODESPELL
 . $(dirname $(readlink -e $0))/load-devel-config
 
 VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
@@ -13,6 +14,9 @@ length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
 
 # override default Linux options
 options="--no-tree"
+if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then
+    options="$options --codespell"
+fi
 options="$options --max-line-length=$length"
 options="$options --show-types"
 options="$options --ignore=LINUX_VERSION_CODE,\
-- 
2.20.1

Re: [PATCH 1/2] Enable codespell from config file.

[Van Haaren, Harry]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michael Santana
> Sent: Wednesday, February 13, 2019 7:08 PM
> To: dev@dpdk.org
> Cc: Thomas Monjalon <thomas@monjalon.net>
> Subject: [dpdk-dev] [PATCH 1/2] Enable codespell from config file.
> 
> Enable turning on codespell from any of the config files for
> checkpatches.sh. codespell is a feature by checkpatch.pl that
> checks for common spelling mistakes in comments in patches.
> 
> This feature is disabled by default. To enable it one must add
> the '--codespell' flag to the $options variable in
> checkpatches.sh. With this change the user can decide to turn
> on codespell from a config file rather than directly modifying
> checkpatches.sh
> 
> Signed-off-by: Michael Santana <msantana@redhat.com>

Oh nice, I didn't know checkpatch had a spell check available.

Would it make sense to turn on automatically if the required spelling program is available?

(Perhaps provide an explicit disable if certain people hate the idea..)

I'm a +1 for tools just doing the right thing by default :)

-H

Re: [PATCH 1/2] Enable codespell from config file.

[Bruce Richardson]

On Wed, Feb 13, 2019 at 07:16:24PM +0000, Van Haaren, Harry wrote:
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michael Santana
> > Sent: Wednesday, February 13, 2019 7:08 PM
> > To: dev@dpdk.org
> > Cc: Thomas Monjalon <thomas@monjalon.net>
> > Subject: [dpdk-dev] [PATCH 1/2] Enable codespell from config file.
> > 
> > Enable turning on codespell from any of the config files for
> > checkpatches.sh. codespell is a feature by checkpatch.pl that
> > checks for common spelling mistakes in comments in patches.
> > 
> > This feature is disabled by default. To enable it one must add
> > the '--codespell' flag to the $options variable in
> > checkpatches.sh. With this change the user can decide to turn
> > on codespell from a config file rather than directly modifying
> > checkpatches.sh
> > 
> > Signed-off-by: Michael Santana <msantana@redhat.com>
> 
> Oh nice, I didn't know checkpatch had a spell check available.
> 
> Would it make sense to turn on automatically if the required spelling program is available?
> 
> (Perhaps provide an explicit disable if certain people hate the idea..)
> 
> I'm a +1 for tools just doing the right thing by default :)
> 
+1 for on by default.

Re: [PATCH 1/2] Enable codespell from config file.

[Rami Rosen]

+1

Reviewed-by: Rami Rosen <ramirose@gmail.com>



>

[PATCH 2/2] Fix variable assignment.

[Michael Santana]

Fix trivial bug. In sh shell, 'foo = 1' is not the same as
'foo=1'. Using 'foo = 1' makes the shell attempt to interpret foo
as a command, rather than a simple variable assignment. 

Signed-off-by: Michael Santana <msantana@redhat.com>
Fixes: dafc04c15174 ("hash: fix out-of-bound write while freeing key slot")
---
 devtools/checkpatches.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index cfe01223b..e01be4046 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -65,7 +65,7 @@ check_forbidden_additions() { # <patch>
 		-v RET_ON_FAIL=1 \
 		-v MESSAGE='Using explicit .svg extension instead of .*' \
 		-f $(dirname $(readlink -e $0))/check-forbidden-tokens.awk \
-		"$1" || res = 1
+		"$1" || res=1
 
 	return $res
 }
-- 
2.20.1

[PATCH v2 0/2] Minor changes to checkpatches

[Michael Santana]

Fixed a minor bug with variable assignment, as well as added an
option for checkpatches

v1->v2:
  Enable codespell by default. Disable via config file

Michael Santana (2):
  Enable codespell by default. Can be disabled from config file.
  Fix variable assignment.

 devtools/checkpatches.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH v2 1/2] Enable codespell by default. Can be disabled from config file.

[Michael Santana]

Enable codespell by default.
codespell is a feature by checkpatch.pl that
checks for common spelling mistakes in patches.

This feature is disabled by default. To enable it one must add
the '--codespell' flag to the $options variable in
checkpatches.sh. With this change codespell is enabled by default.
The user can decide to turn off codespell from a one of the config
files read by checkpatches.sh.

Signed-off-by: Michael Santana <msantana@redhat.com>
Reviewed-by: Rami Rosen <ramirose@gmail.com>
---
 devtools/checkpatches.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 3b03b7ef2..9c2b0a28a 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -2,9 +2,12 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright 2015 6WIND S.A.
 
+# Enable codespell by default. This can be overwritten from a config file.
+DPDK_CHECKPATCH_CODESPELL=enable
 # Load config options:
 # - DPDK_CHECKPATCH_PATH
 # - DPDK_CHECKPATCH_LINE_LENGTH
+# - DPDK_CHECKPATCH_CODESPELL
 . $(dirname $(readlink -e $0))/load-devel-config
 
 VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
@@ -13,6 +16,9 @@ length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
 
 # override default Linux options
 options="--no-tree"
+if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then
+    options="$options --codespell"
+fi
 options="$options --max-line-length=$length"
 options="$options --show-types"
 options="$options --ignore=LINUX_VERSION_CODE,\
-- 
2.20.1

Re: [PATCH v2 1/2] Enable codespell by default. Can be disabled from config file.

[Thomas Monjalon]

14/02/2019 20:35, Michael Santana:
> Enable codespell by default.
> codespell is a feature by checkpatch.pl that
> checks for common spelling mistakes in patches.

What is the difference between codespell and spelling.txt included
with checkpatch?
Is it just a different dictionary?

> This feature is disabled by default. To enable it one must add
> the '--codespell' flag to the $options variable in
> checkpatches.sh.

We need also to specify the dictionary path if not in
/usr/share/codespell/dictionary.txt
In my case, it is in /usr/lib/python3.7/site-packages/codespell_lib/data/dictionary.txt

> With this change codespell is enabled by default.

It seems it is not enabled by default,
because we need DPDK_CHECKPATCH_CODESPELL=enable

> The user can decide to turn off codespell from a one of the config
> files read by checkpatches.sh.
[...]
>  # override default Linux options
>  options="--no-tree"
> +if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then

What about allowing either "enable" or a path?
If it is a path (have some slash), then we can add --codespellfile option.

> +    options="$options --codespell"
> +fi
>  options="$options --max-line-length=$length"
>  options="$options --show-types"
>  options="$options --ignore=LINUX_VERSION_CODE,\
> 

Re: [PATCH v2 1/2] Enable codespell by default. Can be disabled from config file.

[Michael Santana Francisco]

On 2/28/19 6:21 AM, Thomas Monjalon wrote:
> 14/02/2019 20:35, Michael Santana:
>> Enable codespell by default.
>> codespell is a feature by checkpatch.pl that
>> checks for common spelling mistakes in patches.
> What is the difference between codespell and spelling.txt included
> with checkpatch?
> Is it just a different dictionary?
codespell has a larger dictionary, about 15000 word fixes whereas 
spelling.txt has about 1000.
That's really the only big difference
>
>> This feature is disabled by default. To enable it one must add
>> the '--codespell' flag to the $options variable in
>> checkpatches.sh.
> We need also to specify the dictionary path if not in
> /usr/share/codespell/dictionary.txt
> In my case, it is in /usr/lib/python3.7/site-packages/codespell_lib/data/dictionary.txt
>
>> With this change codespell is enabled by default.
> It seems it is not enabled by default,
> because we need DPDK_CHECKPATCH_CODESPELL=enable
V2 sets DPDK_CHECKPATCH_CODESPELL=enable at the beginning of 
checkpatches, right before reading in the config files.
If DPDK_CHECKPATCH_CODESPELL is set in one of the config files it 
overwrites the enabled by default.
This way a user can disable it via a config file
>
>> The user can decide to turn off codespell from a one of the config
>> files read by checkpatches.sh.
> [...]
>>   # override default Linux options
>>   options="--no-tree"
>> +if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then
> What about allowing either "enable" or a path?
> If it is a path (have some slash), then we can add --codespellfile option.
I like your thinking. We can use `if [ -f <file> ]` to see if the path 
given is an existing file.
so, if DPDK_CHECKPATCH_CODESPELL is set to enable, then enable it with 
default path (the way it is right now)
if DPDK_CHECKPATCH_CODESPELL is set to a valid path to a file then 
enable codespell and set --codespellfile to said file
otherwise if it's not set to enable or set to a valid path file, then 
assume it's disabled.
Missed anything?
>
>> +    options="$options --codespell"
>> +fi
>>   options="$options --max-line-length=$length"
>>   options="$options --show-types"
>>   options="$options --ignore=LINUX_VERSION_CODE,\
>>
>
>
>
>

Re: [PATCH v2 1/2] Enable codespell by default. Can be disabled from config file.

[Thomas Monjalon]

28/02/2019 23:09, Michael Santana Francisco:
> On 2/28/19 6:21 AM, Thomas Monjalon wrote:
> > 14/02/2019 20:35, Michael Santana:
> >> Enable codespell by default.
> >> codespell is a feature by checkpatch.pl that
> >> checks for common spelling mistakes in patches.
> > What is the difference between codespell and spelling.txt included
> > with checkpatch?
> > Is it just a different dictionary?
> codespell has a larger dictionary, about 15000 word fixes whereas 
> spelling.txt has about 1000.
> That's really the only big difference

OK

> >> This feature is disabled by default. To enable it one must add
> >> the '--codespell' flag to the $options variable in
> >> checkpatches.sh.
> > We need also to specify the dictionary path if not in
> > /usr/share/codespell/dictionary.txt
> > In my case, it is in /usr/lib/python3.7/site-packages/codespell_lib/data/dictionary.txt
> >
> >> With this change codespell is enabled by default.
> > It seems it is not enabled by default,
> > because we need DPDK_CHECKPATCH_CODESPELL=enable
> V2 sets DPDK_CHECKPATCH_CODESPELL=enable at the beginning of 
> checkpatches, right before reading in the config files.
> If DPDK_CHECKPATCH_CODESPELL is set in one of the config files it 
> overwrites the enabled by default.
> This way a user can disable it via a config file

OK I missed it.

> >> The user can decide to turn off codespell from a one of the config
> >> files read by checkpatches.sh.
> > [...]
> >>   # override default Linux options
> >>   options="--no-tree"
> >> +if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then
> > What about allowing either "enable" or a path?
> > If it is a path (have some slash), then we can add --codespellfile option.
> I like your thinking. We can use `if [ -f <file> ]` to see if the path 
> given is an existing file.
> so, if DPDK_CHECKPATCH_CODESPELL is set to enable, then enable it with 
> default path (the way it is right now)
> if DPDK_CHECKPATCH_CODESPELL is set to a valid path to a file then 
> enable codespell and set --codespellfile to said file
> otherwise if it's not set to enable or set to a valid path file, then 
> assume it's disabled.
> Missed anything?

Well described. Would be nice to see in v3. Thanks

[PATCH v2 2/2] Fix variable assignment.

[Michael Santana]

Fix trivial bug. In sh shell, 'foo = 1' is not the same as
'foo=1'. Using 'foo = 1' makes the shell attempt to interpret foo
as a command, rather than a simple variable assignment.

Signed-off-by: Michael Santana <msantana@redhat.com>
Fixes: dafc04c15174 ("hash: fix out-of-bound write while freeing key slot")
---
v2:
  Nothing changed since v1.

 devtools/checkpatches.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 9c2b0a28a..8852b9412 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -68,7 +68,7 @@ check_forbidden_additions() { # <patch>
 		-v RET_ON_FAIL=1 \
 		-v MESSAGE='Using explicit .svg extension instead of .*' \
 		-f $(dirname $(readlink -e $0))/check-forbidden-tokens.awk \
-		"$1" || res = 1
+		"$1" || res=1
 
 	return $res
 }
-- 
2.20.1

Re: [PATCH v2 2/2] Fix variable assignment.

[Thomas Monjalon]

Hi,

Few minor comments,

The title should start with "devtools:" and not end with dot.
One suggestion:
	devtools: fix result of svg include check

14/02/2019 20:35, Michael Santana:
> Fix trivial bug. In sh shell, 'foo = 1' is not the same as
> 'foo=1'. Using 'foo = 1' makes the shell attempt to interpret foo
> as a command, rather than a simple variable assignment.

Yes, good catch, thanks.

> Signed-off-by: Michael Santana <msantana@redhat.com>
> Fixes: dafc04c15174 ("hash: fix out-of-bound write while freeing key slot")

The reference should be:
Fixes: dafc04c15174 ("devtools: fix return of forbidden addition checks")
and it should be followed by:
Cc: stable@dpdk.org
in order to be backported.

Re: [PATCH v2 0/2] Minor changes to checkpatches

[Bruce Richardson]

On Thu, Feb 14, 2019 at 02:35:45PM -0500, Michael Santana wrote:
> Fixed a minor bug with variable assignment, as well as added an
> option for checkpatches
> 
> v1->v2:
>   Enable codespell by default. Disable via config file
> 
> Michael Santana (2):
>   Enable codespell by default. Can be disabled from config file.
>   Fix variable assignment.
> 
>  devtools/checkpatches.sh | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
Series-Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v3 0/2] Minor changes to checkpatches

[Michael Santana]

Fixed a minor bug with variable assignment, as well as added an
option for checkpatches

v2->v3:
  Also enable codespell by setting a path to a dictionary file.

v1->v2:
  Enable codespell by default. Disable via config file

Michael Santana (2):
  Enable codespell by default. Can be disabled from config file.
  devtools: fix result of svg include check

 devtools/checkpatches.sh | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Series-Acked-by: Bruce Richardson <bruce.richardson@intel.com>

-- 
2.20.1

[PATCH v3 1/2] Enable codespell by default. Can be disabled from config file.

[Michael Santana]

Enable codespell by default.
codespell is a feature by checkpatch.pl that
checks for common spelling mistakes in patches.

This feature is disabled by default. To enable it one must add
the '--codespell' flag to the $options variable in
checkpatches.sh. With this change codespell is enabled by default.
The user can decide to turn off codespell from a one of the config
files read by checkpatches.sh.

Signed-off-by: Michael Santana <msantana@redhat.com>
Reviewed-by: Rami Rosen <ramirose@gmail.com>
---
v2->v3:
  Also enable codespell by setting a path to a dictionary file.

 devtools/checkpatches.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 3b03b7ef2..d3c0b309a 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -2,9 +2,14 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright 2015 6WIND S.A.
 
+# Enable codespell by default. This can be overwritten from a config file.
+# You can also enable codespell by setting DPDK_CHECKPATCH_CODESPELL to a valid path
+# to a dictionary.txt file if your dictionary.txt is not in the default location.
+DPDK_CHECKPATCH_CODESPELL=enable
 # Load config options:
 # - DPDK_CHECKPATCH_PATH
 # - DPDK_CHECKPATCH_LINE_LENGTH
+# - DPDK_CHECKPATCH_CODESPELL
 . $(dirname $(readlink -e $0))/load-devel-config
 
 VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
@@ -13,6 +18,12 @@ length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
 
 # override default Linux options
 options="--no-tree"
+if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then
+    options="$options --codespell"
+elif [ -f "$DPDK_CHECKPATCH_CODESPELL" ]; then
+    options="$options --codespell"
+    options="$options --codespellfile $DPDK_CHECKPATCH_CODESPELL"
+fi
 options="$options --max-line-length=$length"
 options="$options --show-types"
 options="$options --ignore=LINUX_VERSION_CODE,\
-- 
2.20.1

Re: [dpdk-stable] [PATCH v3 1/2] Enable codespell by default. Can be disabled from config file.

[Thomas Monjalon]

01/03/2019 18:08, Michael Santana:
> +# Enable codespell by default. This can be overwritten from a config file.
> +# You can also enable codespell by setting DPDK_CHECKPATCH_CODESPELL to a valid path
> +# to a dictionary.txt file if your dictionary.txt is not in the default location.

Better to avoid "you" form in such comment.

> +DPDK_CHECKPATCH_CODESPELL=enable
>  # Load config options:
>  # - DPDK_CHECKPATCH_PATH
>  # - DPDK_CHECKPATCH_LINE_LENGTH
> +# - DPDK_CHECKPATCH_CODESPELL
>  . $(dirname $(readlink -e $0))/load-devel-config
>  
>  VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
> @@ -13,6 +18,12 @@ length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
>  
>  # override default Linux options
>  options="--no-tree"
> +if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then

This is a bashism.
Standard sh uses a simple =

No need for a v4, I can fix it.

Re: [dpdk-stable] [PATCH v3 1/2] Enable codespell by default. Can be disabled from config file.

[Thomas Monjalon]

01/03/2019 18:43, Thomas Monjalon:
> 01/03/2019 18:08, Michael Santana:
> > +# Enable codespell by default. This can be overwritten from a config file.
> > +# You can also enable codespell by setting DPDK_CHECKPATCH_CODESPELL to a valid path
> > +# to a dictionary.txt file if your dictionary.txt is not in the default location.

This line length won't pass chekpatch ;)

> Better to avoid "you" form in such comment.
> 
> > +DPDK_CHECKPATCH_CODESPELL=enable

It will override the value if passed with an environment variable.
You should do the same as for DPDK_CHECKPATCH_LINE_LENGTH.

> >  # Load config options:
> >  # - DPDK_CHECKPATCH_PATH
> >  # - DPDK_CHECKPATCH_LINE_LENGTH
> > +# - DPDK_CHECKPATCH_CODESPELL
> >  . $(dirname $(readlink -e $0))/load-devel-config
> >  
> >  VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
> > @@ -13,6 +18,12 @@ length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
> >  
> >  # override default Linux options
> >  options="--no-tree"
> > +if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then
> 
> This is a bashism.
> Standard sh uses a simple =
> 
> No need for a v4, I can fix it.

Because of the required change for the env var case,
please do a v4.

Re: [dpdk-stable] [PATCH v3 1/2] Enable codespell by default. Can be disabled from config file.

[Michael Santana Francisco]

On 3/1/19 12:51 PM, Thomas Monjalon wrote:
> 01/03/2019 18:43, Thomas Monjalon:
>> 01/03/2019 18:08, Michael Santana:
>>> +# Enable codespell by default. This can be overwritten from a config file.
>>> +# You can also enable codespell by setting DPDK_CHECKPATCH_CODESPELL to a valid path
>>> +# to a dictionary.txt file if your dictionary.txt is not in the default location.
> This line length won't pass chekpatch ;)
>
>> Better to avoid "you" form in such comment.
>>
>>> +DPDK_CHECKPATCH_CODESPELL=enable
> It will override the value if passed with an environment variable.
> You should do the same as for DPDK_CHECKPATCH_LINE_LENGTH.
If I understood you correctly, you want to be able to set these 
parameters via environment variables (and take precedence over any 
variables set in any config file)
The problem is right now that any environment variable is overwritten by 
the variable being set in one of the config files
The only way I can think of doing this would be by saving the DPDK 
variables (passed via environment) to a file or ironically temporary 
variables (which themselves can also be overwritten, so that doesn't 
really solve the problem) before being overwritten, and then restoring 
said variables after the call to source.
This would add extra clutter in checkpatches, but it can be avoided by 
doing it in load-devel-config instead.

So the bottom line is, environment variables take overall precedence, 
then config files, and then default

Does this sound sane enough?
If anyone knows a better way to do this please share.

I am including in DPDK_CHECKPATCH_PATH, because might as well at this 
point.
>
>>>   # Load config options:
>>>   # - DPDK_CHECKPATCH_PATH
>>>   # - DPDK_CHECKPATCH_LINE_LENGTH
>>> +# - DPDK_CHECKPATCH_CODESPELL
>>>   . $(dirname $(readlink -e $0))/load-devel-config
>>>   
>>>   VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
>>> @@ -13,6 +18,12 @@ length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
>>>   
>>>   # override default Linux options
>>>   options="--no-tree"
>>> +if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then
>> This is a bashism.
>> Standard sh uses a simple =
>>
>> No need for a v4, I can fix it.
> Because of the required change for the env var case,
> please do a v4.
>
>

Re: [dpdk-stable] [PATCH v3 1/2] Enable codespell by default. Can be disabled from config file.

[Thomas Monjalon]

01/03/2019 21:24, Michael Santana Francisco:
> On 3/1/19 12:51 PM, Thomas Monjalon wrote:
> > 01/03/2019 18:43, Thomas Monjalon:
> >> 01/03/2019 18:08, Michael Santana:
> >>> +# Enable codespell by default. This can be overwritten from a config file.
> >>> +# You can also enable codespell by setting DPDK_CHECKPATCH_CODESPELL to a valid path
> >>> +# to a dictionary.txt file if your dictionary.txt is not in the default location.
> > This line length won't pass chekpatch ;)
> >
> >> Better to avoid "you" form in such comment.
> >>
> >>> +DPDK_CHECKPATCH_CODESPELL=enable
> > It will override the value if passed with an environment variable.
> > You should do the same as for DPDK_CHECKPATCH_LINE_LENGTH.
> If I understood you correctly, you want to be able to set these 
> parameters via environment variables (and take precedence over any 
> variables set in any config file)

No :)
The scenario is to have no config file and use environment variables only.

> The problem is right now that any environment variable is overwritten by 
> the variable being set in one of the config files
> The only way I can think of doing this would be by saving the DPDK 
> variables (passed via environment) to a file or ironically temporary 
> variables (which themselves can also be overwritten, so that doesn't 
> really solve the problem) before being overwritten, and then restoring 
> said variables after the call to source.
> This would add extra clutter in checkpatches, but it can be avoided by 
> doing it in load-devel-config instead.
> 
> So the bottom line is, environment variables take overall precedence, 
> then config files, and then default
> 
> Does this sound sane enough?
> If anyone knows a better way to do this please share.

Look how DPDK_CHECKPATCH_LINE_LENGTH is handled.
The default value is used if DPDK_CHECKPATCH_LINE_LENGTH is not set,
neither by environment nor config file.

I think you can just do this after loading config file:

DPDK_CHECKPATCH_CODESPELL=${DPDK_CHECKPATCH_CODESPELL:-enable}

or check for empty value in the test:

[ -z "$DPDK_CHECKPATCH_CODESPELL" -o "$DPDK_CHECKPATCH_CODESPELL" = enable ]


> I am including in DPDK_CHECKPATCH_PATH, because might as well at this 
> point.

Empty DPDK_CHECKPATCH_PATH is already handled.

> >
> >>>   # Load config options:
> >>>   # - DPDK_CHECKPATCH_PATH
> >>>   # - DPDK_CHECKPATCH_LINE_LENGTH
> >>> +# - DPDK_CHECKPATCH_CODESPELL
> >>>   . $(dirname $(readlink -e $0))/load-devel-config
> >>>   
> >>>   VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
> >>> @@ -13,6 +18,12 @@ length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
> >>>   
> >>>   # override default Linux options
> >>>   options="--no-tree"
> >>> +if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then
> >> This is a bashism.
> >> Standard sh uses a simple =
> >>
> >> No need for a v4, I can fix it.
> > Because of the required change for the env var case,
> > please do a v4.

Re: [dpdk-stable] [PATCH v3 1/2] Enable codespell by default. Can be disabled from config file.

[Michael Santana Francisco]

On 3/1/19 4:08 PM, Thomas Monjalon wrote:
> 01/03/2019 21:24, Michael Santana Francisco:
>> On 3/1/19 12:51 PM, Thomas Monjalon wrote:
>>> 01/03/2019 18:43, Thomas Monjalon:
>>>> 01/03/2019 18:08, Michael Santana:
>>>>> +# Enable codespell by default. This can be overwritten from a config file.
>>>>> +# You can also enable codespell by setting DPDK_CHECKPATCH_CODESPELL to a valid path
>>>>> +# to a dictionary.txt file if your dictionary.txt is not in the default location.
>>> This line length won't pass chekpatch ;)
>>>
>>>> Better to avoid "you" form in such comment.
>>>>
>>>>> +DPDK_CHECKPATCH_CODESPELL=enable
>>> It will override the value if passed with an environment variable.
>>> You should do the same as for DPDK_CHECKPATCH_LINE_LENGTH.
>> If I understood you correctly, you want to be able to set these
>> parameters via environment variables (and take precedence over any
>> variables set in any config file)
> No :)
> The scenario is to have no config file and use environment variables only.
>
>> The problem is right now that any environment variable is overwritten by
>> the variable being set in one of the config files
>> The only way I can think of doing this would be by saving the DPDK
>> variables (passed via environment) to a file or ironically temporary
>> variables (which themselves can also be overwritten, so that doesn't
>> really solve the problem) before being overwritten, and then restoring
>> said variables after the call to source.
>> This would add extra clutter in checkpatches, but it can be avoided by
>> doing it in load-devel-config instead.
>>
>> So the bottom line is, environment variables take overall precedence,
>> then config files, and then default
>>
>> Does this sound sane enough?
>> If anyone knows a better way to do this please share.
> Look how DPDK_CHECKPATCH_LINE_LENGTH is handled.
> The default value is used if DPDK_CHECKPATCH_LINE_LENGTH is not set,
> neither by environment nor config file.
>
> I think you can just do this after loading config file:
>
> DPDK_CHECKPATCH_CODESPELL=${DPDK_CHECKPATCH_CODESPELL:-enable}
Oh! So it's much simpler than I thought. Yeah. I will remove what I have 
now and put in your line to go along with the length= line. Will also 
change the comment as I didn't remove the 'you' form in the last commit
>
> or check for empty value in the test:
>
> [ -z "$DPDK_CHECKPATCH_CODESPELL" -o "$DPDK_CHECKPATCH_CODESPELL" = enable ]
>
>
>> I am including in DPDK_CHECKPATCH_PATH, because might as well at this
>> point.
> Empty DPDK_CHECKPATCH_PATH is already handled.
>
>>>>>    # Load config options:
>>>>>    # - DPDK_CHECKPATCH_PATH
>>>>>    # - DPDK_CHECKPATCH_LINE_LENGTH
>>>>> +# - DPDK_CHECKPATCH_CODESPELL
>>>>>    . $(dirname $(readlink -e $0))/load-devel-config
>>>>>    
>>>>>    VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
>>>>> @@ -13,6 +18,12 @@ length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
>>>>>    
>>>>>    # override default Linux options
>>>>>    options="--no-tree"
>>>>> +if [ "$DPDK_CHECKPATCH_CODESPELL" == "enable" ]; then
>>>> This is a bashism.
>>>> Standard sh uses a simple =
>>>>
>>>> No need for a v4, I can fix it.
>>> Because of the required change for the env var case,
>>> please do a v4.
>
>

[PATCH v3 2/2] devtools: fix result of svg include check

[Michael Santana]

Fix trivial bug. In sh shell, 'foo = 1' is not the same as
'foo=1'. Using 'foo = 1' makes the shell attempt to interpret foo
as a command, rather than a simple variable assignment.

Signed-off-by: Michael Santana <msantana@redhat.com>
Fixes: dafc04c15174 ("devtools: fix return of forbidden addition checks")
---
 devtools/checkpatches.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index d3c0b309a..16b705384 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -73,7 +73,7 @@ check_forbidden_additions() { # <patch>
 		-v RET_ON_FAIL=1 \
 		-v MESSAGE='Using explicit .svg extension instead of .*' \
 		-f $(dirname $(readlink -e $0))/check-forbidden-tokens.awk \
-		"$1" || res = 1
+		"$1" || res=1
 
 	return $res
 }
-- 
2.20.1

Re: [dpdk-stable] [PATCH v3 2/2] devtools: fix result of svg include check

[Thomas Monjalon]

01/03/2019 18:08, Michael Santana:
> Fix trivial bug. In sh shell, 'foo = 1' is not the same as
> 'foo=1'. Using 'foo = 1' makes the shell attempt to interpret foo
> as a command, rather than a simple variable assignment.
> 
> Signed-off-by: Michael Santana <msantana@redhat.com>
> Fixes: dafc04c15174 ("devtools: fix return of forbidden addition checks")

CC: stable@dpdk.org must be inserted below Fixes for the record in git.

Note also that Signed-off-by must be below with a blank line.

Re: [dpdk-stable] [PATCH v3 2/2] devtools: fix result of svg include check

[David Marchand]

On Fri, Mar 1, 2019 at 6:52 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> 01/03/2019 18:08, Michael Santana:
> > Fix trivial bug. In sh shell, 'foo = 1' is not the same as
> > 'foo=1'. Using 'foo = 1' makes the shell attempt to interpret foo
> > as a command, rather than a simple variable assignment.
> >
> > Signed-off-by: Michael Santana <msantana@redhat.com>
> > Fixes: dafc04c15174 ("devtools: fix return of forbidden addition checks")
>
> CC: stable@dpdk.org must be inserted below Fixes for the record in git.
>
> Note also that Signed-off-by must be below with a blank line.
>

Ah ah, great that we won't have to check this manually soon :-)

Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

[PATCH v4 0/2] Minor changes to checkpatches

[Michael Santana]

Fixed a minor bug with variable assignment, as well as added an
option for checkpatches

v3->v4:
  Support enabling codespell via environment variable.

v2->v3:
  Also enable codespell by setting a path to a dictionary file.

v1->v2:
  Enable codespell by default. Disable via config file

Michael Santana (2):
  Enable codespell by default. Can be disabled from config file.
  devtools: fix result of svg include check

 devtools/checkpatches.sh | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Series-Acked-by: Bruce Richardson <bruce.richardson@intel.com>
-- 
2.20.1

[PATCH v4 1/2] Enable codespell by default. Can be disabled from config file.

[Michael Santana]

Enable codespell by default.
codespell is a feature by checkpatch.pl that
checks for common spelling mistakes in patches.

This feature is disabled by default. To enable it one must add
the '--codespell' flag to the $options variable in
checkpatches.sh. With this change codespell is enabled by default.
The user can decide to turn off codespell from a one of the config
files read by checkpatches.sh.

Signed-off-by: Michael Santana <msantana@redhat.com>
Reviewed-by: Rami Rosen <ramirose@gmail.com>
---
v3->v4:
  Support enabling codespell via environment variable.

 devtools/checkpatches.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 3b03b7ef2..7e862e0a4 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -5,14 +5,25 @@
 # Load config options:
 # - DPDK_CHECKPATCH_PATH
 # - DPDK_CHECKPATCH_LINE_LENGTH
+# - DPDK_CHECKPATCH_CODESPELL
 . $(dirname $(readlink -e $0))/load-devel-config
 
 VALIDATE_NEW_API=$(dirname $(readlink -e $0))/check-symbol-change.sh
 
+# Enable codespell by default. This can be overwritten from a config file.
+# Codespell can also be enabled by setting DPDK_CHECKPATCH_CODESPELL to a valid path
+# to a dictionary.txt file if dictionary.txt is not in the default location.
+codespell=${DPDK_CHECKPATCH_CODESPELL:-enable}
 length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}
 
 # override default Linux options
 options="--no-tree"
+if [ "$codespell" = "enable" ]; then
+    options="$options --codespell"
+elif [ -f "$codespell" ]; then
+    options="$options --codespell"
+    options="$options --codespellfile $codespell"
+fi
 options="$options --max-line-length=$length"
 options="$options --show-types"
 options="$options --ignore=LINUX_VERSION_CODE,\
-- 
2.20.1

[PATCH v4 2/2] devtools: fix result of svg include check

[Michael Santana]

Fix trivial bug. In sh shell, 'foo = 1' is not the same as
'foo=1'. Using 'foo = 1' makes the shell attempt to interpret foo
as a command, rather than a simple variable assignment.

Fixes: dafc04c15174 ("devtools: fix return of forbidden addition checks")
Cc: stable@dpdk.org

Signed-off-by: Michael Santana <msantana@redhat.com>
---
 devtools/checkpatches.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 7e862e0a4..521534fce 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -73,7 +73,7 @@ check_forbidden_additions() { # <patch>
 		-v RET_ON_FAIL=1 \
 		-v MESSAGE='Using explicit .svg extension instead of .*' \
 		-f $(dirname $(readlink -e $0))/check-forbidden-tokens.awk \
-		"$1" || res = 1
+		"$1" || res=1
 
 	return $res
 }
-- 
2.20.1

Re: [dpdk-stable] [PATCH v4 0/2] Minor changes to checkpatches

[Thomas Monjalon]

04/03/2019 20:07, Michael Santana:
> Fixed a minor bug with variable assignment, as well as added an
> option for checkpatches
> 
> v3->v4:
>   Support enabling codespell via environment variable.
> 
> v2->v3:
>   Also enable codespell by setting a path to a dictionary file.
> 
> v1->v2:
>   Enable codespell by default. Disable via config file
> 
> Michael Santana (2):
>   Enable codespell by default. Can be disabled from config file.
>   devtools: fix result of svg include check
> 
>  devtools/checkpatches.sh | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> Series-Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Applied, thanks