Re: [Qemu-devel] [PATCH v6 2/4] qapi: Use an explicit input file

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3174 bytes --]

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 <vilanova@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]