[10/28] qapi: Rework name checking in preparation of stricter checking

Naming rules differ for the various kinds of names.  To prepare
enforcing them, define functions to check them: check_name_upper(),
check_name_lower(), and check_name_camel().  For now, these merely
wrap around check_name_str(), but that will change shortly.  Replace
the other uses of check_name_str() by appropriate uses of the
wrappers.  No change in behavior just yet.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 15 deletions(-)

On 3/23/21 4:40 AM, Markus Armbruster wrote:
> Naming rules differ for the various kinds of names.  To prepare
> enforcing them, define functions to check them: check_name_upper(),
> check_name_lower(), and check_name_camel().  For now, these merely
> wrap around check_name_str(), but that will change shortly.  Replace
> the other uses of check_name_str() by appropriate uses of the
> wrappers.  No change in behavior just yet.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 36 insertions(+), 15 deletions(-)
> 

> +++ b/scripts/qapi/expr.py
> @@ -21,11 +21,12 @@
>  from .error import QAPISemError
>  
>  
> -# Names must be letters, numbers, -, and _.  They must start with letter,
> -# except for downstream extensions which must start with __RFQDN_.
> -# Dots are only valid in the downstream extension prefix.
> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
> -                        '[a-zA-Z][a-zA-Z0-9_-]*$')

I'm assuming python concatenates r'' with '' in the obvious manner...

> +# Names consist of letters, digits, -, and _, starting with a letter.
> +# An experimental name is prefixed with x-.  A name of a downstream
> +# extension is prefixed with __RFQDN_.  The latter prefix goes first.
> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
> +                        r'(x-)?'
> +                        r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)

...but like your explicit use of r'' r''.

Splitting out special handling of r'(x-)?' does not change behavior, but
is not otherwise mentioned in your commit message.  I suspect you did it
to make it easier to permit x-EVENT_NAME in later patches where upper is
handled differently from lower or camel, so I won't withhold R-b, but it
may be worth a tweak to the commit message.


>  
>  def check_defn_name_str(name, info, meta):
> -    check_name_str(name, info, meta, permit_upper=True)
> +    if meta == 'event':
> +        check_name_upper(name, info, meta)
> +    elif meta == 'command':
> +        check_name_lower(name, info, meta, permit_upper=True)

Why do commands need to permit upper?  I guess just downstream FQDN
extensions?

Otherwise the patch makes sense.

Reviewed-by: Eric Blake <eblake@redhat.com>

On 3/23/21 10:20 AM, Eric Blake wrote:
>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
>> -                        '[a-zA-Z][a-zA-Z0-9_-]*$')
> I'm assuming python concatenates r'' with '' in the obvious manner...
> 

FWIW, I don't think it does, actually. I believe you do need to spell 
out each individual string constant with what type it is.

(In this case, missing the second r has no effect as there are no 
backslash sequences in the string.)

--js

On 3/23/21 9:30 AM, John Snow wrote:
> On 3/23/21 10:20 AM, Eric Blake wrote:
>>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
>>> -                        '[a-zA-Z][a-zA-Z0-9_-]*$')
>> I'm assuming python concatenates r'' with '' in the obvious manner...
>>
> 
> FWIW, I don't think it does, actually. I believe you do need to spell
> out each individual string constant with what type it is.
> 
> (In this case, missing the second r has no effect as there are no
> backslash sequences in the string.)

Aha -
https://docs.python.org/3/reference/lexical_analysis.html#string-literal-concatenation
talks about it more, and even mentions that joining r'' with plain '' is
useful for scenarios where you want easier use of \ through only part of
your overall literal (since string literal concatenation is performed at
compile time).

Eric Blake <eblake@redhat.com> writes:

> On 3/23/21 4:40 AM, Markus Armbruster wrote:
>> Naming rules differ for the various kinds of names.  To prepare
>> enforcing them, define functions to check them: check_name_upper(),
>> check_name_lower(), and check_name_camel().  For now, these merely
>> wrap around check_name_str(), but that will change shortly.  Replace
>> the other uses of check_name_str() by appropriate uses of the
>> wrappers.  No change in behavior just yet.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++-------------
>>  1 file changed, 36 insertions(+), 15 deletions(-)
>> 
>
>> +++ b/scripts/qapi/expr.py
>> @@ -21,11 +21,12 @@
>>  from .error import QAPISemError
>>  
>>  
>> -# Names must be letters, numbers, -, and _.  They must start with letter,
>> -# except for downstream extensions which must start with __RFQDN_.
>> -# Dots are only valid in the downstream extension prefix.
>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
>> -                        '[a-zA-Z][a-zA-Z0-9_-]*$')
>
> I'm assuming python concatenates r'' with '' in the obvious manner...
>
>> +# Names consist of letters, digits, -, and _, starting with a letter.
>> +# An experimental name is prefixed with x-.  A name of a downstream
>> +# extension is prefixed with __RFQDN_.  The latter prefix goes first.
>> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
>> +                        r'(x-)?'
>> +                        r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
>
> ...but like your explicit use of r'' r''.
>
> Splitting out special handling of r'(x-)?' does not change behavior, but
> is not otherwise mentioned in your commit message.  I suspect you did it
> to make it easier to permit x-EVENT_NAME in later patches where upper is
> handled differently from lower or camel,

Yes.

>                                          so I won't withhold R-b, but it
> may be worth a tweak to the commit message.

Probably.  I'm failing at coming up with a concise text that isn't
confusing.

>>  def check_defn_name_str(name, info, meta):
>> -    check_name_str(name, info, meta, permit_upper=True)
>> +    if meta == 'event':
>> +        check_name_upper(name, info, meta)
>> +    elif meta == 'command':
>> +        check_name_lower(name, info, meta, permit_upper=True)
>
> Why do commands need to permit upper?  I guess just downstream FQDN
> extensions?

This is just so that the patch doesn't change behavior.  PATCH 24 will
flip it to False.

> Otherwise the patch makes sense.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Markus Armbruster <armbru@redhat.com> writes:

> Eric Blake <eblake@redhat.com> writes:
>
>> On 3/23/21 4:40 AM, Markus Armbruster wrote:
>>> Naming rules differ for the various kinds of names.  To prepare
>>> enforcing them, define functions to check them: check_name_upper(),
>>> check_name_lower(), and check_name_camel().  For now, these merely
>>> wrap around check_name_str(), but that will change shortly.  Replace
>>> the other uses of check_name_str() by appropriate uses of the
>>> wrappers.  No change in behavior just yet.
>>> 
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++-------------
>>>  1 file changed, 36 insertions(+), 15 deletions(-)
>>> 
>>
>>> +++ b/scripts/qapi/expr.py
>>> @@ -21,11 +21,12 @@
>>>  from .error import QAPISemError
>>>  
>>>  
>>> -# Names must be letters, numbers, -, and _.  They must start with letter,
>>> -# except for downstream extensions which must start with __RFQDN_.
>>> -# Dots are only valid in the downstream extension prefix.
>>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
>>> -                        '[a-zA-Z][a-zA-Z0-9_-]*$')
>>
>> I'm assuming python concatenates r'' with '' in the obvious manner...
>>
>>> +# Names consist of letters, digits, -, and _, starting with a letter.
>>> +# An experimental name is prefixed with x-.  A name of a downstream
>>> +# extension is prefixed with __RFQDN_.  The latter prefix goes first.
>>> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
>>> +                        r'(x-)?'
>>> +                        r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
>>
>> ...but like your explicit use of r'' r''.
>>
>> Splitting out special handling of r'(x-)?' does not change behavior, but
>> is not otherwise mentioned in your commit message.  I suspect you did it
>> to make it easier to permit x-EVENT_NAME in later patches where upper is
>> handled differently from lower or camel,
>
> Yes.
>
>>                                          so I won't withhold R-b, but it
>> may be worth a tweak to the commit message.
>
> Probably.  I'm failing at coming up with a concise text that isn't
> confusing.

Adding this paragraph:

    check_name_str() now returns the name without downstream and x-
    prefix, for use by the wrappers in later patches.  Requires tweaking
    regexp @valid_name.  It accepts the same strings as before.

On 3/23/21 5:40 AM, Markus Armbruster wrote:
> Naming rules differ for the various kinds of names.  To prepare
> enforcing them, define functions to check them: check_name_upper(),
> check_name_lower(), and check_name_camel().  For now, these merely
> wrap around check_name_str(), but that will change shortly.  Replace
> the other uses of check_name_str() by appropriate uses of the
> wrappers.  No change in behavior just yet.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++-------------
>   1 file changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index e00467636c..30285fe334 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -21,11 +21,12 @@
>   from .error import QAPISemError
>   
>   
> -# Names must be letters, numbers, -, and _.  They must start with letter,
> -# except for downstream extensions which must start with __RFQDN_.
> -# Dots are only valid in the downstream extension prefix.
> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
> -                        '[a-zA-Z][a-zA-Z0-9_-]*$')
> +# Names consist of letters, digits, -, and _, starting with a letter.
> +# An experimental name is prefixed with x-.  A name of a downstream
> +# extension is prefixed with __RFQDN_.  The latter prefix goes first.
> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
> +                        r'(x-)?'
> +                        r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
>   
>   
>   def check_name_is_str(name, info, source):
> @@ -37,16 +38,38 @@ def check_name_str(name, info, source,
>                      permit_upper=False):
>       # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
>       # and 'q_obj_*' implicit type names.
> -    if not valid_name.match(name) or \
> -       c_name(name, False).startswith('q_'):
> +    match = valid_name.match(name)
> +    if not match or c_name(name, False).startswith('q_'):
>           raise QAPISemError(info, "%s has an invalid name" % source)
>       if not permit_upper and name.lower() != name:
>           raise QAPISemError(
>               info, "%s uses uppercase in name" % source)
> +    return match.group(3)
> +
> +
> +def check_name_upper(name, info, source):
> +    stem = check_name_str(name, info, source, permit_upper=True)
> +    # TODO reject '[a-z-]' in @stem
> +

Creates (presumably) temporary errors in flake8 for the dead assignment 
here and below.

> +
> +def check_name_lower(name, info, source,
> +                     permit_upper=False):
> +    stem = check_name_str(name, info, source, permit_upper)
> +    # TODO reject '_' in stem
> +
> +
> +def check_name_camel(name, info, source):
> +    stem = check_name_str(name, info, source, permit_upper=True)
> +    # TODO reject '[_-]' in stem, require CamelCase
>   
>   
>   def check_defn_name_str(name, info, meta):
> -    check_name_str(name, info, meta, permit_upper=True)
> +    if meta == 'event':
> +        check_name_upper(name, info, meta)
> +    elif meta == 'command':
> +        check_name_lower(name, info, meta, permit_upper=True)
> +    else:
> +        check_name_camel(name, info, meta)
>       if name.endswith('Kind') or name.endswith('List'):
>           raise QAPISemError(
>               info, "%s name should not end in '%s'" % (meta, name[-4:]))
> @@ -163,8 +186,7 @@ def check_type(value, info, source,
>           key_source = "%s member '%s'" % (source, key)
>           if key.startswith('*'):
>               key = key[1:]
> -        check_name_str(key, info, key_source,
> -                       permit_upper=permit_upper)
> +        check_name_lower(key, info, key_source, permit_upper)
>           if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
>               raise QAPISemError(info, "%s uses reserved name" % key_source)
>           check_keys(arg, info, key_source, ['type'], ['if', 'features'])
> @@ -186,7 +208,7 @@ def check_features(features, info):
>           check_keys(f, info, source, ['name'], ['if'])
>           check_name_is_str(f['name'], info, source)
>           source = "%s '%s'" % (source, f['name'])
> -        check_name_str(f['name'], info, source)
> +        check_name_lower(f['name'], info, source)
>           check_if(f, info, source)
>   
>   
> @@ -213,8 +235,7 @@ def check_enum(expr, info):
>           # Enum members may start with a digit
>           if member_name[0].isdigit():
>               member_name = 'd' + member_name # Hack: hide the digit
> -        check_name_str(member_name, info, source,
> -                       permit_upper=permit_upper)
> +        check_name_lower(member_name, info, source, permit_upper)
>           check_if(member, info, source)
>   
>   
> @@ -244,7 +265,7 @@ def check_union(expr, info):
>       for (key, value) in members.items():
>           source = "'data' member '%s'" % key
>           if discriminator is None:
> -            check_name_str(key, info, source)
> +            check_name_lower(key, info, source)
>           # else: name is in discriminator enum, which gets checked
>           check_keys(value, info, source, ['type'], ['if'])
>           check_if(value, info, source)
> @@ -258,7 +279,7 @@ def check_alternate(expr, info):
>           raise QAPISemError(info, "'data' must not be empty")
>       for (key, value) in members.items():
>           source = "'data' member '%s'" % key
> -        check_name_str(key, info, source)
> +        check_name_lower(key, info, source)
>           check_keys(value, info, source, ['type'], ['if'])
>           check_if(value, info, source)
>           check_type(value['type'], info, source)
>

John Snow <jsnow@redhat.com> writes:

> On 3/23/21 5:40 AM, Markus Armbruster wrote:
>> Naming rules differ for the various kinds of names.  To prepare
>> enforcing them, define functions to check them: check_name_upper(),
>> check_name_lower(), and check_name_camel().  For now, these merely
>> wrap around check_name_str(), but that will change shortly.  Replace
>> the other uses of check_name_str() by appropriate uses of the
>> wrappers.  No change in behavior just yet.
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++-------------
>>   1 file changed, 36 insertions(+), 15 deletions(-)
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index e00467636c..30285fe334 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -21,11 +21,12 @@
>>   from .error import QAPISemError
>>     
>> -# Names must be letters, numbers, -, and _.  They must start with letter,
>> -# except for downstream extensions which must start with __RFQDN_.
>> -# Dots are only valid in the downstream extension prefix.
>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
>> -                        '[a-zA-Z][a-zA-Z0-9_-]*$')
>> +# Names consist of letters, digits, -, and _, starting with a letter.
>> +# An experimental name is prefixed with x-.  A name of a downstream
>> +# extension is prefixed with __RFQDN_.  The latter prefix goes first.
>> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
>> +                        r'(x-)?'
>> +                        r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
>>     
>>   def check_name_is_str(name, info, source):
>> @@ -37,16 +38,38 @@ def check_name_str(name, info, source,
>>                      permit_upper=False):
>>       # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
>>       # and 'q_obj_*' implicit type names.
>> -    if not valid_name.match(name) or \
>> -       c_name(name, False).startswith('q_'):
>> +    match = valid_name.match(name)
>> +    if not match or c_name(name, False).startswith('q_'):
>>           raise QAPISemError(info, "%s has an invalid name" % source)
>>       if not permit_upper and name.lower() != name:
>>           raise QAPISemError(
>>               info, "%s uses uppercase in name" % source)
>> +    return match.group(3)
>> +
>> +
>> +def check_name_upper(name, info, source):
>> +    stem = check_name_str(name, info, source, permit_upper=True)
>> +    # TODO reject '[a-z-]' in @stem
>> +
>
> Creates (presumably) temporary errors in flake8 for the dead
> assignment here and below.

All gone by the end of the series.

"make check" and checkpatch were content.  Anything else you'd like me
to run?

On 3/24/21 1:57 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 3/23/21 5:40 AM, Markus Armbruster wrote:
>>> Naming rules differ for the various kinds of names.  To prepare
>>> enforcing them, define functions to check them: check_name_upper(),
>>> check_name_lower(), and check_name_camel().  For now, these merely
>>> wrap around check_name_str(), but that will change shortly.  Replace
>>> the other uses of check_name_str() by appropriate uses of the
>>> wrappers.  No change in behavior just yet.
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>    scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++-------------
>>>    1 file changed, 36 insertions(+), 15 deletions(-)
>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>> index e00467636c..30285fe334 100644
>>> --- a/scripts/qapi/expr.py
>>> +++ b/scripts/qapi/expr.py
>>> @@ -21,11 +21,12 @@
>>>    from .error import QAPISemError
>>>      
>>> -# Names must be letters, numbers, -, and _.  They must start with letter,
>>> -# except for downstream extensions which must start with __RFQDN_.
>>> -# Dots are only valid in the downstream extension prefix.
>>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
>>> -                        '[a-zA-Z][a-zA-Z0-9_-]*$')
>>> +# Names consist of letters, digits, -, and _, starting with a letter.
>>> +# An experimental name is prefixed with x-.  A name of a downstream
>>> +# extension is prefixed with __RFQDN_.  The latter prefix goes first.
>>> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
>>> +                        r'(x-)?'
>>> +                        r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
>>>      
>>>    def check_name_is_str(name, info, source):
>>> @@ -37,16 +38,38 @@ def check_name_str(name, info, source,
>>>                       permit_upper=False):
>>>        # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
>>>        # and 'q_obj_*' implicit type names.
>>> -    if not valid_name.match(name) or \
>>> -       c_name(name, False).startswith('q_'):
>>> +    match = valid_name.match(name)
>>> +    if not match or c_name(name, False).startswith('q_'):
>>>            raise QAPISemError(info, "%s has an invalid name" % source)
>>>        if not permit_upper and name.lower() != name:
>>>            raise QAPISemError(
>>>                info, "%s uses uppercase in name" % source)
>>> +    return match.group(3)
>>> +
>>> +
>>> +def check_name_upper(name, info, source):
>>> +    stem = check_name_str(name, info, source, permit_upper=True)
>>> +    # TODO reject '[a-z-]' in @stem
>>> +
>>
>> Creates (presumably) temporary errors in flake8 for the dead
>> assignment here and below.
> 
> All gone by the end of the series.
> 
> "make check" and checkpatch were content.  Anything else you'd like me
> to run?
> 

Eventually it'll be part of CI, with targets to run locally.

I never expected the process to take this long, so I did not invest my 
time in developing an interim solution.

I use a hastily written script to do my own testing, which I run for 
every commit that touches QAPI:

#!/usr/bin/env bash
set -e

if [[ -f qapi/.flake8 ]]; then
     echo "flake8 --config=qapi/.flake8 qapi/"
     flake8 --config=qapi/.flake8 qapi/
fi
if [[ -f qapi/pylintrc ]]; then
     echo "pylint --rcfile=qapi/pylintrc qapi/"
     pylint --rcfile=qapi/pylintrc qapi/
fi
if [[ -f qapi/mypy.ini ]]; then
     echo "mypy --config-file=qapi/mypy.ini qapi/"
     mypy --config-file=qapi/mypy.ini qapi/
fi

if [[ -f qapi/.isort.cfg ]]; then
     pushd qapi
     echo "isort -c ."
     isort -c .
     popd
fi

pushd ../bin/git
make -j9
make check-qapi-schema
popd

John Snow <jsnow@redhat.com> writes:

> On 3/24/21 1:57 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 3/23/21 5:40 AM, Markus Armbruster wrote:
>>>> Naming rules differ for the various kinds of names.  To prepare
>>>> enforcing them, define functions to check them: check_name_upper(),
>>>> check_name_lower(), and check_name_camel().  For now, these merely
>>>> wrap around check_name_str(), but that will change shortly.  Replace
>>>> the other uses of check_name_str() by appropriate uses of the
>>>> wrappers.  No change in behavior just yet.
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>    scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++-------------
>>>>    1 file changed, 36 insertions(+), 15 deletions(-)
>>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>>> index e00467636c..30285fe334 100644
>>>> --- a/scripts/qapi/expr.py
>>>> +++ b/scripts/qapi/expr.py
>>>> @@ -21,11 +21,12 @@
>>>>    from .error import QAPISemError
>>>>      -# Names must be letters, numbers, -, and _.  They must start
>>>> with letter,
>>>> -# except for downstream extensions which must start with __RFQDN_.
>>>> -# Dots are only valid in the downstream extension prefix.
>>>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
>>>> -                        '[a-zA-Z][a-zA-Z0-9_-]*$')
>>>> +# Names consist of letters, digits, -, and _, starting with a letter.
>>>> +# An experimental name is prefixed with x-.  A name of a downstream
>>>> +# extension is prefixed with __RFQDN_.  The latter prefix goes first.
>>>> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
>>>> +                        r'(x-)?'
>>>> +                        r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
>>>>         def check_name_is_str(name, info, source):
>>>> @@ -37,16 +38,38 @@ def check_name_str(name, info, source,
>>>>                       permit_upper=False):
>>>>        # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
>>>>        # and 'q_obj_*' implicit type names.
>>>> -    if not valid_name.match(name) or \
>>>> -       c_name(name, False).startswith('q_'):
>>>> +    match = valid_name.match(name)
>>>> +    if not match or c_name(name, False).startswith('q_'):
>>>>            raise QAPISemError(info, "%s has an invalid name" % source)
>>>>        if not permit_upper and name.lower() != name:
>>>>            raise QAPISemError(
>>>>                info, "%s uses uppercase in name" % source)
>>>> +    return match.group(3)
>>>> +
>>>> +
>>>> +def check_name_upper(name, info, source):
>>>> +    stem = check_name_str(name, info, source, permit_upper=True)
>>>> +    # TODO reject '[a-z-]' in @stem
>>>> +
>>>
>>> Creates (presumably) temporary errors in flake8 for the dead
>>> assignment here and below.
>> 
>> All gone by the end of the series.
>> 
>> "make check" and checkpatch were content.  Anything else you'd like me
>> to run?
>
> Eventually it'll be part of CI, with targets to run locally.
>
> I never expected the process to take this long, so I did not invest my
> time in developing an interim solution.
>
> I use a hastily written script to do my own testing, which I run for
> every commit that touches QAPI:
>
> #!/usr/bin/env bash
> set -e
>
> if [[ -f qapi/.flake8 ]]; then
>     echo "flake8 --config=qapi/.flake8 qapi/"
>     flake8 --config=qapi/.flake8 qapi/
> fi
> if [[ -f qapi/pylintrc ]]; then
>     echo "pylint --rcfile=qapi/pylintrc qapi/"
>     pylint --rcfile=qapi/pylintrc qapi/
> fi
> if [[ -f qapi/mypy.ini ]]; then
>     echo "mypy --config-file=qapi/mypy.ini qapi/"
>     mypy --config-file=qapi/mypy.ini qapi/
> fi
>
> if [[ -f qapi/.isort.cfg ]]; then
>     pushd qapi
>     echo "isort -c ."
>     isort -c .
>     popd
> fi
>
> pushd ../bin/git
> make -j9
> make check-qapi-schema
> popd

Thanks for sharing this!

Apropos qapi-gen testing scripts.  I have scripts to show me how the
generated code changes along the way in a branch.  They evolved over a
long time, and try to cope with changes in the tree that are hardly
relevant anymore.  By now, they could quite possibly make Frankenstein
recoil in horror.

As a secondary purpose, the scripts show me how output of pycodestyle-3
and pylint change.  This would be uninteresting if the code in master
was clean against a useful configuration of these tools.  Your work has
been making it less interesting.

On 3/25/21 2:18 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 3/24/21 1:57 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> On 3/23/21 5:40 AM, Markus Armbruster wrote:
>>>>> Naming rules differ for the various kinds of names.  To prepare
>>>>> enforcing them, define functions to check them: check_name_upper(),
>>>>> check_name_lower(), and check_name_camel().  For now, these merely
>>>>> wrap around check_name_str(), but that will change shortly.  Replace
>>>>> the other uses of check_name_str() by appropriate uses of the
>>>>> wrappers.  No change in behavior just yet.
>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>>> ---
>>>>>     scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++-------------
>>>>>     1 file changed, 36 insertions(+), 15 deletions(-)
>>>>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>>>>> index e00467636c..30285fe334 100644
>>>>> --- a/scripts/qapi/expr.py
>>>>> +++ b/scripts/qapi/expr.py
>>>>> @@ -21,11 +21,12 @@
>>>>>     from .error import QAPISemError
>>>>>       -# Names must be letters, numbers, -, and _.  They must start
>>>>> with letter,
>>>>> -# except for downstream extensions which must start with __RFQDN_.
>>>>> -# Dots are only valid in the downstream extension prefix.
>>>>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
>>>>> -                        '[a-zA-Z][a-zA-Z0-9_-]*$')
>>>>> +# Names consist of letters, digits, -, and _, starting with a letter.
>>>>> +# An experimental name is prefixed with x-.  A name of a downstream
>>>>> +# extension is prefixed with __RFQDN_.  The latter prefix goes first.
>>>>> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
>>>>> +                        r'(x-)?'
>>>>> +                        r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
>>>>>          def check_name_is_str(name, info, source):
>>>>> @@ -37,16 +38,38 @@ def check_name_str(name, info, source,
>>>>>                        permit_upper=False):
>>>>>         # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty'
>>>>>         # and 'q_obj_*' implicit type names.
>>>>> -    if not valid_name.match(name) or \
>>>>> -       c_name(name, False).startswith('q_'):
>>>>> +    match = valid_name.match(name)
>>>>> +    if not match or c_name(name, False).startswith('q_'):
>>>>>             raise QAPISemError(info, "%s has an invalid name" % source)
>>>>>         if not permit_upper and name.lower() != name:
>>>>>             raise QAPISemError(
>>>>>                 info, "%s uses uppercase in name" % source)
>>>>> +    return match.group(3)
>>>>> +
>>>>> +
>>>>> +def check_name_upper(name, info, source):
>>>>> +    stem = check_name_str(name, info, source, permit_upper=True)
>>>>> +    # TODO reject '[a-z-]' in @stem
>>>>> +
>>>>
>>>> Creates (presumably) temporary errors in flake8 for the dead
>>>> assignment here and below.
>>>
>>> All gone by the end of the series.
>>>
>>> "make check" and checkpatch were content.  Anything else you'd like me
>>> to run?
>>
>> Eventually it'll be part of CI, with targets to run locally.
>>
>> I never expected the process to take this long, so I did not invest my
>> time in developing an interim solution.
>>
>> I use a hastily written script to do my own testing, which I run for
>> every commit that touches QAPI:
>>
>> #!/usr/bin/env bash
>> set -e
>>
>> if [[ -f qapi/.flake8 ]]; then
>>      echo "flake8 --config=qapi/.flake8 qapi/"
>>      flake8 --config=qapi/.flake8 qapi/
>> fi
>> if [[ -f qapi/pylintrc ]]; then
>>      echo "pylint --rcfile=qapi/pylintrc qapi/"
>>      pylint --rcfile=qapi/pylintrc qapi/
>> fi
>> if [[ -f qapi/mypy.ini ]]; then
>>      echo "mypy --config-file=qapi/mypy.ini qapi/"
>>      mypy --config-file=qapi/mypy.ini qapi/
>> fi
>>
>> if [[ -f qapi/.isort.cfg ]]; then
>>      pushd qapi
>>      echo "isort -c ."
>>      isort -c .
>>      popd
>> fi
>>
>> pushd ../bin/git
>> make -j9
>> make check-qapi-schema
>> popd
> 
> Thanks for sharing this!
> 

My intent, eventually, was to get scripts/qapi under python/qemu/qapi; 
under which there is a testing framework thing I have been debating on 
and off with Cleber for a while that does (basically) the same thing as 
what my hasty script does, but more integrated with Python ecosystem.

It'll do a few things:

(1) Gets these checks on CI
(2) Allows developers to manually run the checks locally
(3) Allows developers to manually run the checks locally using a 
pre-determined set of pinned linter versions to guarantee synchronicity 
with CI

cd python/qemu && "make check" would do more or less the same as the 
hacky script above, "make venv-check" would create a virtual environment 
with precisely the right packages and then run the same.

I have been stalled there for a while, and I missed freeze deadline from 
illness. Anguish. For now, the dumb script in my scripts directory does 
the lifting I need it to, using packages I have selected in my local 
environment that just-so-happen to pass.

> Apropos qapi-gen testing scripts.  I have scripts to show me how the
> generated code changes along the way in a branch.  They evolved over a
> long time, and try to cope with changes in the tree that are hardly
> relevant anymore.  By now, they could quite possibly make Frankenstein
> recoil in horror.
> 

Are they in the tree? Largely if the generated code changes it's 
invisible to me, but I rely heavily on the unit tests. I guess maybe if 
they are not in a state to upstream it might not be worth the hassle to 
clean them, but I don't know.

> As a secondary purpose, the scripts show me how output of pycodestyle-3
> and pylint change.  This would be uninteresting if the code in master
> was clean against a useful configuration of these tools.  Your work has
> been making it less interesting.
> 

--js

John Snow <jsnow@redhat.com> writes:

> On 3/25/21 2:18 AM, Markus Armbruster wrote:
[...]
>> Apropos qapi-gen testing scripts.  I have scripts to show me how the
>> generated code changes along the way in a branch.  They evolved over a
>> long time, and try to cope with changes in the tree that are hardly
>> relevant anymore.  By now, they could quite possibly make Frankenstein
>> recoil in horror.
>> 
>
> Are they in the tree?

No, because in their current state, they are incomprehensible *and* need
frequent tinkering.

>                       Largely if the generated code changes it's 
> invisible to me, but I rely heavily on the unit tests. I guess maybe if 
> they are not in a state to upstream it might not be worth the hassle to 
> clean them, but I don't know.

The negative unit tests are fairly comprehensive, and guard against
screwing up error path reasonably well.

The positive unit tests compare the frontend state dumped by
test-qapi.py, and compile-test the generated code.  Reasonable
protection against frontend screwups.  Some protection against backend
screwups.  Plenty of unwanted code generation changes can go undetected.

A tool to show generated code changes for review is useful, and having
such a tool in the tree would be nice.

>> As a secondary purpose, the scripts show me how output of pycodestyle-3
>> and pylint change.  This would be uninteresting if the code in master
>> was clean against a useful configuration of these tools.  Your work has
>> been making it less interesting.
>> 
>
> --js