diff mbox

Makefile: Don't build generated headers before Makefile is reread

Message ID 1403974776-23966-1-git-send-email-peter.maydell@linaro.org
State Rejected
Headers show

Commit Message

Peter Maydell June 28, 2014, 4:59 p.m. UTC
Having a direct dependency
   Makefile: $(GENERATED_HEADERS)
can result in not-from-clean builds failing sometimes, because it means
that when Make does its "is any makefile or include out of date and
needing a rebuild?" check, as well as possibly running configure (to
update config-host.mak) it will also rebuild GENERATED_HEADERS under
the "old" config-host.mak regime. If the makefile rules for rebuilding
the generated headers have changed in a way that means they're not
compatible with the old config-host.mak variable names, the build will
fail. Even if it does work, it's wasted effort since we'll need to
rebuild them with the right config-host.mak settings immediately.

Instead, make the generated headers depend on config-host.mak
and config-target.mak. This means we'll definitely rebuild them
if the configuration changes, but it will be done after Make
reloads its makefiles and includes so will happen with the
correct set of config-host.mak settings.

We rely on the various .o files having correct autogenerated
dependency rules to cause the generated headers to be generated
as and when they are needed.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The specific example of this is the change from CONFIG_TRACE_BACKEND
to CONFIG_TRACE_BACKENDS, which I am still hitting on a pretty
regular basis. This patch fixes that problem and I don't think it
has any unpleasant side effects; Paolo, have I missed anything?

 Makefile        | 8 +++-----
 Makefile.target | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

Comments

Paolo Bonzini June 30, 2014, 12:09 p.m. UTC | #1
Il 28/06/2014 18:59, Peter Maydell ha scritto:
> Having a direct dependency
>    Makefile: $(GENERATED_HEADERS)
> can result in not-from-clean builds failing sometimes, because it means
> that when Make does its "is any makefile or include out of date and
> needing a rebuild?" check, as well as possibly running configure (to
> update config-host.mak) it will also rebuild GENERATED_HEADERS under
> the "old" config-host.mak regime. If the makefile rules for rebuilding
> the generated headers have changed in a way that means they're not
> compatible with the old config-host.mak variable names, the build will
> fail. Even if it does work, it's wasted effort since we'll need to
> rebuild them with the right config-host.mak settings immediately.
>
> Instead, make the generated headers depend on config-host.mak
> and config-target.mak. This means we'll definitely rebuild them
> if the configuration changes, but it will be done after Make
> reloads its makefiles and includes so will happen with the
> correct set of config-host.mak settings.
>
> We rely on the various .o files having correct autogenerated
> dependency rules to cause the generated headers to be generated
> as and when they are needed.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> The specific example of this is the change from CONFIG_TRACE_BACKEND
> to CONFIG_TRACE_BACKENDS, which I am still hitting on a pretty
> regular basis. This patch fixes that problem and I don't think it
> has any unpleasant side effects; Paolo, have I missed anything?
>
>  Makefile        | 8 +++-----
>  Makefile.target | 2 +-
>  2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 145adb6..dca33b4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -546,11 +546,9 @@ ifdef SIGNCODE
>  endif # SIGNCODE
>  endif # CONFIG_WIN
>
> -# Add a dependency on the generated files, so that they are always
> -# rebuilt before other object files
> -ifneq ($(filter-out %clean,$(MAKECMDGOALS)),$(if $(MAKECMDGOALS),,fail))
> -Makefile: $(GENERATED_HEADERS)
> -endif
> +# Make the generated files depend on config-host.mak so that if
> +# the configuration settings change we will rebuild them
> +$(GENERATED_HEADERS): config-host.mak
>
>  # Include automatically generated dependency files
>  # Dependencies in Makefile.objs files come from our recursive subdir rules
> diff --git a/Makefile.target b/Makefile.target
> index 6089d29..ea8614a 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -198,4 +198,4 @@ ifdef CONFIG_TRACE_SYSTEMTAP
>  endif
>
>  GENERATED_HEADERS += config-target.h
> -Makefile: $(GENERATED_HEADERS)
> +$(GENERATED_HEADERS): config-target.mak config-devices.mak

config-devices.mak is not reflected in any C header file.  Apart from this,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo
Peter Maydell June 30, 2014, 12:13 p.m. UTC | #2
On 30 June 2014 13:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 28/06/2014 18:59, Peter Maydell ha scritto:
>
>> Having a direct dependency
>>    Makefile: $(GENERATED_HEADERS)
>> can result in not-from-clean builds failing sometimes, because it means
>> that when Make does its "is any makefile or include out of date and
>> needing a rebuild?" check, as well as possibly running configure (to
>> update config-host.mak) it will also rebuild GENERATED_HEADERS under
>> the "old" config-host.mak regime. If the makefile rules for rebuilding
>> the generated headers have changed in a way that means they're not
>> compatible with the old config-host.mak variable names, the build will
>> fail. Even if it does work, it's wasted effort since we'll need to
>> rebuild them with the right config-host.mak settings immediately.
>>
>> Instead, make the generated headers depend on config-host.mak
>> and config-target.mak. This means we'll definitely rebuild them
>> if the configuration changes, but it will be done after Make
>> reloads its makefiles and includes so will happen with the
>> correct set of config-host.mak settings.
>>
>> We rely on the various .o files having correct autogenerated
>> dependency rules to cause the generated headers to be generated
>> as and when they are needed.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> The specific example of this is the change from CONFIG_TRACE_BACKEND
>> to CONFIG_TRACE_BACKENDS, which I am still hitting on a pretty
>> regular basis. This patch fixes that problem and I don't think it
>> has any unpleasant side effects; Paolo, have I missed anything?
>>
>>  Makefile        | 8 +++-----
>>  Makefile.target | 2 +-
>>  2 files changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 145adb6..dca33b4 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -546,11 +546,9 @@ ifdef SIGNCODE
>>  endif # SIGNCODE
>>  endif # CONFIG_WIN
>>
>> -# Add a dependency on the generated files, so that they are always
>> -# rebuilt before other object files
>> -ifneq ($(filter-out %clean,$(MAKECMDGOALS)),$(if $(MAKECMDGOALS),,fail))
>> -Makefile: $(GENERATED_HEADERS)
>> -endif
>> +# Make the generated files depend on config-host.mak so that if
>> +# the configuration settings change we will rebuild them
>> +$(GENERATED_HEADERS): config-host.mak
>>
>>  # Include automatically generated dependency files
>>  # Dependencies in Makefile.objs files come from our recursive subdir
>> rules
>> diff --git a/Makefile.target b/Makefile.target
>> index 6089d29..ea8614a 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -198,4 +198,4 @@ ifdef CONFIG_TRACE_SYSTEMTAP
>>  endif
>>
>>  GENERATED_HEADERS += config-target.h
>> -Makefile: $(GENERATED_HEADERS)
>> +$(GENERATED_HEADERS): config-target.mak config-devices.mak
>
>
> config-devices.mak is not reflected in any C header file.  Apart from this,
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Do you mean "...and therefore should not be listed on the
RHS of this dependency" ?

thanks
-- PMM
Paolo Bonzini June 30, 2014, 12:34 p.m. UTC | #3
Il 30/06/2014 14:13, Peter Maydell ha scritto:
>> > config-devices.mak is not reflected in any C header file.  Apart from this,
>> >
>> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Do you mean "...and therefore should not be listed on the
> RHS of this dependency" ?

It need not be listed; it doesn't hurt to leave it in, so whatever the 
submitter wants to do is fine. :)

Paolo
Peter Maydell June 30, 2014, 12:42 p.m. UTC | #4
On 30 June 2014 13:34, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 30/06/2014 14:13, Peter Maydell ha scritto:
>
>>> > config-devices.mak is not reflected in any C header file.  Apart from
>>> > this,
>>> >
>>> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Do you mean "...and therefore should not be listed on the
>> RHS of this dependency" ?
>
>
> It need not be listed; it doesn't hurt to leave it in, so whatever the
> submitter wants to do is fine. :)

In that case I will leave it be rather than re-rolling :-)
(Maybe one day we'll have a header that does reflect it...)

thanks
-- PMM
Paolo Bonzini June 30, 2014, 12:45 p.m. UTC | #5
Il 30/06/2014 14:42, Peter Maydell ha scritto:
> In that case I will leave it be rather than re-rolling :-)
> (Maybe one day we'll have a header that does reflect it...)

So far we never had the need and Blue Swirl objected to it (in favor of 
run-time probing of QOM classes), but you never know.

Paolo
Peter Maydell July 4, 2014, 2:33 p.m. UTC | #6
On 30 June 2014 13:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 28/06/2014 18:59, Peter Maydell ha scritto:
>
>> Having a direct dependency
>>    Makefile: $(GENERATED_HEADERS)
>> can result in not-from-clean builds failing sometimes, because it means
>> that when Make does its "is any makefile or include out of date and
>> needing a rebuild?" check, as well as possibly running configure (to
>> update config-host.mak) it will also rebuild GENERATED_HEADERS under
>> the "old" config-host.mak regime. If the makefile rules for rebuilding
>> the generated headers have changed in a way that means they're not
>> compatible with the old config-host.mak variable names, the build will
>> fail. Even if it does work, it's wasted effort since we'll need to
>> rebuild them with the right config-host.mak settings immediately.
>>
>> Instead, make the generated headers depend on config-host.mak
>> and config-target.mak. This means we'll definitely rebuild them
>> if the configuration changes, but it will be done after Make
>> reloads its makefiles and includes so will happen with the
>> correct set of config-host.mak settings.
>>
>> We rely on the various .o files having correct autogenerated
>> dependency rules to cause the generated headers to be generated
>> as and when they are needed.

> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>


Any objections to this going into 2.1 for rc1 ?

-- PMM
Paolo Bonzini July 4, 2014, 3:47 p.m. UTC | #7
Il 04/07/2014 16:33, Peter Maydell ha scritto:
> On 30 June 2014 13:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 28/06/2014 18:59, Peter Maydell ha scritto:
>>
>>> Having a direct dependency
>>>    Makefile: $(GENERATED_HEADERS)
>>> can result in not-from-clean builds failing sometimes, because it means
>>> that when Make does its "is any makefile or include out of date and
>>> needing a rebuild?" check, as well as possibly running configure (to
>>> update config-host.mak) it will also rebuild GENERATED_HEADERS under
>>> the "old" config-host.mak regime. If the makefile rules for rebuilding
>>> the generated headers have changed in a way that means they're not
>>> compatible with the old config-host.mak variable names, the build will
>>> fail. Even if it does work, it's wasted effort since we'll need to
>>> rebuild them with the right config-host.mak settings immediately.
>>>
>>> Instead, make the generated headers depend on config-host.mak
>>> and config-target.mak. This means we'll definitely rebuild them
>>> if the configuration changes, but it will be done after Make
>>> reloads its makefiles and includes so will happen with the
>>> correct set of config-host.mak settings.
>>>
>>> We rely on the various .o files having correct autogenerated
>>> dependency rules to cause the generated headers to be generated
>>> as and when they are needed.
>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
>
> Any objections to this going into 2.1 for rc1 ?

Not from me.

Paolo
Peter Maydell July 8, 2014, 12:03 p.m. UTC | #8
On 4 July 2014 15:33, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 June 2014 13:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 28/06/2014 18:59, Peter Maydell ha scritto:
>>
>>> Having a direct dependency
>>>    Makefile: $(GENERATED_HEADERS)
>>> can result in not-from-clean builds failing sometimes, because it means
>>> that when Make does its "is any makefile or include out of date and
>>> needing a rebuild?" check, as well as possibly running configure (to
>>> update config-host.mak) it will also rebuild GENERATED_HEADERS under
>>> the "old" config-host.mak regime. If the makefile rules for rebuilding
>>> the generated headers have changed in a way that means they're not
>>> compatible with the old config-host.mak variable names, the build will
>>> fail. Even if it does work, it's wasted effort since we'll need to
>>> rebuild them with the right config-host.mak settings immediately.
>>>
>>> Instead, make the generated headers depend on config-host.mak
>>> and config-target.mak. This means we'll definitely rebuild them
>>> if the configuration changes, but it will be done after Make
>>> reloads its makefiles and includes so will happen with the
>>> correct set of config-host.mak settings.
>>>
>>> We rely on the various .o files having correct autogenerated
>>> dependency rules to cause the generated headers to be generated
>>> as and when they are needed.
>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> Any objections to this going into 2.1 for rc1 ?

Unfortunately it doesn't seem to work correctly for a build
from clean:

e104462:trusty:qemu-for-merges$ make -C build/alldbg V=1
make: Entering directory `/home/petmay01/linaro/qemu-for-merges/build/alldbg'
libtool  --mode=compile --tag=CC ccache gcc
-I/home/petmay01/linaro/qemu-for-merges/tcg
-I/home/petmay01/linaro/qemu-for-merges/tcg/i386
-I/home/petmay01/linaro/qemu-for-merges/linux-headers
-I/home/petmay01/linaro/qemu-for-merges/build/alldbg/linux-headers -I.
-I/home/petmay01/linaro/qemu-for-merges
-I/home/petmay01/linaro/qemu-for-merges/include
-I/home/petmay01/linaro/qemu-for-merges/libcacard -Ilibcacard -Werror
-fPIE -DPIE -m64 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64
-D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall
-Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing
-fno-common  -Wendif-labels -Wmissing-include-dirs -Wempty-body
-Wnested-externs -Wformat-security -Wformat-y2k -Winit-self
-Wignored-qualifiers -Wold-style-declaration -Wold-style-definition
-Wtype-limits -fstack-protector-all   -I/usr/include/p11-kit-1
-I/usr/include/p11-kit-1     -I/usr/include/libpng12
-I/usr/include/spice-server -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1
-I/usr/include/spice-1   -I/usr/include/libusb-1.0
-I/usr/include/pixman-1
-I/home/petmay01/linaro/qemu-for-merges/tests -MMD -MP -MT
libcacard/cac.lo -MF libcacard/cac.d -pthread -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include   -g   -c -o
libcacard/cac.lo /home/petmay01/linaro/qemu-for-merges/libcacard/cac.c
libtool: compile:  ccache gcc
-I/home/petmay01/linaro/qemu-for-merges/tcg
-I/home/petmay01/linaro/qemu-for-merges/tcg/i386
-I/home/petmay01/linaro/qemu-for-merges/linux-headers
-I/home/petmay01/linaro/qemu-for-merges/build/alldbg/linux-headers -I.
-I/home/petmay01/linaro/qemu-for-merges
-I/home/petmay01/linaro/qemu-for-merges/include
-I/home/petmay01/linaro/qemu-for-merges/libcacard -Ilibcacard -Werror
-DPIE -m64 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
-Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings
-Wmissing-prototypes -fno-strict-aliasing -fno-common -Wendif-labels
-Wmissing-include-dirs -Wempty-body -Wnested-externs -Wformat-security
-Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration
-Wold-style-definition -Wtype-limits -fstack-protector-all
-I/usr/include/p11-kit-1 -I/usr/include/p11-kit-1
-I/usr/include/libpng12 -I/usr/include/spice-server
-I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include
-I/usr/include/pixman-1 -I/usr/include/spice-1
-I/usr/include/libusb-1.0 -I/usr/include/pixman-1
-I/home/petmay01/linaro/qemu-for-merges/tests -MMD -MP -MT
libcacard/cac.lo -MF libcacard/cac.d -pthread -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -g -c
/home/petmay01/linaro/qemu-for-merges/libcacard/cac.c  -fPIC -DPIC -o
libcacard/.libs/cac.o
In file included from
/home/petmay01/linaro/qemu-for-merges/include/qemu-common.h:15:0,
                 from /home/petmay01/linaro/qemu-for-merges/libcacard/cac.c:8:
/home/petmay01/linaro/qemu-for-merges/include/qemu/compiler.h:6:25:
fatal error: config-host.h: No such file or directory
 #include "config-host.h"
                         ^
compilation terminated.

We're trying to do a "build .o file and .d file in the same go"
gcc command, so the fact that we don't have the generated header
yet causes us to blow up. This would work if we built the .d files
separately using -MG to tell gcc to ignore missing headers...

thanks
-- PMM
diff mbox

Patch

diff --git a/Makefile b/Makefile
index 145adb6..dca33b4 100644
--- a/Makefile
+++ b/Makefile
@@ -546,11 +546,9 @@  ifdef SIGNCODE
 endif # SIGNCODE
 endif # CONFIG_WIN
 
-# Add a dependency on the generated files, so that they are always
-# rebuilt before other object files
-ifneq ($(filter-out %clean,$(MAKECMDGOALS)),$(if $(MAKECMDGOALS),,fail))
-Makefile: $(GENERATED_HEADERS)
-endif
+# Make the generated files depend on config-host.mak so that if
+# the configuration settings change we will rebuild them
+$(GENERATED_HEADERS): config-host.mak
 
 # Include automatically generated dependency files
 # Dependencies in Makefile.objs files come from our recursive subdir rules
diff --git a/Makefile.target b/Makefile.target
index 6089d29..ea8614a 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -198,4 +198,4 @@  ifdef CONFIG_TRACE_SYSTEMTAP
 endif
 
 GENERATED_HEADERS += config-target.h
-Makefile: $(GENERATED_HEADERS)
+$(GENERATED_HEADERS): config-target.mak config-devices.mak