Patchwork [1/2] Makefile: Remove BUILD_DIR from qapi-dir

login
register
mail settings
Submitter Stefan Weil
Date June 9, 2012, 7:08 a.m.
Message ID <1339225719-5434-1-git-send-email-sw@weilnetz.de>
Download mbox | patch
Permalink /patch/163889/
State Accepted
Headers show

Comments

Stefan Weil - June 9, 2012, 7:08 a.m.
qapi-dir does not need an absolute path. All other build directories
are relative. When BUILD_DIR is removed, the build output looks better
(no long lines with absolute paths when everything else uses short
lines):

  GEN   qapi-generated/qga-qapi-types.c
  CC    qapi-generated/qga-qapi-types.o
  GEN   qapi-generated/qga-qapi-visit.c
  CC    qapi-generated/qga-qapi-visit.o
  GEN   qapi-generated/qga-qmp-marshal.c
  CC    qapi-generated/qga-qmp-marshal.o

Using a relative path also avoids potential problems when BUILD_DIR
includes blanks.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 Makefile |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Stefan Hajnoczi - June 12, 2012, 9:46 a.m.
On Sat, Jun 09, 2012 at 09:08:38AM +0200, Stefan Weil wrote:
> qapi-dir does not need an absolute path. All other build directories
> are relative. When BUILD_DIR is removed, the build output looks better
> (no long lines with absolute paths when everything else uses short
> lines):
> 
>   GEN   qapi-generated/qga-qapi-types.c
>   CC    qapi-generated/qga-qapi-types.o
>   GEN   qapi-generated/qga-qapi-visit.c
>   CC    qapi-generated/qga-qapi-visit.o
>   GEN   qapi-generated/qga-qmp-marshal.c
>   CC    qapi-generated/qga-qmp-marshal.o
> 
> Using a relative path also avoids potential problems when BUILD_DIR
> includes blanks.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  Makefile |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, applied to the trivial patches tree:
https://github.com/stefanha/qemu/commits/trivial-patches

Stefan
Stefan Hajnoczi - June 12, 2012, 9:49 a.m.
On Sat, Jun 09, 2012 at 09:08:38AM +0200, Stefan Weil wrote:
> qapi-dir does not need an absolute path. All other build directories
> are relative. When BUILD_DIR is removed, the build output looks better
> (no long lines with absolute paths when everything else uses short
> lines):
> 
>   GEN   qapi-generated/qga-qapi-types.c
>   CC    qapi-generated/qga-qapi-types.o
>   GEN   qapi-generated/qga-qapi-visit.c
>   CC    qapi-generated/qga-qapi-visit.o
>   GEN   qapi-generated/qga-qmp-marshal.c
>   CC    qapi-generated/qga-qmp-marshal.o
> 
> Using a relative path also avoids potential problems when BUILD_DIR
> includes blanks.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  Makefile |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I merged this but have CCed Michael Roth to take a look.  qemu.git
contains an earlier commit to explicitly add $(BUILD_DIR):

commit 9b129408589b2ed7bb2cdea03d2aba46a5fd74d4
Author: Michael Roth <mdroth@linux.vnet.ibm.com>
Date:   Tue Nov 29 16:47:49 2011 -0600

    Makefile: use full path for qapi-generated directory
    
    Generally $(BUILD_DIR) == $(CURDIR), but that isn't necessarilly the
    case, so use $(BUILD_DIR)/qapi-generated for generated files to
    avoid potentionally sticking generating files in odd places outside
    the build's include paths.
    
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Does this rationale still apply?  Is it really a good ideal to remove
$(BUILD_DIR)?

Stefan
Michael Roth - June 12, 2012, 2:14 p.m.
On Tue, Jun 12, 2012 at 10:49:57AM +0100, Stefan Hajnoczi wrote:
> On Sat, Jun 09, 2012 at 09:08:38AM +0200, Stefan Weil wrote:
> > qapi-dir does not need an absolute path. All other build directories
> > are relative. When BUILD_DIR is removed, the build output looks better
> > (no long lines with absolute paths when everything else uses short
> > lines):
> > 
> >   GEN   qapi-generated/qga-qapi-types.c
> >   CC    qapi-generated/qga-qapi-types.o
> >   GEN   qapi-generated/qga-qapi-visit.c
> >   CC    qapi-generated/qga-qapi-visit.o
> >   GEN   qapi-generated/qga-qmp-marshal.c
> >   CC    qapi-generated/qga-qmp-marshal.o
> > 
> > Using a relative path also avoids potential problems when BUILD_DIR
> > includes blanks.
> > 
> > Signed-off-by: Stefan Weil <sw@weilnetz.de>
> > ---
> >  Makefile |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> I merged this but have CCed Michael Roth to take a look.  qemu.git
> contains an earlier commit to explicitly add $(BUILD_DIR):
> 
> commit 9b129408589b2ed7bb2cdea03d2aba46a5fd74d4
> Author: Michael Roth <mdroth@linux.vnet.ibm.com>
> Date:   Tue Nov 29 16:47:49 2011 -0600
> 
>     Makefile: use full path for qapi-generated directory
>     
>     Generally $(BUILD_DIR) == $(CURDIR), but that isn't necessarilly the
>     case, so use $(BUILD_DIR)/qapi-generated for generated files to
>     avoid potentionally sticking generating files in odd places outside
>     the build's include paths.
>     
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> Does this rationale still apply?  Is it really a good ideal to remove
> $(BUILD_DIR)?

Here's some context for the patch:

http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg03474.html

Originally a change was made to add a $(SRC_DIR) prefix to fix a build.
This triggered an unrelated bug, and was not quite correct. I submitted
a patch to fix the bug it triggered, and then a patch that used
$(BUILD_DIR) instead.

I'm not sure the latter was ever needed however, it looks like it was
just the classic issue of someone having a dirty $(SRC_DIR) and
subsequently switching to out of tree builds. I think the original issue
was simple a stale qapi-generated/ directory sitting in $(SRC_DIR) that
was taking precedence over the one being created in $(BUILD_DIR)

So, while having the $(BUILD_DIR) prefix does make the build safer in
this regard, I think it addresses a use-case that was never supported to
begin with. So I'd be fine with applying Stefan's patch if it makes
things cleaner/more consistent with everything else.

> 
> Stefan
>
Stefan Hajnoczi - June 13, 2012, 9:24 a.m.
On Tue, Jun 12, 2012 at 3:14 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> On Tue, Jun 12, 2012 at 10:49:57AM +0100, Stefan Hajnoczi wrote:
>> On Sat, Jun 09, 2012 at 09:08:38AM +0200, Stefan Weil wrote:
>> > qapi-dir does not need an absolute path. All other build directories
>> > are relative. When BUILD_DIR is removed, the build output looks better
>> > (no long lines with absolute paths when everything else uses short
>> > lines):
>> >
>> >   GEN   qapi-generated/qga-qapi-types.c
>> >   CC    qapi-generated/qga-qapi-types.o
>> >   GEN   qapi-generated/qga-qapi-visit.c
>> >   CC    qapi-generated/qga-qapi-visit.o
>> >   GEN   qapi-generated/qga-qmp-marshal.c
>> >   CC    qapi-generated/qga-qmp-marshal.o
>> >
>> > Using a relative path also avoids potential problems when BUILD_DIR
>> > includes blanks.
>> >
>> > Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> > ---
>> >  Makefile |    2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> I merged this but have CCed Michael Roth to take a look.  qemu.git
>> contains an earlier commit to explicitly add $(BUILD_DIR):
>>
>> commit 9b129408589b2ed7bb2cdea03d2aba46a5fd74d4
>> Author: Michael Roth <mdroth@linux.vnet.ibm.com>
>> Date:   Tue Nov 29 16:47:49 2011 -0600
>>
>>     Makefile: use full path for qapi-generated directory
>>
>>     Generally $(BUILD_DIR) == $(CURDIR), but that isn't necessarilly the
>>     case, so use $(BUILD_DIR)/qapi-generated for generated files to
>>     avoid potentionally sticking generating files in odd places outside
>>     the build's include paths.
>>
>>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>
>> Does this rationale still apply?  Is it really a good ideal to remove
>> $(BUILD_DIR)?
>
> Here's some context for the patch:
>
> http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg03474.html
>
> Originally a change was made to add a $(SRC_DIR) prefix to fix a build.
> This triggered an unrelated bug, and was not quite correct. I submitted
> a patch to fix the bug it triggered, and then a patch that used
> $(BUILD_DIR) instead.
>
> I'm not sure the latter was ever needed however, it looks like it was
> just the classic issue of someone having a dirty $(SRC_DIR) and
> subsequently switching to out of tree builds. I think the original issue
> was simple a stale qapi-generated/ directory sitting in $(SRC_DIR) that
> was taking precedence over the one being created in $(BUILD_DIR)
>
> So, while having the $(BUILD_DIR) prefix does make the build safer in
> this regard, I think it addresses a use-case that was never supported to
> begin with. So I'd be fine with applying Stefan's patch if it makes
> things cleaner/more consistent with everything else.

Thanks for explaining.  I will include Stefan Weil's patch.

Stefan

Patch

diff --git a/Makefile b/Makefile
index 32550cb..8880a3e 100644
--- a/Makefile
+++ b/Makefile
@@ -171,7 +171,7 @@  fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap
 qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
 	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > $@,"  GEN   $@")
 
-qapi-dir := $(BUILD_DIR)/qapi-generated
+qapi-dir := qapi-generated
 qemu-ga$(EXESUF): LIBS = $(LIBS_QGA)
 qemu-ga$(EXESUF): QEMU_CFLAGS += -I $(qapi-dir)