[dpdk-dev] [PATCH] drivers: fix indentation in build files

[dpdk-dev] [PATCH] drivers: fix indentation in build files

[Thomas Monjalon]

A couple of mistakes slipped in the mass change.

More mistakes could happen, especially when rebasing pending patches,
so we need an automatic check.

Fixes: 4ad4b20a7905 ("drivers: change indentation in build files")

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 drivers/crypto/qat/meson.build   | 4 ++--
 drivers/raw/skeleton/meson.build | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/qat/meson.build b/drivers/crypto/qat/meson.build
index 4da819d65a..544f481067 100644
--- a/drivers/crypto/qat/meson.build
+++ b/drivers/crypto/qat/meson.build
@@ -17,9 +17,9 @@ if dep.found()
             'qat_asym_pmd.c',
             'qat_sym.c',
             'qat_sym_hw_dp.c',
-			'qat_sym_pmd.c',
+            'qat_sym_pmd.c',
             'qat_sym_session.c',
-	)
+    )
     qat_ext_deps += dep
     qat_cflags += '-DBUILD_QAT_SYM'
     qat_cflags += '-DBUILD_QAT_ASYM'
diff --git a/drivers/raw/skeleton/meson.build b/drivers/raw/skeleton/meson.build
index b5d87652f5..950a33cc20 100644
--- a/drivers/raw/skeleton/meson.build
+++ b/drivers/raw/skeleton/meson.build
@@ -3,6 +3,6 @@
 
 deps += ['rawdev', 'kvargs', 'mbuf', 'bus_vdev']
 sources = files(
-		'skeleton_rawdev.c',
+        'skeleton_rawdev.c',
         'skeleton_rawdev_test.c',
 )
-- 
2.31.1

Re: [dpdk-dev] [PATCH] drivers: fix indentation in build files

[Bruce Richardson]

On Thu, Apr 22, 2021 at 12:03:57AM +0200, Thomas Monjalon wrote:
> A couple of mistakes slipped in the mass change.
> 
> More mistakes could happen, especially when rebasing pending patches, so
> we need an automatic check.
>
I have a partially-done script from when I was doing the original
reindenting work, which I'll clean up a little and send on shortly.

It only checks indent of lists and I'm extending it to check for trailing
commas, but it may serve as a basis for further checks. On the plus side,
it does automatic fixing of those issues though. Running it will probably
show up more minor misses, so I'd suggest holding off on this patch until
then.

Regards,
/Bruce

Re: [dpdk-dev] [PATCH] drivers: fix indentation in build files

[Thomas Monjalon]

22/04/2021 10:39, Bruce Richardson:
> On Thu, Apr 22, 2021 at 12:03:57AM +0200, Thomas Monjalon wrote:
> > A couple of mistakes slipped in the mass change.
> > 
> > More mistakes could happen, especially when rebasing pending patches, so
> > we need an automatic check.
> >
> I have a partially-done script from when I was doing the original
> reindenting work, which I'll clean up a little and send on shortly.
> 
> It only checks indent of lists and I'm extending it to check for trailing
> commas, but it may serve as a basis for further checks. On the plus side,
> it does automatic fixing of those issues though. Running it will probably
> show up more minor misses, so I'd suggest holding off on this patch until
> then.

OK, I've sent this patch just for remembering.
I am happy to leave it to you for a v2 :)
Feel free to change the author of course.

Note: I've found tabs with this simple grep:
	git grep ' ' '**/meson.build'
The tab is inserted with ^V-tab.

Re: [dpdk-dev] [PATCH] drivers: fix indentation in build files

[Bruce Richardson]

On Thu, Apr 22, 2021 at 11:20:19AM +0200, Thomas Monjalon wrote:
> 22/04/2021 10:39, Bruce Richardson:
> > On Thu, Apr 22, 2021 at 12:03:57AM +0200, Thomas Monjalon wrote:
> > > A couple of mistakes slipped in the mass change.
> > > 
> > > More mistakes could happen, especially when rebasing pending patches, so
> > > we need an automatic check.
> > >
> > I have a partially-done script from when I was doing the original
> > reindenting work, which I'll clean up a little and send on shortly.
> > 
> > It only checks indent of lists and I'm extending it to check for trailing
> > commas, but it may serve as a basis for further checks. On the plus side,
> > it does automatic fixing of those issues though. Running it will probably
> > show up more minor misses, so I'd suggest holding off on this patch until
> > then.
> 
> OK, I've sent this patch just for remembering.
> I am happy to leave it to you for a v2 :)
> Feel free to change the author of course.
> 
> Note: I've found tabs with this simple grep:
> 	git grep ' ' '**/meson.build'
> The tab is inserted with ^V-tab.
> 
I've included fixes for these issues in V2 set of the python script to
check for them.

http://patches.dpdk.org/project/dpdk/patch/20210426105403.226004-2-bruce.richardson@intel.com/

[dpdk-dev] [RFC PATCH] devtools: script to check meson indentation of lists

[Bruce Richardson]

This is a draft script developed when I was working on the whitespace rework
changes, since extended a little to attempt to fix some trailing comma issues.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 devtools/dpdk_meson_check.py | 106 +++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)
 create mode 100755 devtools/dpdk_meson_check.py

diff --git a/devtools/dpdk_meson_check.py b/devtools/dpdk_meson_check.py
new file mode 100755
index 000000000..dc4c714ad
--- /dev/null
+++ b/devtools/dpdk_meson_check.py
@@ -0,0 +1,106 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2021 Intel Corporation
+
+'''
+A Python script to run some checks on meson.build files in DPDK
+'''
+
+import sys
+import os
+from os.path import relpath, join
+from argparse import ArgumentParser
+
+VERBOSE = False
+FIX = False
+
+def scan_dir(path):
+    '''return meson.build files found in path'''
+    for root, dirs, files in os.walk(path):
+        if 'meson.build' in files:
+            yield(relpath(join(root, 'meson.build')))
+
+
+def check_indentation(filename, contents):
+    '''check that a list or files() is correctly indented'''
+    infiles = False
+    inlist = False
+    edit_count = 0
+    for lineno in range(len(contents)):
+        line = contents[lineno].rstrip()
+        if not line:
+            continue
+        if line.endswith('files('):
+            if infiles:
+                raise(f'Error parsing {filename}:{lineno}, got "files(" when already parsing files list')
+            if inlist:
+                print(f'Error parsing {filename}:{lineno}, got "files(" when already parsing array list')
+            infiles = True
+            indent = 0
+            while line[indent] == ' ':
+                indent += 1
+            indent += 8  # double indent required
+        elif line.endswith('= ['):
+            if infiles:
+                raise(f'Error parsing {filename}:{lineno}, got start of array when already parsing files list')
+            if inlist:
+                print(f'Error parsing {filename}:{lineno}, got start of array when already parsing array list')
+            inlist = True
+            indent = 0
+            while line[indent] == ' ':
+                indent += 1
+            indent += 8  # double indent required
+        elif infiles and (line.endswith(')') or line.strip().startswith(')')):
+            infiles = False
+            continue
+        elif inlist and line.endswith(']') or line.strip().startswith(']'):
+            inlist = False
+            continue
+        elif inlist or infiles:
+            # skip further subarrays or lists
+            if '[' in line  or ']' in line:
+                continue
+            if not line.startswith(' ' * indent) or line[indent] == ' ':
+                print(f'Error: Incorrect indent at {filename}:{lineno + 1}')
+                contents[lineno] = (' ' * indent) + line.strip() + '\n'
+                line = contents[lineno].rstrip()
+                edit_count += 1
+            if not line.endswith(',') and '#' not in line:
+                # TODO: support stripping comment and adding ','
+                print(f'Error: Missing trailing "," in list at {filename}:{lineno + 1}')
+                contents[lineno] = line + ',\n'
+                line = contents[lineno].rstrip()
+                edit_count += 1
+    return edit_count
+
+
+def process_file(filename):
+    '''run checks on file "filename"'''
+    if VERBOSE:
+        print(f'Processing {filename}')
+    with open(filename) as f:
+        contents = f.readlines()
+
+    if check_indentation(filename, contents) > 0 and FIX:
+        print(f"Fixing {filename}")
+        with open(filename, 'w') as f:
+            f.writelines(contents)
+
+
+def main():
+    '''parse arguments and then call other functions to do work'''
+    global VERBOSE
+    global FIX
+    parser = ArgumentParser(description='Run syntax checks on DPDK meson.build files')
+    parser.add_argument('-d', metavar='directory', default='.', help='Directory to process')
+    parser.add_argument('--fix', action='store_true', help='Attempt to fix errors')
+    parser.add_argument('-v', action='store_true', help='Verbose output')
+    args = parser.parse_args()
+
+    VERBOSE = args.v
+    FIX = args.fix
+    for f in scan_dir(args.d):
+        process_file(f)
+
+if __name__ == "__main__":
+    main()
--
2.27.0

Re: [dpdk-dev] [RFC PATCH] devtools: script to check meson indentation of lists

[Burakov, Anatoly]

On 22-Apr-21 10:02 AM, Bruce Richardson wrote:
> This is a draft script developed when I was working on the whitespace rework
> changes, since extended a little to attempt to fix some trailing comma issues.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   devtools/dpdk_meson_check.py | 106 +++++++++++++++++++++++++++++++++++
>   1 file changed, 106 insertions(+)
>   create mode 100755 devtools/dpdk_meson_check.py
> 
> diff --git a/devtools/dpdk_meson_check.py b/devtools/dpdk_meson_check.py
> new file mode 100755
> index 000000000..dc4c714ad
> --- /dev/null
> +++ b/devtools/dpdk_meson_check.py
> @@ -0,0 +1,106 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2021 Intel Corporation
> +
> +'''
> +A Python script to run some checks on meson.build files in DPDK
> +'''
> +
> +import sys
> +import os
> +from os.path import relpath, join
> +from argparse import ArgumentParser
> +
> +VERBOSE = False
> +FIX = False
> +
> +def scan_dir(path):
> +    '''return meson.build files found in path'''
> +    for root, dirs, files in os.walk(path):
> +        if 'meson.build' in files:
> +            yield(relpath(join(root, 'meson.build')))
> +
> +
> +def check_indentation(filename, contents):
> +    '''check that a list or files() is correctly indented'''
> +    infiles = False
> +    inlist = False
> +    edit_count = 0
> +    for lineno in range(len(contents)):

for lineno, line in enumerate(contents)

?

> +        line = contents[lineno].rstrip()
> +        if not line:
> +            continue
> +        if line.endswith('files('):
> +            if infiles:
> +                raise(f'Error parsing {filename}:{lineno}, got "files(" when already parsing files list')
> +            if inlist:
> +                print(f'Error parsing {filename}:{lineno}, got "files(" when already parsing array list')
> +            infiles = True
> +            indent = 0
> +            while line[indent] == ' ':
> +                indent += 1

Here and in other places, if this is measuring length of indent, maybe 
do something like:

indent = len(line) - len(line.lstrip(' '))

?

> +            indent += 8  # double indent required
> +        elif line.endswith('= ['):
> +            if infiles:
> +                raise(f'Error parsing {filename}:{lineno}, got start of array when already parsing files list')
> +            if inlist:
> +                print(f'Error parsing {filename}:{lineno}, got start of array when already parsing array list')
> +            inlist = True
> +            indent = 0
> +            while line[indent] == ' ':
> +                indent += 1
> +            indent += 8  # double indent required
> +        elif infiles and (line.endswith(')') or line.strip().startswith(')')):

It's kinda hard to read with all the endswith/startswith, maybe extract 
those into a function? e.g. 'elif infiles and is_file_start(line)'

> +            infiles = False
> +            continue
> +        elif inlist and line.endswith(']') or line.strip().startswith(']'):
> +            inlist = False
> +            continue
> +        elif inlist or infiles:
> +            # skip further subarrays or lists
> +            if '[' in line  or ']' in line:
> +                continue

I guess you could make it recursive instead of giving up? Does this 
happen with any kind of regularity?

> +            if not line.startswith(' ' * indent) or line[indent] == ' ':
> +                print(f'Error: Incorrect indent at {filename}:{lineno + 1}')
> +                contents[lineno] = (' ' * indent) + line.strip() + '\n'
> +                line = contents[lineno].rstrip()
> +                edit_count += 1
> +            if not line.endswith(',') and '#' not in line:
> +                # TODO: support stripping comment and adding ','
> +                print(f'Error: Missing trailing "," in list at {filename}:{lineno + 1}')
> +                contents[lineno] = line + ',\n'
> +                line = contents[lineno].rstrip()

What is the point of setting `line` here?

> +                edit_count += 1
> +    return edit_count
> +
> +
> +def process_file(filename):
> +    '''run checks on file "filename"'''
> +    if VERBOSE:
> +        print(f'Processing {filename}')
> +    with open(filename) as f:
> +        contents = f.readlines()

I guess meson build files don't get too big so it's OK to read the 
entire file in memory and then work on it, rather than go line by line...

> +
> +    if check_indentation(filename, contents) > 0 and FIX:
> +        print(f"Fixing {filename}")
> +        with open(filename, 'w') as f:
> +            f.writelines(contents)
> +
> +
> +def main():
> +    '''parse arguments and then call other functions to do work'''
> +    global VERBOSE
> +    global FIX

Seems like globals are unnecessary here when you can just pass them into 
process_file?

> +    parser = ArgumentParser(description='Run syntax checks on DPDK meson.build files')
> +    parser.add_argument('-d', metavar='directory', default='.', help='Directory to process')
> +    parser.add_argument('--fix', action='store_true', help='Attempt to fix errors')
> +    parser.add_argument('-v', action='store_true', help='Verbose output')
> +    args = parser.parse_args()
> +
> +    VERBOSE = args.v
> +    FIX = args.fix
> +    for f in scan_dir(args.d):
> +        process_file(f)
> +
> +if __name__ == "__main__":
> +    main()
> --
> 2.27.0
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [RFC PATCH] devtools: script to check meson indentation of lists

[Bruce Richardson]

On Thu, Apr 22, 2021 at 10:40:37AM +0100, Burakov, Anatoly wrote:
> On 22-Apr-21 10:02 AM, Bruce Richardson wrote:
> > This is a draft script developed when I was working on the whitespace rework
> > changes, since extended a little to attempt to fix some trailing comma issues.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >   devtools/dpdk_meson_check.py | 106 +++++++++++++++++++++++++++++++++++
> >   1 file changed, 106 insertions(+)
> >   create mode 100755 devtools/dpdk_meson_check.py
> > 
> > diff --git a/devtools/dpdk_meson_check.py b/devtools/dpdk_meson_check.py
> > new file mode 100755
> > index 000000000..dc4c714ad
> > --- /dev/null
> > +++ b/devtools/dpdk_meson_check.py
> > @@ -0,0 +1,106 @@
> > +#!/usr/bin/env python3
> > +# SPDX-License-Identifier: BSD-3-Clause
> > +# Copyright(c) 2021 Intel Corporation
> > +
> > +'''
> > +A Python script to run some checks on meson.build files in DPDK
> > +'''
> > +
> > +import sys
> > +import os
> > +from os.path import relpath, join
> > +from argparse import ArgumentParser
> > +
> > +VERBOSE = False
> > +FIX = False
> > +
> > +def scan_dir(path):
> > +    '''return meson.build files found in path'''
> > +    for root, dirs, files in os.walk(path):
> > +        if 'meson.build' in files:
> > +            yield(relpath(join(root, 'meson.build')))
> > +
> > +
> > +def check_indentation(filename, contents):
> > +    '''check that a list or files() is correctly indented'''
> > +    infiles = False
> > +    inlist = False
> > +    edit_count = 0
> > +    for lineno in range(len(contents)):
> 
> for lineno, line in enumerate(contents)
> 
Yep, that's a good idea. Wasn't aware of enumerate. [Learn something new
every day, eh? :-)]

> ?
> 
> > +        line = contents[lineno].rstrip()
> > +        if not line:
> > +            continue
> > +        if line.endswith('files('):
> > +            if infiles:
> > +                raise(f'Error parsing {filename}:{lineno}, got "files(" when already parsing files list')
> > +            if inlist:
> > +                print(f'Error parsing {filename}:{lineno}, got "files(" when already parsing array list')
> > +            infiles = True
> > +            indent = 0
> > +            while line[indent] == ' ':
> > +                indent += 1
> 
> Here and in other places, if this is measuring length of indent, maybe do
> something like:
> 
> indent = len(line) - len(line.lstrip(' '))
> 
Yep, that is cleaner

> ?
> 
> > +            indent += 8  # double indent required
> > +        elif line.endswith('= ['):
> > +            if infiles:
> > +                raise(f'Error parsing {filename}:{lineno}, got start of array when already parsing files list')
> > +            if inlist:
> > +                print(f'Error parsing {filename}:{lineno}, got start of array when already parsing array list')
> > +            inlist = True
> > +            indent = 0
> > +            while line[indent] == ' ':
> > +                indent += 1
> > +            indent += 8  # double indent required
> > +        elif infiles and (line.endswith(')') or line.strip().startswith(')')):
> 
> It's kinda hard to read with all the endswith/startswith, maybe extract
> those into a function? e.g. 'elif infiles and is_file_start(line)'
> 
It is all a bit of a mess, yes, and needs cleaning up - which is why I
didn't previously send it with the patchset. Splitting into functions is
something I'll look at.

> > +            infiles = False
> > +            continue
> > +        elif inlist and line.endswith(']') or line.strip().startswith(']'):
> > +            inlist = False
> > +            continue
> > +        elif inlist or infiles:
> > +            # skip further subarrays or lists
> > +            if '[' in line  or ']' in line:
> > +                continue
> 
> I guess you could make it recursive instead of giving up? Does this happen
> with any kind of regularity?
> 
No, not that much, which is why I haven't explicitly tried to deal with it.
It would be good to support in future.

> > +            if not line.startswith(' ' * indent) or line[indent] == ' ':
> > +                print(f'Error: Incorrect indent at {filename}:{lineno + 1}')
> > +                contents[lineno] = (' ' * indent) + line.strip() + '\n'
> > +                line = contents[lineno].rstrip()
> > +                edit_count += 1
> > +            if not line.endswith(',') and '#' not in line:
> > +                # TODO: support stripping comment and adding ','
> > +                print(f'Error: Missing trailing "," in list at {filename}:{lineno + 1}')
> > +                contents[lineno] = line + ',\n'
> > +                line = contents[lineno].rstrip()
> 
> What is the point of setting `line` here?
> 
Because it allows us to make further checks using "line" as we add them
later.
The one question is whether it's worth using line as a shortcut for
contents[lineno] or not. I'd tend to keep it, as it also allows us not to
worry about the '\n' at the end. Then again, other rework might just change
the whole script to strip off all the '\n' post-read and add them back
again pre-write.

Again, further cleanup work.

> > +                edit_count += 1
> > +    return edit_count
> > +
> > +
> > +def process_file(filename):
> > +    '''run checks on file "filename"'''
> > +    if VERBOSE:
> > +        print(f'Processing {filename}')
> > +    with open(filename) as f:
> > +        contents = f.readlines()
> 
> I guess meson build files don't get too big so it's OK to read the entire
> file in memory and then work on it, rather than go line by line...
> 
This was a deliberate choice when starting the script. For now the script
only checks indentation and formatting of lists, but ideally in future it
should check other things, e.g. alphabetical order of lists, or formatting
of other parts. Rather than checking it all in one go, the script is
structured so that we can call multiple functions with "contents" each of
which does the processing without constantly re-reading and rewriting the
file. Something like sorting a list alphabetically also requires changing
multiple lines at a time, which is easier to do with lists that when
streaming input.

> > +
> > +    if check_indentation(filename, contents) > 0 and FIX:
> > +        print(f"Fixing {filename}")
> > +        with open(filename, 'w') as f:
> > +            f.writelines(contents)
> > +
> > +
> > +def main():
> > +    '''parse arguments and then call other functions to do work'''
> > +    global VERBOSE
> > +    global FIX
> 
> Seems like globals are unnecessary here when you can just pass them into
> process_file?
> 
Yes, they can just be passed, but I actually prefer to have those as
globals rather than having larger parameter lists. It's also possible that
e.g. verbose, should need to be passed through multiple levels of functions.
Personal preference, though really.

> > +    parser = ArgumentParser(description='Run syntax checks on DPDK meson.build files')
> > +    parser.add_argument('-d', metavar='directory', default='.', help='Directory to process')
> > +    parser.add_argument('--fix', action='store_true', help='Attempt to fix errors')
> > +    parser.add_argument('-v', action='store_true', help='Verbose output')
> > +    args = parser.parse_args()
> > +
> > +    VERBOSE = args.v
> > +    FIX = args.fix
> > +    for f in scan_dir(args.d):
> > +        process_file(f)
> > +
> > +if __name__ == "__main__":
> > +    main()
> > --
> > 2.27.0
> > 
> 
> 
> -- 
> Thanks,

Thanks for the review Anatoly.

/Bruce

Re: [dpdk-dev] [RFC PATCH] devtools: script to check meson indentation of lists

[Burakov, Anatoly]

On 22-Apr-21 10:58 AM, Bruce Richardson wrote:
> On Thu, Apr 22, 2021 at 10:40:37AM +0100, Burakov, Anatoly wrote:
>> On 22-Apr-21 10:02 AM, Bruce Richardson wrote:
>>> This is a draft script developed when I was working on the whitespace rework
>>> changes, since extended a little to attempt to fix some trailing comma issues.
>>>
>>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>>> ---
>>>    devtools/dpdk_meson_check.py | 106 +++++++++++++++++++++++++++++++++++
>>>    1 file changed, 106 insertions(+)
>>>    create mode 100755 devtools/dpdk_meson_check.py
>>>
>>> diff --git a/devtools/dpdk_meson_check.py b/devtools/dpdk_meson_check.py
>>> new file mode 100755
>>> index 000000000..dc4c714ad
>>> --- /dev/null
>>> +++ b/devtools/dpdk_meson_check.py
>>> @@ -0,0 +1,106 @@
>>> +#!/usr/bin/env python3
>>> +# SPDX-License-Identifier: BSD-3-Clause
>>> +# Copyright(c) 2021 Intel Corporation
>>> +
>>> +'''
>>> +A Python script to run some checks on meson.build files in DPDK
>>> +'''
>>> +
>>> +import sys
>>> +import os
>>> +from os.path import relpath, join
>>> +from argparse import ArgumentParser
>>> +
>>> +VERBOSE = False
>>> +FIX = False
>>> +
>>> +def scan_dir(path):
>>> +    '''return meson.build files found in path'''
>>> +    for root, dirs, files in os.walk(path):
>>> +        if 'meson.build' in files:
>>> +            yield(relpath(join(root, 'meson.build')))
>>> +
>>> +
>>> +def check_indentation(filename, contents):
>>> +    '''check that a list or files() is correctly indented'''
>>> +    infiles = False
>>> +    inlist = False
>>> +    edit_count = 0
>>> +    for lineno in range(len(contents)):
>>
>> for lineno, line in enumerate(contents)
>>
> Yep, that's a good idea. Wasn't aware of enumerate. [Learn something new
> every day, eh? :-)]
> 
>> ?
>>
>>> +        line = contents[lineno].rstrip()
>>> +        if not line:
>>> +            continue
>>> +        if line.endswith('files('):
>>> +            if infiles:
>>> +                raise(f'Error parsing {filename}:{lineno}, got "files(" when already parsing files list')
>>> +            if inlist:
>>> +                print(f'Error parsing {filename}:{lineno}, got "files(" when already parsing array list')
>>> +            infiles = True
>>> +            indent = 0
>>> +            while line[indent] == ' ':
>>> +                indent += 1
>>
>> Here and in other places, if this is measuring length of indent, maybe do
>> something like:
>>
>> indent = len(line) - len(line.lstrip(' '))
>>
> Yep, that is cleaner
> 
>> ?
>>
>>> +            indent += 8  # double indent required
>>> +        elif line.endswith('= ['):
>>> +            if infiles:
>>> +                raise(f'Error parsing {filename}:{lineno}, got start of array when already parsing files list')
>>> +            if inlist:
>>> +                print(f'Error parsing {filename}:{lineno}, got start of array when already parsing array list')
>>> +            inlist = True
>>> +            indent = 0
>>> +            while line[indent] == ' ':
>>> +                indent += 1
>>> +            indent += 8  # double indent required
>>> +        elif infiles and (line.endswith(')') or line.strip().startswith(')')):
>>
>> It's kinda hard to read with all the endswith/startswith, maybe extract
>> those into a function? e.g. 'elif infiles and is_file_start(line)'
>>
> It is all a bit of a mess, yes, and needs cleaning up - which is why I
> didn't previously send it with the patchset. Splitting into functions is
> something I'll look at.
> 
>>> +            infiles = False
>>> +            continue
>>> +        elif inlist and line.endswith(']') or line.strip().startswith(']'):
>>> +            inlist = False
>>> +            continue
>>> +        elif inlist or infiles:
>>> +            # skip further subarrays or lists
>>> +            if '[' in line  or ']' in line:
>>> +                continue
>>
>> I guess you could make it recursive instead of giving up? Does this happen
>> with any kind of regularity?
>>
> No, not that much, which is why I haven't explicitly tried to deal with it.
> It would be good to support in future.
> 
>>> +            if not line.startswith(' ' * indent) or line[indent] == ' ':
>>> +                print(f'Error: Incorrect indent at {filename}:{lineno + 1}')
>>> +                contents[lineno] = (' ' * indent) + line.strip() + '\n'
>>> +                line = contents[lineno].rstrip()
>>> +                edit_count += 1
>>> +            if not line.endswith(',') and '#' not in line:
>>> +                # TODO: support stripping comment and adding ','
>>> +                print(f'Error: Missing trailing "," in list at {filename}:{lineno + 1}')
>>> +                contents[lineno] = line + ',\n'
>>> +                line = contents[lineno].rstrip()
>>
>> What is the point of setting `line` here?
>>
> Because it allows us to make further checks using "line" as we add them
> later.
> The one question is whether it's worth using line as a shortcut for
> contents[lineno] or not. I'd tend to keep it, as it also allows us not to
> worry about the '\n' at the end. Then again, other rework might just change
> the whole script to strip off all the '\n' post-read and add them back
> again pre-write.
> 
> Again, further cleanup work.

Well, yes, i got that, it's just that as far as i can tell, the last 
"line = ..." is at the end of the iteration, so there's no point in 
setting it. Maybe i'm missing something :)

> 
>>> +                edit_count += 1
>>> +    return edit_count
>>> +
>>> +
>>> +def process_file(filename):
>>> +    '''run checks on file "filename"'''
>>> +    if VERBOSE:
>>> +        print(f'Processing {filename}')
>>> +    with open(filename) as f:
>>> +        contents = f.readlines()
>>
>> I guess meson build files don't get too big so it's OK to read the entire
>> file in memory and then work on it, rather than go line by line...
>>
> This was a deliberate choice when starting the script. For now the script
> only checks indentation and formatting of lists, but ideally in future it
> should check other things, e.g. alphabetical order of lists, or formatting
> of other parts. Rather than checking it all in one go, the script is
> structured so that we can call multiple functions with "contents" each of
> which does the processing without constantly re-reading and rewriting the
> file. Something like sorting a list alphabetically also requires changing
> multiple lines at a time, which is easier to do with lists that when
> streaming input.

Right, gotcha.

> 
>>> +
>>> +    if check_indentation(filename, contents) > 0 and FIX:
>>> +        print(f"Fixing {filename}")
>>> +        with open(filename, 'w') as f:
>>> +            f.writelines(contents)
>>> +
>>> +
>>> +def main():
>>> +    '''parse arguments and then call other functions to do work'''
>>> +    global VERBOSE
>>> +    global FIX
>>
>> Seems like globals are unnecessary here when you can just pass them into
>> process_file?
>>
> Yes, they can just be passed, but I actually prefer to have those as
> globals rather than having larger parameter lists. It's also possible that
> e.g. verbose, should need to be passed through multiple levels of functions.
> Personal preference, though really.

Static analyzers (e.g. pylint) will complain about it, which is why i 
usually avoid those unless really necessary :) For something like 
"verbose", sure, one could argue that it's OK to have it as a global, 
but FIX is definitely something that could be a parameter, as you don't 
seem to use it anywhere other than in process_file(), nor would it seem 
likely that it will be used in the future.

> 
>>> +    parser = ArgumentParser(description='Run syntax checks on DPDK meson.build files')
>>> +    parser.add_argument('-d', metavar='directory', default='.', help='Directory to process')
>>> +    parser.add_argument('--fix', action='store_true', help='Attempt to fix errors')
>>> +    parser.add_argument('-v', action='store_true', help='Verbose output')
>>> +    args = parser.parse_args()
>>> +
>>> +    VERBOSE = args.v
>>> +    FIX = args.fix
>>> +    for f in scan_dir(args.d):
>>> +        process_file(f)
>>> +
>>> +if __name__ == "__main__":
>>> +    main()
>>> --
>>> 2.27.0
>>>
>>
>>
>> -- 
>> Thanks,
> 
> Thanks for the review Anatoly.
> 
> /Bruce
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [RFC PATCH] devtools: script to check meson indentation of lists

[Bruce Richardson]

On Thu, Apr 22, 2021 at 11:21:26AM +0100, Burakov, Anatoly wrote:
> On 22-Apr-21 10:58 AM, Bruce Richardson wrote:
> > On Thu, Apr 22, 2021 at 10:40:37AM +0100, Burakov, Anatoly wrote:
> > > On 22-Apr-21 10:02 AM, Bruce Richardson wrote:
> > > > This is a draft script developed when I was working on the whitespace rework
> > > > changes, since extended a little to attempt to fix some trailing comma issues.
> > > > 
> > > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > > ---

<snip>

> > > > +            if not line.endswith(',') and '#' not in line:
> > > > +                # TODO: support stripping comment and adding ','
> > > > +                print(f'Error: Missing trailing "," in list at {filename}:{lineno + 1}')
> > > > +                contents[lineno] = line + ',\n'
> > > > +                line = contents[lineno].rstrip()
> > > 
> > > What is the point of setting `line` here?
> > > 
> > Because it allows us to make further checks using "line" as we add them
> > later.
> > The one question is whether it's worth using line as a shortcut for
> > contents[lineno] or not. I'd tend to keep it, as it also allows us not to
> > worry about the '\n' at the end. Then again, other rework might just change
> > the whole script to strip off all the '\n' post-read and add them back
> > again pre-write.
> > 
> > Again, further cleanup work.
> 
> Well, yes, i got that, it's just that as far as i can tell, the last "line =
> ..." is at the end of the iteration, so there's no point in setting it.
> Maybe i'm missing something :)

No, you are not, and indeed it is at the very end. The fact it's there is
to make sure it's not forgotten when adding in new checks. It also allows
the checks to be reordered easier. Think of it like the trailing comma on a
list! :-)
 
> > 
> > > > +                edit_count += 1
> > > > +    return edit_count
> > > > +
> > > > +
> > > > +def process_file(filename):
> > > > +    '''run checks on file "filename"'''
> > > > +    if VERBOSE:
> > > > +        print(f'Processing {filename}')
> > > > +    with open(filename) as f:
> > > > +        contents = f.readlines()
> > > 
> > > I guess meson build files don't get too big so it's OK to read the entire
> > > file in memory and then work on it, rather than go line by line...
> > > 
> > This was a deliberate choice when starting the script. For now the script
> > only checks indentation and formatting of lists, but ideally in future it
> > should check other things, e.g. alphabetical order of lists, or formatting
> > of other parts. Rather than checking it all in one go, the script is
> > structured so that we can call multiple functions with "contents" each of
> > which does the processing without constantly re-reading and rewriting the
> > file. Something like sorting a list alphabetically also requires changing
> > multiple lines at a time, which is easier to do with lists that when
> > streaming input.
> 
> Right, gotcha.
> 
> > 
> > > > +
> > > > +    if check_indentation(filename, contents) > 0 and FIX:
> > > > +        print(f"Fixing {filename}")
> > > > +        with open(filename, 'w') as f:
> > > > +            f.writelines(contents)
> > > > +
> > > > +
> > > > +def main():
> > > > +    '''parse arguments and then call other functions to do work'''
> > > > +    global VERBOSE
> > > > +    global FIX
> > > 
> > > Seems like globals are unnecessary here when you can just pass them into
> > > process_file?
> > > 
> > Yes, they can just be passed, but I actually prefer to have those as
> > globals rather than having larger parameter lists. It's also possible that
> > e.g. verbose, should need to be passed through multiple levels of functions.
> > Personal preference, though really.
> 
> Static analyzers (e.g. pylint) will complain about it, which is why i
> usually avoid those unless really necessary :) For something like "verbose",
> sure, one could argue that it's OK to have it as a global, but FIX is
> definitely something that could be a parameter, as you don't seem to use it
> anywhere other than in process_file(), nor would it seem likely that it will
> be used in the future.
> 

Yep, agreed. Fix can be just a parameter. It may be that it's kept as a
global here is for consistency with verbose, but more likely it's just an
oversight on my part. I will change it to parameter instead in any next
revision.

/Bruce

[dpdk-dev] [PATCH v2 1/2] devtools: script to check meson indentation of lists

[Bruce Richardson]

This is a script to fix up minor formatting issues in meson files.
It scans for, and can optionally fix, indentation issues and missing
trailing commas in the lists in meson.build files. It also detects,
and can fix, multi-line lists where more than one entry appears on a
line.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 devtools/dpdk_meson_check.py | 125 +++++++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)
 create mode 100755 devtools/dpdk_meson_check.py

diff --git a/devtools/dpdk_meson_check.py b/devtools/dpdk_meson_check.py
new file mode 100755
index 000000000..29f788796
--- /dev/null
+++ b/devtools/dpdk_meson_check.py
@@ -0,0 +1,125 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2021 Intel Corporation
+
+'''
+A Python script to run some checks on meson.build files in DPDK
+'''
+
+import sys
+import os
+from os.path import relpath, join
+from argparse import ArgumentParser
+
+VERBOSE = False
+
+
+def scan_dir(path):
+    '''return meson.build files found in path'''
+    for root, dirs, files in os.walk(path):
+        if 'meson.build' in files:
+            yield(relpath(join(root, 'meson.build')))
+
+
+def split_code_comments(line):
+    'splits a line into a code part and a comment part, returns (code, comment) tuple'
+    if line.lstrip().startswith('#'):
+        return ('', line)
+    elif '#' in line and '#include' not in line:  # catch 99% of cases, not 100%
+        idx = line.index('#')
+        while (line[idx - 1].isspace()):
+            idx -= 1
+        return line[:idx], line[idx:]
+    else:
+        return (line, '')
+
+
+def setline(contents, index, value):
+    'sets the contents[index] to value. Returns the line, along with code and comments part'
+    line = contents[index] = value
+    code, comments = split_code_comments(line)
+    return line, code, comments
+
+
+def check_indentation(filename, contents):
+    '''check that a list or files() is correctly indented'''
+    infiles = False
+    inlist = False
+    edit_count = 0
+    for lineno, line in enumerate(contents):
+        code, comments = split_code_comments(line)
+        if not code.strip():
+            continue
+        if code.endswith('files('):
+            if infiles:
+                raise(f'Error parsing {filename}:{lineno}, got "files(" when already parsing files list')
+            if inlist:
+                print(f'Error parsing {filename}:{lineno}, got "files(" when already parsing array list')
+            infiles = True
+            indent_count = len(code) - len(code.lstrip(' '))
+            indent = ' ' * (indent_count + 8)  # double indent required
+        elif code.endswith('= ['):
+            if infiles:
+                raise(f'Error parsing {filename}:{lineno}, got start of array when already parsing files list')
+            if inlist:
+                print(f'Error parsing {filename}:{lineno}, got start of array when already parsing array list')
+            inlist = True
+            indent_count = len(code) - len(code.lstrip(' '))
+            indent = ' ' * (indent_count + 8)  # double indent required
+        elif infiles and (code.endswith(')') or code.strip().startswith(')')):
+            infiles = False
+            continue
+        elif inlist and (code.endswith(']') or code.strip().startswith(']')):
+            inlist = False
+            continue
+        elif inlist or infiles:
+            # skip further subarrays or lists
+            if '[' in code or ']' in code:
+                continue
+            if not code.startswith(indent) or code[len(indent)] == ' ':
+                print(f'Error: Incorrect indent at {filename}:{lineno + 1}')
+                line, code, comments = setline(contents, lineno, indent + line.strip())
+                edit_count += 1
+            if not code.endswith(','):
+                print(f'Error: Missing trailing "," in list at {filename}:{lineno + 1}')
+                line, code, comments = setline(contents, lineno, code + ',' + comments)
+                edit_count += 1
+            if len(code.split(',')) > 2:  # only one comma per line
+                print(f'Error: multiple entries per line in list at {filename}:{lineno +1}')
+                entries = [e.strip() for e in code.split(',') if e.strip()]
+                line, code, comments = setline(contents, lineno,
+                                               indent + (',\n' + indent).join(entries) +
+                                               ',' + comments)
+                edit_count += 1
+    return edit_count
+
+
+def process_file(filename, fix):
+    '''run checks on file "filename"'''
+    if VERBOSE:
+        print(f'Processing {filename}')
+    with open(filename) as f:
+        contents = [ln.rstrip() for ln in f.readlines()]
+
+    if check_indentation(filename, contents) > 0 and fix:
+        print(f"Fixing {filename}")
+        with open(filename, 'w') as f:
+            f.writelines([f'{ln}\n' for ln in contents])
+
+
+def main():
+    '''parse arguments and then call other functions to do work'''
+    global VERBOSE
+    parser = ArgumentParser(description='Run syntax checks on DPDK meson.build files')
+    parser.add_argument('-d', metavar='directory', default='.', help='Directory to process')
+    parser.add_argument('--fix', action='store_true', help='Attempt to fix errors')
+    parser.add_argument('-v', action='store_true', help='Verbose output')
+    args = parser.parse_args()
+
+    VERBOSE = args.v
+    for f in scan_dir(args.d):
+        process_file(f, args.fix)
+
+
+if __name__ == "__main__":
+    main()
-- 
2.30.2

[dpdk-dev] [PATCH v2 2/2] build: style fixup on meson files

[Bruce Richardson]

Running "./devtools/dpdk_meson_check.py --fix" on the DPDK repo fixes a
number of issues with whitespace and formatting of files:

* indentation of lists
* missing trailing commas on final list element
* multiple list entries per line when list is not all single-line

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test-eventdev/meson.build                 |  2 +-
 config/x86/meson.build                        | 24 +++++++++----------
 drivers/bus/fslmc/meson.build                 |  2 +-
 drivers/common/mlx5/meson.build               |  2 +-
 drivers/common/sfc_efx/base/meson.build       |  2 +-
 drivers/common/sfc_efx/meson.build            |  4 ++--
 drivers/compress/mlx5/meson.build             |  2 +-
 drivers/crypto/bcmfs/meson.build              |  2 +-
 drivers/crypto/nitrox/meson.build             |  2 +-
 drivers/crypto/qat/meson.build                |  2 +-
 drivers/net/e1000/base/meson.build            |  2 +-
 drivers/net/fm10k/base/meson.build            |  2 +-
 drivers/net/i40e/base/meson.build             |  2 +-
 drivers/net/ixgbe/base/meson.build            |  2 +-
 drivers/net/octeontx/base/meson.build         |  2 +-
 drivers/net/sfc/meson.build                   |  4 ++--
 drivers/net/thunderx/base/meson.build         |  2 +-
 drivers/raw/skeleton/meson.build              |  2 +-
 examples/l3fwd/meson.build                    | 14 +++++------
 .../client_server_mp/mp_client/meson.build    |  2 +-
 .../client_server_mp/mp_server/meson.build    |  4 +++-
 examples/multi_process/hotplug_mp/meson.build |  3 ++-
 examples/multi_process/simple_mp/meson.build  |  3 ++-
 .../multi_process/symmetric_mp/meson.build    |  2 +-
 lib/mbuf/meson.build                          |  2 +-
 lib/meson.build                               |  6 ++---
 lib/power/meson.build                         |  2 +-
 lib/table/meson.build                         |  2 +-
 28 files changed, 53 insertions(+), 49 deletions(-)

diff --git a/app/test-eventdev/meson.build b/app/test-eventdev/meson.build
index 04117dbe4..45c3ff456 100644
--- a/app/test-eventdev/meson.build
+++ b/app/test-eventdev/meson.build
@@ -14,6 +14,6 @@ sources = files(
         'test_perf_queue.c',
         'test_pipeline_atq.c',
         'test_pipeline_common.c',
-        'test_pipeline_queue.c'
+        'test_pipeline_queue.c',
 )
 deps += 'eventdev'
diff --git a/config/x86/meson.build b/config/x86/meson.build
index 34b116481..b9348c44d 100644
--- a/config/x86/meson.build
+++ b/config/x86/meson.build
@@ -22,18 +22,18 @@ foreach f:base_flags
 endforeach
 
 optional_flags = [
-    'AES',
-    'AVX',
-    'AVX2',
-    'AVX512BW',
-    'AVX512CD',
-    'AVX512DQ',
-    'AVX512F',
-    'AVX512VL',
-    'PCLMUL',
-    'RDRND',
-    'RDSEED',
-    'VPCLMULQDQ',
+        'AES',
+        'AVX',
+        'AVX2',
+        'AVX512BW',
+        'AVX512CD',
+        'AVX512DQ',
+        'AVX512F',
+        'AVX512VL',
+        'PCLMUL',
+        'RDRND',
+        'RDSEED',
+        'VPCLMULQDQ',
 ]
 foreach f:optional_flags
     if cc.get_define('__@0@__'.format(f), args: machine_args) == '1'
diff --git a/drivers/bus/fslmc/meson.build b/drivers/bus/fslmc/meson.build
index e99218ca7..54be76f51 100644
--- a/drivers/bus/fslmc/meson.build
+++ b/drivers/bus/fslmc/meson.build
@@ -21,7 +21,7 @@ sources = files(
         'portal/dpaa2_hw_dpci.c',
         'portal/dpaa2_hw_dpio.c',
         'qbman/qbman_portal.c',
-        'qbman/qbman_debug.c'
+        'qbman/qbman_debug.c',
 )
 
 includes += include_directories('mc', 'qbman/include', 'portal')
diff --git a/drivers/common/mlx5/meson.build b/drivers/common/mlx5/meson.build
index b5585d2cc..e78b4f47b 100644
--- a/drivers/common/mlx5/meson.build
+++ b/drivers/common/mlx5/meson.build
@@ -23,7 +23,7 @@ cflags_options = [
         '-Wno-strict-prototypes',
         '-D_BSD_SOURCE',
         '-D_DEFAULT_SOURCE',
-        '-D_XOPEN_SOURCE=600'
+        '-D_XOPEN_SOURCE=600',
 ]
 foreach option:cflags_options
     if cc.has_argument(option)
diff --git a/drivers/common/sfc_efx/base/meson.build b/drivers/common/sfc_efx/base/meson.build
index 313e53d77..9fba47b1c 100644
--- a/drivers/common/sfc_efx/base/meson.build
+++ b/drivers/common/sfc_efx/base/meson.build
@@ -70,7 +70,7 @@ extra_flags = [
         '-Wno-unused-parameter',
         '-Wno-unused-variable',
         '-Wno-empty-body',
-        '-Wno-unused-but-set-variable'
+        '-Wno-unused-but-set-variable',
 ]
 
 c_args = cflags
diff --git a/drivers/common/sfc_efx/meson.build b/drivers/common/sfc_efx/meson.build
index d87ba396b..f42ccf609 100644
--- a/drivers/common/sfc_efx/meson.build
+++ b/drivers/common/sfc_efx/meson.build
@@ -19,13 +19,13 @@ extra_flags = []
 
 # Enable more warnings
 extra_flags += [
-        '-Wdisabled-optimization'
+        '-Wdisabled-optimization',
 ]
 
 # Compiler and version dependent flags
 extra_flags += [
         '-Waggregate-return',
-        '-Wbad-function-cast'
+        '-Wbad-function-cast',
 ]
 
 foreach flag: extra_flags
diff --git a/drivers/compress/mlx5/meson.build b/drivers/compress/mlx5/meson.build
index c5c5edfa7..7aac32998 100644
--- a/drivers/compress/mlx5/meson.build
+++ b/drivers/compress/mlx5/meson.build
@@ -17,7 +17,7 @@ cflags_options = [
         '-Wno-strict-prototypes',
         '-D_BSD_SOURCE',
         '-D_DEFAULT_SOURCE',
-        '-D_XOPEN_SOURCE=600'
+        '-D_XOPEN_SOURCE=600',
 ]
 foreach option:cflags_options
     if cc.has_argument(option)
diff --git a/drivers/crypto/bcmfs/meson.build b/drivers/crypto/bcmfs/meson.build
index 0388ba19a..d67e78d51 100644
--- a/drivers/crypto/bcmfs/meson.build
+++ b/drivers/crypto/bcmfs/meson.build
@@ -16,5 +16,5 @@ sources = files(
         'bcmfs_sym_capabilities.c',
         'bcmfs_sym_session.c',
         'bcmfs_sym.c',
-        'bcmfs_sym_engine.c'
+        'bcmfs_sym_engine.c',
 )
diff --git a/drivers/crypto/nitrox/meson.build b/drivers/crypto/nitrox/meson.build
index 2fdae03c1..2cc47c462 100644
--- a/drivers/crypto/nitrox/meson.build
+++ b/drivers/crypto/nitrox/meson.build
@@ -14,5 +14,5 @@ sources = files(
         'nitrox_sym.c',
         'nitrox_sym_capabilities.c',
         'nitrox_sym_reqmgr.c',
-        'nitrox_qp.c'
+        'nitrox_qp.c',
 )
diff --git a/drivers/crypto/qat/meson.build b/drivers/crypto/qat/meson.build
index 4da819d65..b3b2d1725 100644
--- a/drivers/crypto/qat/meson.build
+++ b/drivers/crypto/qat/meson.build
@@ -17,7 +17,7 @@ if dep.found()
             'qat_asym_pmd.c',
             'qat_sym.c',
             'qat_sym_hw_dp.c',
-			'qat_sym_pmd.c',
+            'qat_sym_pmd.c',
             'qat_sym_session.c',
 	)
     qat_ext_deps += dep
diff --git a/drivers/net/e1000/base/meson.build b/drivers/net/e1000/base/meson.build
index 99468764e..317692dfa 100644
--- a/drivers/net/e1000/base/meson.build
+++ b/drivers/net/e1000/base/meson.build
@@ -19,7 +19,7 @@ sources = [
         'e1000_nvm.c',
         'e1000_osdep.c',
         'e1000_phy.c',
-        'e1000_vf.c'
+        'e1000_vf.c',
 ]
 
 error_cflags = ['-Wno-uninitialized', '-Wno-unused-parameter',
diff --git a/drivers/net/fm10k/base/meson.build b/drivers/net/fm10k/base/meson.build
index 6467a1278..ca98d34d4 100644
--- a/drivers/net/fm10k/base/meson.build
+++ b/drivers/net/fm10k/base/meson.build
@@ -7,7 +7,7 @@ sources = [
         'fm10k_mbx.c',
         'fm10k_pf.c',
         'fm10k_tlv.c',
-        'fm10k_vf.c'
+        'fm10k_vf.c',
 ]
 
 error_cflags = ['-Wno-unused-parameter', '-Wno-unused-value',
diff --git a/drivers/net/i40e/base/meson.build b/drivers/net/i40e/base/meson.build
index c319902cd..79a887a29 100644
--- a/drivers/net/i40e/base/meson.build
+++ b/drivers/net/i40e/base/meson.build
@@ -8,7 +8,7 @@ sources = [
         'i40e_diag.c',
         'i40e_hmc.c',
         'i40e_lan_hmc.c',
-        'i40e_nvm.c'
+        'i40e_nvm.c',
 ]
 
 error_cflags = ['-Wno-sign-compare', '-Wno-unused-value',
diff --git a/drivers/net/ixgbe/base/meson.build b/drivers/net/ixgbe/base/meson.build
index 4d5e41bde..7d3cec002 100644
--- a/drivers/net/ixgbe/base/meson.build
+++ b/drivers/net/ixgbe/base/meson.build
@@ -14,7 +14,7 @@ sources = [
         'ixgbe_phy.c',
         'ixgbe_vf.c',
         'ixgbe_x540.c',
-        'ixgbe_x550.c'
+        'ixgbe_x550.c',
 ]
 
 error_cflags = ['-Wno-unused-value',
diff --git a/drivers/net/octeontx/base/meson.build b/drivers/net/octeontx/base/meson.build
index 0a69dfd46..c86a72670 100644
--- a/drivers/net/octeontx/base/meson.build
+++ b/drivers/net/octeontx/base/meson.build
@@ -4,7 +4,7 @@
 sources = [
         'octeontx_pkovf.c',
         'octeontx_pkivf.c',
-        'octeontx_bgx.c'
+        'octeontx_bgx.c',
 ]
 
 depends = ['ethdev', 'mempool_octeontx']
diff --git a/drivers/net/sfc/meson.build b/drivers/net/sfc/meson.build
index b58425bf9..ccf5984d8 100644
--- a/drivers/net/sfc/meson.build
+++ b/drivers/net/sfc/meson.build
@@ -24,13 +24,13 @@ extra_flags += '-Wno-strict-aliasing'
 
 # Enable more warnings
 extra_flags += [
-        '-Wdisabled-optimization'
+        '-Wdisabled-optimization',
 ]
 
 # Compiler and version dependent flags
 extra_flags += [
         '-Waggregate-return',
-        '-Wbad-function-cast'
+        '-Wbad-function-cast',
 ]
 
 foreach flag: extra_flags
diff --git a/drivers/net/thunderx/base/meson.build b/drivers/net/thunderx/base/meson.build
index 9cf4bf553..704ee6577 100644
--- a/drivers/net/thunderx/base/meson.build
+++ b/drivers/net/thunderx/base/meson.build
@@ -4,7 +4,7 @@
 sources = [
         'nicvf_hw.c',
         'nicvf_mbox.c',
-        'nicvf_bsvf.c'
+        'nicvf_bsvf.c',
 ]
 
 c_args = cflags
diff --git a/drivers/raw/skeleton/meson.build b/drivers/raw/skeleton/meson.build
index b5d87652f..950a33cc2 100644
--- a/drivers/raw/skeleton/meson.build
+++ b/drivers/raw/skeleton/meson.build
@@ -3,6 +3,6 @@
 
 deps += ['rawdev', 'kvargs', 'mbuf', 'bus_vdev']
 sources = files(
-		'skeleton_rawdev.c',
+        'skeleton_rawdev.c',
         'skeleton_rawdev_test.c',
 )
diff --git a/examples/l3fwd/meson.build b/examples/l3fwd/meson.build
index 773ebf478..0830b3eb3 100644
--- a/examples/l3fwd/meson.build
+++ b/examples/l3fwd/meson.build
@@ -9,11 +9,11 @@
 allow_experimental_apis = true
 deps += ['hash', 'lpm', 'fib', 'eventdev']
 sources = files(
-    'l3fwd_em.c',
-    'l3fwd_event.c',
-    'l3fwd_event_internal_port.c',
-    'l3fwd_event_generic.c',
-    'l3fwd_fib.c',
-    'l3fwd_lpm.c',
-    'main.c',
+        'l3fwd_em.c',
+        'l3fwd_event.c',
+        'l3fwd_event_internal_port.c',
+        'l3fwd_event_generic.c',
+        'l3fwd_fib.c',
+        'l3fwd_lpm.c',
+        'main.c',
 )
diff --git a/examples/multi_process/client_server_mp/mp_client/meson.build b/examples/multi_process/client_server_mp/mp_client/meson.build
index 3c4848bde..c7190b212 100644
--- a/examples/multi_process/client_server_mp/mp_client/meson.build
+++ b/examples/multi_process/client_server_mp/mp_client/meson.build
@@ -10,5 +10,5 @@ includes += include_directories('../shared')
 
 allow_experimental_apis = true
 sources = files(
-        'client.c'
+        'client.c',
 )
diff --git a/examples/multi_process/client_server_mp/mp_server/meson.build b/examples/multi_process/client_server_mp/mp_server/meson.build
index 74814e5fe..9e585e80b 100644
--- a/examples/multi_process/client_server_mp/mp_server/meson.build
+++ b/examples/multi_process/client_server_mp/mp_server/meson.build
@@ -10,5 +10,7 @@ includes += include_directories('../shared')
 
 allow_experimental_apis = true
 sources = files(
-        'args.c', 'init.c', 'main.c'
+        'args.c',
+        'init.c',
+        'main.c',
 )
diff --git a/examples/multi_process/hotplug_mp/meson.build b/examples/multi_process/hotplug_mp/meson.build
index ae9290504..a1ad98ca2 100644
--- a/examples/multi_process/hotplug_mp/meson.build
+++ b/examples/multi_process/hotplug_mp/meson.build
@@ -8,5 +8,6 @@
 
 allow_experimental_apis = true
 sources = files(
-        'commands.c', 'main.c'
+        'commands.c',
+        'main.c',
 )
diff --git a/examples/multi_process/simple_mp/meson.build b/examples/multi_process/simple_mp/meson.build
index adb06b0a7..359af4384 100644
--- a/examples/multi_process/simple_mp/meson.build
+++ b/examples/multi_process/simple_mp/meson.build
@@ -8,5 +8,6 @@
 
 allow_experimental_apis = true
 sources = files(
-        'mp_commands.c', 'main.c'
+        'mp_commands.c',
+        'main.c',
 )
diff --git a/examples/multi_process/symmetric_mp/meson.build b/examples/multi_process/symmetric_mp/meson.build
index a5b988c44..e768a324d 100644
--- a/examples/multi_process/symmetric_mp/meson.build
+++ b/examples/multi_process/symmetric_mp/meson.build
@@ -8,5 +8,5 @@
 
 allow_experimental_apis = true
 sources = files(
-        'main.c'
+        'main.c',
 )
diff --git a/lib/mbuf/meson.build b/lib/mbuf/meson.build
index c8c08220a..0435c5e62 100644
--- a/lib/mbuf/meson.build
+++ b/lib/mbuf/meson.build
@@ -12,6 +12,6 @@ headers = files(
         'rte_mbuf_core.h',
         'rte_mbuf_ptype.h',
         'rte_mbuf_pool_ops.h',
-        'rte_mbuf_dyn.h'
+        'rte_mbuf_dyn.h',
 )
 deps += ['mempool']
diff --git a/lib/meson.build b/lib/meson.build
index c9a20f65b..64a59abab 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -82,9 +82,9 @@ if is_windows
 endif
 
 optional_libs = [
-    'kni',
-    'power',
-    'vhost',
+        'kni',
+        'power',
+        'vhost',
 ]
 
 disabled_libs = []
diff --git a/lib/power/meson.build b/lib/power/meson.build
index a2cc9fe2e..c1097d32f 100644
--- a/lib/power/meson.build
+++ b/lib/power/meson.build
@@ -19,6 +19,6 @@ headers = files(
         'rte_power.h',
         'rte_power_empty_poll.h',
         'rte_power_pmd_mgmt.h',
-        'rte_power_guest_channel.h'
+        'rte_power_guest_channel.h',
 )
 deps += ['timer', 'ethdev']
diff --git a/lib/table/meson.build b/lib/table/meson.build
index 230f21ea8..b7b70b805 100644
--- a/lib/table/meson.build
+++ b/lib/table/meson.build
@@ -36,5 +36,5 @@ deps += ['mbuf', 'port', 'lpm', 'hash', 'acl']
 indirect_headers += files(
         'rte_lru_arm64.h',
         'rte_lru_x86.h',
-        'rte_table_hash_func_arm64.h'
+        'rte_table_hash_func_arm64.h',
 )
-- 
2.30.2

Re: [dpdk-dev] [PATCH v2 1/2] devtools: script to check meson indentation of lists

[Burakov, Anatoly]

On 26-Apr-21 11:54 AM, Bruce Richardson wrote:
> This is a script to fix up minor formatting issues in meson files.
> It scans for, and can optionally fix, indentation issues and missing
> trailing commas in the lists in meson.build files. It also detects,
> and can fix, multi-line lists where more than one entry appears on a
> line.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---

<snip>

> +def split_code_comments(line):
> +    'splits a line into a code part and a comment part, returns (code, comment) tuple'
> +    if line.lstrip().startswith('#'):
> +        return ('', line)
> +    elif '#' in line and '#include' not in line:  # catch 99% of cases, not 100% > +        idx = line.index('#')
> +        while (line[idx - 1].isspace()):
> +            idx -= 1
> +        return line[:idx], line[idx:]


I think this could be simplified with regex:

# find any occurrences of '#' but only if it's not an '#include'
if not re.search(r'#(?!include)', line)
     return line, ''
return line.split('#', maxsplit=1)

> +    else:
> +        return (line, '')
> +
> +
> +def setline(contents, index, value):
> +    'sets the contents[index] to value. Returns the line, along with code and comments part'
> +    line = contents[index] = value
> +    code, comments = split_code_comments(line)
> +    return line, code, comments
> +
> +
> +def check_indentation(filename, contents):
> +    '''check that a list or files() is correctly indented'''
> +    infiles = False
> +    inlist = False
> +    edit_count = 0
> +    for lineno, line in enumerate(contents):
> +        code, comments = split_code_comments(line)

Nitpicking, but maybe instead of calling strip() all over the place, 
just count the number of spaces and strip right at the outset? E.g. 
something like:

stripped = code.strip()
line_indent = len(code) - len(stripped)

You can then reason about indent levels by comparing stripped to code 
afterwards, and avoid doing this:

> +            # skip further subarrays or lists
> +            if '[' in code or ']' in code:
> +                continue
> +            if not code.startswith(indent) or code[len(indent)] == ' ':

Opting to just check the indent size you calculated initially. Unless 
i'm missing something :)

You could also increment edit_count if `calculated indent + stripped` is 
equal to `code`. Seems easier logic than raw string manipulation you're 
going for here...

<snip>

> +def process_file(filename, fix):
> +    '''run checks on file "filename"'''
> +    if VERBOSE:
> +        print(f'Processing {filename}')
> +    with open(filename) as f:
> +        contents = [ln.rstrip() for ln in f.readlines()]

So any trailing whitespace gets automatically and silently fixed?

> +
> +    if check_indentation(filename, contents) > 0 and fix:
> +        print(f"Fixing {filename}")
> +        with open(filename, 'w') as f:
> +            f.writelines([f'{ln}\n' for ln in contents])

Something seems suspect here. So, if `fix` is *not* specified, the 
script just opens the file, reads it, and... does nothing else?

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2 1/2] devtools: script to check meson indentation of lists

[Bruce Richardson]

On Mon, Apr 26, 2021 at 02:40:25PM +0100, Burakov, Anatoly wrote:
> On 26-Apr-21 11:54 AM, Bruce Richardson wrote:
> > This is a script to fix up minor formatting issues in meson files.
> > It scans for, and can optionally fix, indentation issues and missing
> > trailing commas in the lists in meson.build files. It also detects,
> > and can fix, multi-line lists where more than one entry appears on a
> > line.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> 
> <snip>
> 
> > +def split_code_comments(line):
> > +    'splits a line into a code part and a comment part, returns (code, comment) tuple'
> > +    if line.lstrip().startswith('#'):
> > +        return ('', line)
> > +    elif '#' in line and '#include' not in line:  # catch 99% of cases, not 100% > +        idx = line.index('#')
> > +        while (line[idx - 1].isspace()):
> > +            idx -= 1
> > +        return line[:idx], line[idx:]
> 
> 
> I think this could be simplified with regex:
> 
> # find any occurrences of '#' but only if it's not an '#include'
> if not re.search(r'#(?!include)', line)
>     return line, ''
> return line.split('#', maxsplit=1)

Not sure that is simpler, and just splitting on '#' is actually not what we
want either.

Firstly, while r'#(?!include)' is not a massively complex regex, just
checking for "'#' in line and '#include' not in line" is just easier to
read for most mortals.

In terms of the split, I did initially do as you have here and split on
'#', but we don't actually want that, because we want to preserve the
whitespace in the line before the comment too - as part of the comment, not
the code. This is why after finding the '#' we walk backwards to find the
end of the code and find that as the split point. It then saves us worrying
about any strips() breaking any comment alignment the user has explicitly
set up. Not using split also means that we can just merge the strings back
with '+' rather than having to use "'#'.join()".

> 
> > +    else:
> > +        return (line, '')
> > +
> > +
> > +def setline(contents, index, value):
> > +    'sets the contents[index] to value. Returns the line, along with code and comments part'
> > +    line = contents[index] = value
> > +    code, comments = split_code_comments(line)
> > +    return line, code, comments
> > +
> > +
> > +def check_indentation(filename, contents):
> > +    '''check that a list or files() is correctly indented'''
> > +    infiles = False
> > +    inlist = False
> > +    edit_count = 0
> > +    for lineno, line in enumerate(contents):
> > +        code, comments = split_code_comments(line)
> 
> Nitpicking, but maybe instead of calling strip() all over the place, just
> count the number of spaces and strip right at the outset? E.g. something
> like:
> 
> stripped = code.strip()
> line_indent = len(code) - len(stripped)
> 
> You can then reason about indent levels by comparing stripped to code
> afterwards, and avoid doing this:
> 
> > +            # skip further subarrays or lists
> > +            if '[' in code or ']' in code:
> > +                continue
> > +            if not code.startswith(indent) or code[len(indent)] == ' ':
> 
> Opting to just check the indent size you calculated initially. Unless i'm
> missing something :)
> 
> You could also increment edit_count if `calculated indent + stripped` is
> equal to `code`. Seems easier logic than raw string manipulation you're
> going for here...
> 
> <snip>

Interesting. That could be a good approach alright. If I do a V3 (not
guaranteed for this release) I can try taking that idea on board.

> 
> > +def process_file(filename, fix):
> > +    '''run checks on file "filename"'''
> > +    if VERBOSE:
> > +        print(f'Processing {filename}')
> > +    with open(filename) as f:
> > +        contents = [ln.rstrip() for ln in f.readlines()]
> 
> So any trailing whitespace gets automatically and silently fixed?
> 
Hadn't actually thought of that, but yes, that will happen if --fix is
given and other changes are made to the file. Ideally, that should be fixed
to "non-silently" do so, but I'd view it as low priority since other tools
tend to be good at flagging trailing whitespace issues anyway.

> > +
> > +    if check_indentation(filename, contents) > 0 and fix:
> > +        print(f"Fixing {filename}")
> > +        with open(filename, 'w') as f:
> > +            f.writelines([f'{ln}\n' for ln in contents])
> 
> Something seems suspect here. So, if `fix` is *not* specified, the script
> just opens the file, reads it, and... does nothing else?
> 
No, it prints out all the errors without actually fixing them.

Regards,
/Bruce

Re: [dpdk-dev] [PATCH v2 1/2] devtools: script to check meson indentation of lists

[Burakov, Anatoly]

On 26-Apr-21 3:05 PM, Bruce Richardson wrote:
> On Mon, Apr 26, 2021 at 02:40:25PM +0100, Burakov, Anatoly wrote:
>> On 26-Apr-21 11:54 AM, Bruce Richardson wrote:
>>> This is a script to fix up minor formatting issues in meson files.
>>> It scans for, and can optionally fix, indentation issues and missing
>>> trailing commas in the lists in meson.build files. It also detects,
>>> and can fix, multi-line lists where more than one entry appears on a
>>> line.
>>>
>>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>>> ---
>>
>> <snip>
>>
>>> +def split_code_comments(line):
>>> +    'splits a line into a code part and a comment part, returns (code, comment) tuple'
>>> +    if line.lstrip().startswith('#'):
>>> +        return ('', line)
>>> +    elif '#' in line and '#include' not in line:  # catch 99% of cases, not 100% > +        idx = line.index('#')
>>> +        while (line[idx - 1].isspace()):
>>> +            idx -= 1
>>> +        return line[:idx], line[idx:]
>>
>>
>> I think this could be simplified with regex:
>>
>> # find any occurrences of '#' but only if it's not an '#include'
>> if not re.search(r'#(?!include)', line)
>>      return line, ''
>> return line.split('#', maxsplit=1)
> 
> Not sure that is simpler, and just splitting on '#' is actually not what we
> want either.
> 
> Firstly, while r'#(?!include)' is not a massively complex regex, just
> checking for "'#' in line and '#include' not in line" is just easier to
> read for most mortals.
> 
> In terms of the split, I did initially do as you have here and split on
> '#', but we don't actually want that, because we want to preserve the
> whitespace in the line before the comment too - as part of the comment, not
> the code. This is why after finding the '#' we walk backwards to find the
> end of the code and find that as the split point. It then saves us worrying
> about any strips() breaking any comment alignment the user has explicitly
> set up. Not using split also means that we can just merge the strings back
> with '+' rather than having to use "'#'.join()".

Fair enough so!

> 
>>
>>> +    else:
>>> +        return (line, '')
>>> +
>>> +
>>> +def setline(contents, index, value):
>>> +    'sets the contents[index] to value. Returns the line, along with code and comments part'
>>> +    line = contents[index] = value
>>> +    code, comments = split_code_comments(line)
>>> +    return line, code, comments
>>> +
>>> +
>>> +def check_indentation(filename, contents):
>>> +    '''check that a list or files() is correctly indented'''
>>> +    infiles = False
>>> +    inlist = False
>>> +    edit_count = 0
>>> +    for lineno, line in enumerate(contents):
>>> +        code, comments = split_code_comments(line)
>>
>> Nitpicking, but maybe instead of calling strip() all over the place, just
>> count the number of spaces and strip right at the outset? E.g. something
>> like:
>>
>> stripped = code.strip()
>> line_indent = len(code) - len(stripped)
>>
>> You can then reason about indent levels by comparing stripped to code
>> afterwards, and avoid doing this:
>>
>>> +            # skip further subarrays or lists
>>> +            if '[' in code or ']' in code:
>>> +                continue
>>> +            if not code.startswith(indent) or code[len(indent)] == ' ':
>>
>> Opting to just check the indent size you calculated initially. Unless i'm
>> missing something :)
>>
>> You could also increment edit_count if `calculated indent + stripped` is
>> equal to `code`. Seems easier logic than raw string manipulation you're
>> going for here...
>>
>> <snip>
> 
> Interesting. That could be a good approach alright. If I do a V3 (not
> guaranteed for this release) I can try taking that idea on board.
> 
>>
>>> +def process_file(filename, fix):
>>> +    '''run checks on file "filename"'''
>>> +    if VERBOSE:
>>> +        print(f'Processing {filename}')
>>> +    with open(filename) as f:
>>> +        contents = [ln.rstrip() for ln in f.readlines()]
>>
>> So any trailing whitespace gets automatically and silently fixed?
>>
> Hadn't actually thought of that, but yes, that will happen if --fix is
> given and other changes are made to the file. Ideally, that should be fixed
> to "non-silently" do so, but I'd view it as low priority since other tools
> tend to be good at flagging trailing whitespace issues anyway.

Yeah, i think there's not a lot of trailing whitespace there in the 
first place, so probably a non issue.

> 
>>> +
>>> +    if check_indentation(filename, contents) > 0 and fix:
>>> +        print(f"Fixing {filename}")
>>> +        with open(filename, 'w') as f:
>>> +            f.writelines([f'{ln}\n' for ln in contents])
>>
>> Something seems suspect here. So, if `fix` is *not* specified, the script
>> just opens the file, reads it, and... does nothing else?
>>
> No, it prints out all the errors without actually fixing them.
> 

Oh, right, check_indentation will run first. Never mind!

In any case,

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2 1/2] devtools: script to check meson indentation of lists

[Thomas Monjalon]

26/04/2021 12:54, Bruce Richardson:
> This is a script to fix up minor formatting issues in meson files.
> It scans for, and can optionally fix, indentation issues and missing
> trailing commas in the lists in meson.build files. It also detects,
> and can fix, multi-line lists where more than one entry appears on a
> line.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  devtools/dpdk_meson_check.py | 125 +++++++++++++++++++++++++++++++++++

Renamed to check-meson.py and added in MAINTAINERS.

Series applied, thanks.

Re: [dpdk-dev] [PATCH v2 1/2] devtools: script to check meson indentation of lists

[David Marchand]

On Tue, May 4, 2021 at 3:05 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 26/04/2021 12:54, Bruce Richardson:
> > This is a script to fix up minor formatting issues in meson files.
> > It scans for, and can optionally fix, indentation issues and missing
> > trailing commas in the lists in meson.build files. It also detects,
> > and can fix, multi-line lists where more than one entry appears on a
> > line.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  devtools/dpdk_meson_check.py | 125 +++++++++++++++++++++++++++++++++++
>
> Renamed to check-meson.py and added in MAINTAINERS.
>
> Series applied, thanks.

The default behavior is not that friendly (to me).
I have build directories in my sources with stuff from older branches
where the coding fixes are not backported.
This results in the check complaining on all those directories.

Can we have a default behavior where only committed files are considered?
Maybe filtering with git ls-files **/meson.build.


-- 
David Marchand