On 03/31/2014 01:16 PM, Lluís Vilanova wrote: > Use an explicit input file on the command-line instead of reading from > standard input > > Signed-off-by: Lluís Vilanova <vilan...@ac.upc.edu> > ---
> +++ b/Makefile
> @@ -238,33 +238,33 @@ qapi-py = $(SRC_PATH)/scripts/qapi.py
> $(SRC_PATH)/scripts/ordereddict.py
> qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h :\
> $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py \
> - $(gen-out-type) -o qga/qapi-generated -p "qga-" < $<, \
> + $(gen-out-type) -i "$<" -o qga/qapi-generated -p "qga-", \
I still wonder if:
-o qga/qapi-generated -p "qga-" -i "$<"
is easier to read than injecting -i up front. But it's not something
that I will block the patch for.
> @@ -368,7 +368,8 @@ check-tests/test-qapi.py: tests/test-qapi.py
>
> .PHONY: $(patsubst %, check-%, $(check-qapi-schema-y))
> $(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")
> + $(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")
Why is this line still long? Shouldn't it have been split in 1/4? And
why is this form using an argument of the input file, instead of adding
a -i option like all the others?
> + @sed -i -e "s|$(SRC_PATH)/||g" $*.test.err
'sed -i' is a GNU-ism, and not portable to all sed. It's not the first
use of 'sed -i' in the code base, but most of those uses so far are for
situations that are Linux-specific where GNU sed can be assumed.
> @diff -q $(SRC_PATH)/$*.out $*.test.out
> @diff -q $(SRC_PATH)/$*.err $*.test.err
You could instead have done:
@sed "s|$(SRC_PATH)/||g" $*.test.err \
| diff -q $(SRC_PATH_/$*.err -
to avoid the portability question.
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -12,10 +12,11 @@
>
> from qapi import *
> from pprint import pprint
> +import os
> import sys
>
> try:
> - exprs = parse_schema(sys.stdin)
> + exprs = parse_schema(sys.argv[1])
Here's where I found it inconsistent with the rest of the patch. It
seems it would be better to either use -i everywhere, or use sys.argv[1]
everywhere.
> +++ b/tests/qapi-schema/trailing-comma-list.err
> @@ -1 +1 @@
> -<stdin>:2:36: Expected "{", "[" or string
> +tests/qapi-schema/trailing-comma-list.json:2:36: Expected "{", "[" or string
These are some long error messages; is it also worth trimming the
leading "tests/qapi-schema/" in the sed script where you massage the
data before doing the diff on expected errors?
It's too late for this to go into 2.0, but I think we're getting close
to having a solution worth using at the start of the 2.1 devel cycle.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
