Eric Blake <ebl...@redhat.com> writes:
> On 04/29/2014 02:20 PM, Lluís Vilanova wrote:
>> Markus Armbruster writes:
>>
>>> Lluís Vilanova <vilan...@ac.upc.edu> writes:
>>>> Use an explicit input file on the command-line instead of reading
>>>> from standard input
>>
>>> Please limit commit message line length to 70 characters.
>>
>>> Worth mentioning that this commit improves error messages!
>>
>>> <stdin>:123: Borked
>>
>>> becomes
>>
>>> qapi-schema.json:123: Borked
>>
>>> which enables Emacs to jump to the error.
>
>>>> @diff -q $(SRC_PATH)/$*.out $*.test.out
>>>> - @diff -q $(SRC_PATH)/$*.err $*.test.err
>>>> + @# Sanitize error messages (make them independent of build directory)
>>>> + @sed 's|$(SRC_PATH)/||g' $*.test.err | diff -q $(SRC_PATH)/$*.err -
>>>> @diff -q $(SRC_PATH)/$*.exit $*.test.exit
>>>>
>>
>>> Breaks when $(SRC_PATH) interpreted as regexp matches anything but a
>>> prefix of the source file part. In particular, it breaks when SRC_PATH
>>> is ".." (which is common) and the error message contains "/".
>>
>>> Here's a safer version:
>>
>>> @perl -p -e 's|^\Q$(SRC_PATH)\E/||' $*.test.err | diff -q
>>> $(SRC_PATH)/$*.err -
>
> I guess there's other ways to avoid the problems without using perl
> (such as sanitizing all metacharacters before passing a munged
> $(SRC_PATH) to sed), but it gets hairy.
Indeed, and that's why I prefer the santizing to be done by the regexp
package. Perl does it with escapes (anybody surprised?). Emacs
provides a function regexp-quote. sed provides... nothing.
Let me decipher the Perl regexp for you:
^ anchors to the beginning of the line
\Q$(SRC_PATH)\E matches $(SRC_PATH) exactly (\Q disables pattern
metacharacters until \E)
/ matches the / your script appends to $(SRC_PATH), which it
receives as argument of -o
Clear as mud?
Feel free to adopt something else, just make it work :)
>> Not a perl fan, so I'll assume your line as correct. Also, can perl be
>> assumed
>> as present?
>
> Makefile already uses perl to generate documentation from .pod files;
> although this appears to be the first use of perl in the testsuite.
There's one in tests/qemu-iotests/common.config's _readlink(), but it
appears unused.
We also have perl scripts checkpatch.pl and get_maintainer.pl.
>>> This one reports missing input file name argument as "IndexError: list
>>> index out of range". Again, fits right in.
>>
>>> [...]
>>
>> AFAIR, Eric said that it'd be better to leave it as simple as
>> possible, since no
>> one else uses this script.
>>
>> Same applies to the argument parsing of the other scripts (since no one is
>> sufficiently annoyed to rewrite it with argparse or similar).
>
> Careful - I think that keeping _this patch_ simple is okay, but I would
> also _welcome_ further patches in this (or other) series that further
> improves the error message quality and consistency when using the
> various .py generators from the command line. I think the point we are
> making here is that your code has very ugly inconsistent results when
> detecting errors across the various scripts, but that 1) it's no uglier
> than pre-patch, and 2) in the normal case, 'make' and 'make check' will
> not trigger these ugly error paths.
Exactly.
