Re: [Qemu-devel] [PATCH v4 1/3] qapi: Use an explicit input file

[Markus Armbruster]

Lluís Vilanova <address@hidden> writes:

> Markus Armbruster writes:
> [...]
>>>>> self.fp = schema.fp
>>>>> self.msg = msg
>>>>> self.line = self.col = 1
>>>>> @@ -50,12 +52,17 @@ class QAPISchemaError(Exception):
>>>>> self.col += 1
>>>>> 
>>>>> def __str__(self):
>>>>> - return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col,
>>>>> self.msg)
>>>>> +        name = os.path.relpath(self.fp.name, self.base)
>>>>> +        return "%s:%s:%s: %s" % (name, self.line, self.col, self.msg)
>>> 
>>>> Can you explain what this change does, and why it's wanted?
>>> 
>>> Paths are shown as relative so that the test outputs (stderr) can be 
>>> verified
>>> with diff. Otherwise the actual message depends on the path from which 
>>> you're
>>> running the tests.
>
>> Hmm.  This is the applicable make rule:
>
>> $(patsubst %, check-%, $(check-qapi-schema-y)): check-%.json: 
>> $(SRC_PATH)/%.json
>>      $(call quiet-command, PYTHONPATH=$(SRC_PATH)/scripts $(PYTHON) 
>> $(SRC_PATH)/tests/qapi-schema/test-qapi.py "$^" >$*.test.out 2>$*.test.err; 
>> echo $$? >$*.test.exit, "  TEST  $*.out")
>>      @diff -q $(SRC_PATH)/$*.out $*.test.out
>>      @diff -q $(SRC_PATH)/$*.err $*.test.err
>>      @diff -q $(SRC_PATH)/$*.exit $*.test.exit
>
>> Since $^ is in $(SRC_PATH), it's like $(SRC_PATH)/foo.json.  If
>> $(SRC_PATH)/foo.json has an error, the error messages duly points to
>> $(SRC_PATH)/foo.json.
>
>> The "diff -q $(SRC_PATH)/$*.err $*.test.err" fails unless your SRC_PATH
>> matches the one that's encoded in tests/qapi-schema/foo.err.  Is that
>> the problem you're trying to solve?
>
> Right. Paths are internally stored as absolute in "qapi.py" (to check for
> include cycles), and the "base directory" is only used to show them as
> relative. I can try to change the code to retain this functionality but 
> without
> special-casing the tests.

Well, my fav method to check for include cycles is really simple:

1. Estimate how many levels of inclusion you're going to need.

2. Shift left a couple of times.

3. Error out when inclusion depth hits that limit.

Obviously 100% reliable.  File name comparisons tend to be unreliable or
unobvious :)

With this realpath business out of the way, file names in tests should
all be of the form $(SRC_PATH)/tests/qapi-schema/*.json, shouldn't they?

The $(SRC_PATH) prefix depends on where the user's build tree is, the
rest is fixed.  We could strip the prefix from error messages with a
simple filter:

    perl -pe 's,\Q$(SRC_PATH)/tests/qapi-schema/\E,tests/qapi-schema/,'