| Message ID | 20190521081227.30799-1-richardw.yang@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Makefile: simplify qapi-py definition with wildcard | expand |
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 <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.
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?
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.
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.
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 >
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 \
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(-)