[Qemu-devel] Re: [PATCH] Makefile: Update unmodified config-devices.mak automatically

[Michael S. Tsirkin]

On Thu, Dec 24, 2009 at 02:31:58PM +0100, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
> > On Sun, Dec 20, 2009 at 03:39:03PM +0100, Stefan Weil wrote:
> >   
> >> This makes rebuilds after source updates easier
> >> for most users (who don't edit config-devices.mak).
> >>
> >> Signed-off-by: Stefan Weil <address@hidden>
> >>     
> >
> > Sorry about missing this and not commenting earlier.
> >
> > So the problem here is that it relies on keeping
> > .old file around. This is a generated file, but
> > - you don't remove it with make clean or distclean
> >   
> 
> As long as config-devices.mak is not removed,
> config-devices.mak.old has to be kept, too.

Yes but you keep it around even after config-devices.mak
is removed.

> > - it is not removed on error properly as it is not
> >   a target of makefile
> > so it seems easy to get into a situation where
> > a corrupted file will be created and the only way
> > to get rid of it would be by manual rm command.
> >   
> 
> I don't think that this is a real world scenario.
> The .old file is a simple copy of config-devices.mak.

By the way, what if you really *do* want configuration that happens to
exactly match the default at some point?  There is no way to take that
config and make it persistent, is there?  With my approach, any edit of
the file adding something will do.

> > Instead, what I think we should do is make
> > the generated file *almost empty*
> > and then it is easy to detect user tweaking
> > it without keeping more state.
> >   
> 
> This is a matter of personal preferences.
> Maybe other users who want to modify
> config-devices.mak prefer to have a full
> version where they can remove some lines
> they don't need.

IMO this encourages sloppiness, it is better to have a file which does
1. include default
2. change some values
than have a full copy of said default and then try to figure out what
did you change.  If you want to look at the original, it is there.

> The solution implemented here was the result
> of Juan's feedback to an earlier suggestion
> from me.
> 
> > A patch I posted does this by sticking
> > a single "include" directive in a generated file.
> >
> >
> >   
> >> ---
> >>  Makefile |   20 +++++++++++++++-----
> >>  1 files changed, 15 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/Makefile b/Makefile
> >> index c1fa08c..684365d 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -43,12 +43,22 @@ config-all-devices.mak: $(SUBDIR_DEVICES_MAK)
> >>  
> >>  %/config-devices.mak: default-configs/%.mak
> >>    $(call quiet-command,cat $< > address@hidden, "  GEN   $@")
> >> -  @if test -f $@ ; then \
> >> -    echo "WARNING: $@ out of date." ;\
> >> -    echo "Run \"make defconfig\" to regenerate." ; \
> >> -    rm address@hidden ; \
> >> +  @if test -f $@; then \
> >> +    if cmp -s address@hidden $@ || cmp -s $@ address@hidden; then \
> >> +      mv address@hidden $@; \
> >> +      cp -p $@ address@hidden; \
> >> +    else \
> >> +      if test -f address@hidden; then \
> >> +        echo "WARNING: $@ (user modified) out of date.";\
> >> +      else \
> >> +        echo "WARNING: $@ out of date.";\
> >> +      fi; \
> >> +      echo "Run \"make defconfig\" to regenerate."; \
> >> +      rm address@hidden; \
> >> +    fi; \
> >>     else \
> >> -    mv address@hidden $@ ; \
> >> +    mv address@hidden $@; \
> >> +    cp -p $@ address@hidden; \
> >>     fi
> >>  
> >>  defconfig:
> >> -- 
> >> 1.6.5