Re: [Buildroot] [PATCH 2/3] utils/get-developers: bail out on parsing errors

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello,

On Sun, 24 Jul 2022 20:53:00 -0300
Ricardo Martincoski <ricardo.martincoski@gmail.com> wrote:

> diff --git a/utils/getdeveloperlib.py b/utils/getdeveloperlib.py
> index 2a8d5c213c..f9f84ee8e1 100644
> --- a/utils/getdeveloperlib.py
> +++ b/utils/getdeveloperlib.py
> @@ -225,34 +225,37 @@ def parse_developer_runtime_tests(fnames):
>      return runtimes
>  
>  
>  def parse_developers(filename=None):
>      """Parse the DEVELOPERS file and return a list of Developer objects."""
>      developers = []
>      linen = 0
> +    errors = 0
>      global unittests
>      unittests = list_unittests()
>      developers_fname = filename or os.path.join(brpath, 'DEVELOPERS')
>      with open(developers_fname, mode='r', encoding='utf_8') as f:
>          files = []
>          name = None
>          for line in f:
>              line = line.strip()
>              if line.startswith("#"):
>                  continue
>              elif line.startswith("N:"):
>                  if name is not None or len(files) != 0:
> +                    errors += 1
>                      print("Syntax error in DEVELOPERS file, line %d" % linen,
>                            file=sys.stderr)

So in this error condition, we continue the parsing...

>                  name = line[2:].strip()
>              elif line.startswith("F:"):
>                  fname = line[2:].strip()
>                  dev_files = glob.glob(os.path.join(brpath, fname))
>                  if len(dev_files) == 0:
> -                    print("WARNING: '%s' doesn't match any file" % fname,
> +                    errors += 1
> +                    print("Syntax error in DEVELOPERS file, line %d: '%s' doesn't match any file" % (linen, fname),
>                            file=sys.stderr)
>                  for f in dev_files:
>                      dev_file = os.path.relpath(f, brpath)
>                      dev_file = dev_file.replace(os.sep, '/')  # force unix sep
>                      if f[-1] == '/':  # relpath removes the trailing /
>                          dev_file = dev_file + '/'
>                      files.append(dev_file)
> @@ -266,14 +269,16 @@ def parse_developers(filename=None):
>                  print("Syntax error in DEVELOPERS file, line %d: '%s'" % (linen, line),
>                        file=sys.stderr)
>                  return None

but in this one we abort immediately. It is quite confusing that those
two (similar?) error conditions are handled in a different way. Of
course, it is already the case in the current code, but your errors +=
1 made me think about this. I guess on syntax errors, we should stop
parsing immediately, because the rest of the state machine is not
designed to handle malformed input.

So on the two syntax errors conditions: return None immediately, don't
bother counting errors.

On the file not matching case, perhaps a dedicated counter:
'unmatched_files' ?

Regarding the "%s doesn't match any file", I am not happy with having
the message start with "Syntax error in DEVELOPERS file", because it's
not a syntax error. The syntax is perfectly correct. So I appreciate
your wish to make message more consistent, but it doesn't work well
here I'm afraid.

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot