Re: [Qemu-devel] [PATCH] make: improve check for stale generated files in source dir

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 02:36:01PM +0000, Peter Maydell wrote:
> On 20 March 2018 at 14:23, Daniel P. Berrangé <address@hidden> wrote:
> > When doing a build with builddir != srcdir, if any generated files are
> > accidentally present in srcdir from a previous build, these can cause
> > unexpected failures.
> >
> > Currently there is a rule that checks for existance of config-host.mak,
> > but there have been cases where config-host.mak is absent, while other
> > generated files still exist.
> >
> > Update the check to look at every file listed in $(GENERATED_FILES). To
> > do this we must move the check further down after $(GENERATED_FILES) has
> > been populated.
> >
> > Signed-off-by: Daniel P. Berrangé <address@hidden>
> > ---
> >  Makefile | 29 +++++++++++++++++++----------
> >  1 file changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index c8116694a0..73caabc019 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -52,16 +52,6 @@ endif
> >
> >  .git-submodule-status: git-submodule-update config-host.mak
> >
> > -# Check that we're not trying to do an out-of-tree build from
> > -# a tree that's been used for an in-tree build.
> > -ifneq ($(realpath $(SRC_PATH)),$(realpath .))
> > -ifneq ($(wildcard $(SRC_PATH)/config-host.mak),)
> > -$(error This is an out of tree build but your source tree ($(SRC_PATH)) \
> > -seems to have been used for an in-tree build. You can fix this by running \
> > -"$(MAKE) distclean && rm -rf *-linux-user *-softmmu" in your source tree)
> > -endif
> > -endif
> > -
> >  CONFIG_SOFTMMU := $(if $(filter %-softmmu,$(TARGET_DIRS)),y)
> >  CONFIG_USER_ONLY := $(if $(filter %-user,$(TARGET_DIRS)),y)
> >  CONFIG_XEN := $(CONFIG_XEN_BACKEND)
> > @@ -1027,6 +1017,25 @@ ifdef SIGNCODE
> >  endif # SIGNCODE
> >  endif # CONFIG_WIN
> >
> > +define nl
> > +
> > +
> > +endef
> > +
> > +CHECK_FILES = config-host.mak $(filter-out .git-submodule-status, 
> > $(GENERATED_FILES))
> > +UNEXPECTED_FILES = $(wildcard $(CHECK_FILES:%=$(SRC_PATH)/%))
> > +
> > +# Check that we're not trying to do an out-of-tree build from
> > +# a tree that's been used for an in-tree build.
> > +ifneq ($(realpath $(SRC_PATH)),$(realpath .))
> > +ifneq ($(UNEXPECTED_FILES),)
> > +$(error Stale files in source tree:${nl}${nl} $(UNEXPECTED_FILES:%=  
> > %${nl}) ${nl}\
> > +This is an out of tree build but your source tree ($(SRC_PATH)) \
> > +seems to have been used for an in-tree build. You can fix this by running \
> > +"$(MAKE) distclean && rm -rf *-linux-user *-softmmu" in your source tree)
> > +endif
> > +endif
> > +
> >  # Add a dependency on the generated files, so that they are always
> >  # rebuilt before other object files
> >  ifneq ($(wildcard config-host.mak),)
> > --
> 
> Does this still mean the check is made sufficiently early, before
> we start trying to build config-all-devices.mak and other things?
> This change has moved it down after 'include' directives which
> I think will cause make to try to build the file that it must
> include.

I'm not 100% sure, but what I did to test was as follows, starting
from git srcdir

   ./configure --target-list=x86_64-softmmu
   make -j 8
   mkdir build
   cd build
   ../configure --target-list=x86_64-softmmu
   find -type f > /tmp/before.txt
   make -j 8
   find -type f > /tmp/after.txt

and confirmed that before.txt & after.txt have same list of files
after make complains about dirty srcdir. So it does not appear to
be triggering any rebuild rules.

I also did another tests where instad of running 'make' in the src
dir, I just arbitrarily touchd the single qemu-version.h file

I think that's important here is that this error checking logic is
not attached to any make target - it is something that is run
immediately as make parses the makefile, before executing any
rules. So as long as we don't have other code that is target
independant, we should be ok.

> It's also moved out of the "only run this if we've actually run
> configure" block, but I think that part is OK.

Yeah I think that is safe for this usage scenario.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|