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

[Lluís Vilanova]

Markus Armbruster writes:

> 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 :)

Yes, I just wanted to error-out in a more helpful way. I really hate to have
some arbitrary limit and an unhelpful (or unnecessarily long) message.


> 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/,'

Right, post-processing is the other option, silly me.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth