[Qemu-devel] [PATCH] Makefile: simplify qapi-py definition with wildcard

[Qemu-devel] [PATCH] Makefile: simplify qapi-py definition with wildcard

[Wei Yang]

All the python script in scripts/qapi is used to generate qapi code. Use
wildcard to simplify it.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 Makefile | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index 155f066a20..38b74782d9 100644
--- a/Makefile
+++ b/Makefile
@@ -541,13 +541,7 @@ qemu-ga$(EXESUF): QEMU_CFLAGS += -I qga/qapi-generated
 qemu-keymap$(EXESUF): LIBS += $(XKBCOMMON_LIBS)
 qemu-keymap$(EXESUF): QEMU_CFLAGS += $(XKBCOMMON_CFLAGS)
 
-qapi-py = $(SRC_PATH)/scripts/qapi/commands.py \
-$(SRC_PATH)/scripts/qapi/events.py \
-$(SRC_PATH)/scripts/qapi/introspect.py \
-$(SRC_PATH)/scripts/qapi/types.py \
-$(SRC_PATH)/scripts/qapi/visit.py \
-$(SRC_PATH)/scripts/qapi/common.py \
-$(SRC_PATH)/scripts/qapi/doc.py \
+qapi-py = $(wildcard $(SRC_PATH)/scripts/qapi/*.py) \
 $(SRC_PATH)/scripts/qapi-gen.py
 
 qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h \
-- 
2.19.1

Re: [Qemu-devel] [PATCH] Makefile: simplify qapi-py definition with wildcard

[Eric Blake]

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

On 5/21/19 3:12 AM, Wei Yang wrote:
> All the python script in scripts/qapi is used to generate qapi code. Use
> wildcard to simplify it.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  Makefile | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)

I'm not a fan of $(wildcard). It makes your tarball creation
non-deterministic - if there is a leftover file from development that is
no longer part of the build, wildcard will still pick it up.  Explicit
lists are better.  I'm inclined to NACK this, but Markus has final say
since he maintains the qapi generator.

> 
> diff --git a/Makefile b/Makefile
> index 155f066a20..38b74782d9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -541,13 +541,7 @@ qemu-ga$(EXESUF): QEMU_CFLAGS += -I qga/qapi-generated
>  qemu-keymap$(EXESUF): LIBS += $(XKBCOMMON_LIBS)
>  qemu-keymap$(EXESUF): QEMU_CFLAGS += $(XKBCOMMON_CFLAGS)
>  
> -qapi-py = $(SRC_PATH)/scripts/qapi/commands.py \
> -$(SRC_PATH)/scripts/qapi/events.py \
> -$(SRC_PATH)/scripts/qapi/introspect.py \
> -$(SRC_PATH)/scripts/qapi/types.py \
> -$(SRC_PATH)/scripts/qapi/visit.py \
> -$(SRC_PATH)/scripts/qapi/common.py \
> -$(SRC_PATH)/scripts/qapi/doc.py \
> +qapi-py = $(wildcard $(SRC_PATH)/scripts/qapi/*.py) \
>  $(SRC_PATH)/scripts/qapi-gen.py
>  
>  qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h \
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH] Makefile: simplify qapi-py definition with wildcard

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 5/21/19 3:12 AM, Wei Yang wrote:
>> All the python script in scripts/qapi is used to generate qapi code. Use
>> wildcard to simplify it.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  Makefile | 8 +-------
>>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> I'm not a fan of $(wildcard). It makes your tarball creation
> non-deterministic - if there is a leftover file from development that is
> no longer part of the build, wildcard will still pick it up.  Explicit

Actually, in this case can "only" adds spurious prerequisites.

> lists are better.  I'm inclined to NACK this, but Markus has final say
> since he maintains the qapi generator.

I consider use of $(wildcard) for the purpose of collecting sources a
lazy mistake.

Re: [Qemu-devel] [PATCH] Makefile: simplify qapi-py definition with wildcard

[Wei Yang]

On Tue, May 21, 2019 at 07:00:12AM -0500, Eric Blake wrote:
>On 5/21/19 3:12 AM, Wei Yang wrote:
>> All the python script in scripts/qapi is used to generate qapi code. Use
>> wildcard to simplify it.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  Makefile | 8 +-------
>>  1 file changed, 1 insertion(+), 7 deletions(-)
>
>I'm not a fan of $(wildcard). It makes your tarball creation
>non-deterministic - if there is a leftover file from development that is
>no longer part of the build, wildcard will still pick it up.  Explicit
>lists are better.  I'm inclined to NACK this, but Markus has final say
>since he maintains the qapi generator.
>

Thanks for comment.

Yes, for tarball, the order is a problem. But those python scripts are not
used in tarballs.

For leftover files, you mean those files not used and should be deleted?

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH] Makefile: simplify qapi-py definition with wildcard

[Wei Yang]

On Tue, May 21, 2019 at 05:28:27PM +0200, Markus Armbruster wrote:
>Eric Blake <eblake@redhat.com> writes:
>
>> On 5/21/19 3:12 AM, Wei Yang wrote:
>>> All the python script in scripts/qapi is used to generate qapi code. Use
>>> wildcard to simplify it.
>>> 
>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>> ---
>>>  Makefile | 8 +-------
>>>  1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> I'm not a fan of $(wildcard). It makes your tarball creation
>> non-deterministic - if there is a leftover file from development that is
>> no longer part of the build, wildcard will still pick it up.  Explicit
>
>Actually, in this case can "only" adds spurious prerequisites.

Hmm... which spurious prerequisites will be added? I may miss something here.

>
>> lists are better.  I'm inclined to NACK this, but Markus has final say
>> since he maintains the qapi generator.
>
>I consider use of $(wildcard) for the purpose of collecting sources a
>lazy mistake.

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH] Makefile: simplify qapi-py definition with wildcard

[Eric Blake]

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

On 5/21/19 7:53 PM, Wei Yang wrote:
> On Tue, May 21, 2019 at 05:28:27PM +0200, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 5/21/19 3:12 AM, Wei Yang wrote:
>>>> All the python script in scripts/qapi is used to generate qapi code. Use
>>>> wildcard to simplify it.
>>>>
>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>>> ---
>>>>  Makefile | 8 +-------
>>>>  1 file changed, 1 insertion(+), 7 deletions(-)
>>>
>>> I'm not a fan of $(wildcard). It makes your tarball creation
>>> non-deterministic - if there is a leftover file from development that is
>>> no longer part of the build, wildcard will still pick it up.  Explicit
>>
>> Actually, in this case can "only" adds spurious prerequisites.
> 
> Hmm... which spurious prerequisites will be added? I may miss something here.

If I touch the file scripts/qapi/foo.py, then that becomes a
prerequisite, and will rebuild my files even though it doesn't need to.
And if the definition of $(qapi-py) is ever applied to determining which
files to include in a tarball, then my tarball will include foo.py, even
though it is not essential to the tarball, while the next person, who
does not have foo.py lying around in their directory, produces a
different tarball.  Non-deterministic results are bad, hence our rule of
thumb to avoid $(wildcard) when listing files in a Makefile.

> 
>>
>>> lists are better.  I'm inclined to NACK this, but Markus has final say
>>> since he maintains the qapi generator.
>>
>> I consider use of $(wildcard) for the purpose of collecting sources a
>> lazy mistake.

Which is another polite way of saying that this patch should not be applied.

If you WANT to factor things for less typing, you could use something like:

var=commands.py events.py introspect.py types.py visit.py common.py doc.py
$(addprefix $(SRC_PATH)/scripts/qapi/,$(var))

for some suitably-named var.  Or even factor out the .py suffix as well.
That doesn't use the problematic $(wildcard), but still lets you get
away with easier maintenance of adding a new file to the explicit list
should we add a file in the future.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH] Makefile: simplify qapi-py definition with wildcard

[Wei Yang]

On Tue, May 21, 2019 at 08:36:42PM -0500, Eric Blake wrote:
>On 5/21/19 7:53 PM, Wei Yang wrote:
>> On Tue, May 21, 2019 at 05:28:27PM +0200, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> writes:
>>>
>>>> On 5/21/19 3:12 AM, Wei Yang wrote:
>>>>> All the python script in scripts/qapi is used to generate qapi code. Use
>>>>> wildcard to simplify it.
>>>>>
>>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>>>> ---
>>>>>  Makefile | 8 +-------
>>>>>  1 file changed, 1 insertion(+), 7 deletions(-)
>>>>
>>>> I'm not a fan of $(wildcard). It makes your tarball creation
>>>> non-deterministic - if there is a leftover file from development that is
>>>> no longer part of the build, wildcard will still pick it up.  Explicit
>>>
>>> Actually, in this case can "only" adds spurious prerequisites.
>> 
>> Hmm... which spurious prerequisites will be added? I may miss something here.
>
>If I touch the file scripts/qapi/foo.py, then that becomes a
>prerequisite, and will rebuild my files even though it doesn't need to.
>And if the definition of $(qapi-py) is ever applied to determining which
>files to include in a tarball, then my tarball will include foo.py, even
>though it is not essential to the tarball, while the next person, who
>does not have foo.py lying around in their directory, produces a
>different tarball.  Non-deterministic results are bad, hence our rule of
>thumb to avoid $(wildcard) when listing files in a Makefile.
>

Ok, so $(wildcard) should be avoided in Makefile. Will not try to do this next
time.

>> 
>>>
>>>> lists are better.  I'm inclined to NACK this, but Markus has final say
>>>> since he maintains the qapi generator.
>>>
>>> I consider use of $(wildcard) for the purpose of collecting sources a
>>> lazy mistake.
>
>Which is another polite way of saying that this patch should not be applied.
>
>If you WANT to factor things for less typing, you could use something like:
>
>var=commands.py events.py introspect.py types.py visit.py common.py doc.py
>$(addprefix $(SRC_PATH)/scripts/qapi/,$(var))
>
>for some suitably-named var.  Or even factor out the .py suffix as well.
>That doesn't use the problematic $(wildcard), but still lets you get
>away with easier maintenance of adding a new file to the explicit list
>should we add a file in the future.
>
>-- 
>Eric Blake, Principal Software Engineer
>Red Hat, Inc.           +1-919-301-3226
>Virtualization:  qemu.org | libvirt.org
>




-- 
Wei Yang
Help you, Help me