Patchwork Makefile: Update unmodified config-devices.mak automatically

login
register
mail settings
Submitter Stefan Weil
Date Dec. 20, 2009, 2:39 p.m.
Message ID <1261319943-9224-1-git-send-email-weil@mail.berlios.de>
Download mbox | patch
Permalink /patch/41507/
State New
Headers show

Comments

Stefan Weil - Dec. 20, 2009, 2:39 p.m.
This makes rebuilds after source updates easier
for most users (who don't edit config-devices.mak).

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 Makefile |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)
Michael S. Tsirkin - Dec. 24, 2009, 12:58 p.m.
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 <weil@mail.berlios.de>

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
- 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.

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.

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 $< > $@.tmp, "  GEN   $@")
> -	@if test -f $@ ; then \
> -	  echo "WARNING: $@ out of date." ;\
> -	  echo "Run \"make defconfig\" to regenerate." ; \
> -	  rm $@.tmp ; \
> +	@if test -f $@; then \
> +	  if cmp -s $@.old $@ || cmp -s $@ $@.tmp; then \
> +	    mv $@.tmp $@; \
> +	    cp -p $@ $@.old; \
> +	  else \
> +	    if test -f $@.old; then \
> +	      echo "WARNING: $@ (user modified) out of date.";\
> +	    else \
> +	      echo "WARNING: $@ out of date.";\
> +	    fi; \
> +	    echo "Run \"make defconfig\" to regenerate."; \
> +	    rm $@.tmp; \
> +	  fi; \
>  	 else \
> -	  mv $@.tmp $@ ; \
> +	  mv $@.tmp $@; \
> +	  cp -p $@ $@.old; \
>  	 fi
>  
>  defconfig:
> -- 
> 1.6.5
> 
>
Stefan Weil - Dec. 24, 2009, 1:31 p.m.
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 <weil@mail.berlios.de>
>>     
>
> 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.

> - 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.

> 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.

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 $< > $@.tmp, "  GEN   $@")
>> -	@if test -f $@ ; then \
>> -	  echo "WARNING: $@ out of date." ;\
>> -	  echo "Run \"make defconfig\" to regenerate." ; \
>> -	  rm $@.tmp ; \
>> +	@if test -f $@; then \
>> +	  if cmp -s $@.old $@ || cmp -s $@ $@.tmp; then \
>> +	    mv $@.tmp $@; \
>> +	    cp -p $@ $@.old; \
>> +	  else \
>> +	    if test -f $@.old; then \
>> +	      echo "WARNING: $@ (user modified) out of date.";\
>> +	    else \
>> +	      echo "WARNING: $@ out of date.";\
>> +	    fi; \
>> +	    echo "Run \"make defconfig\" to regenerate."; \
>> +	    rm $@.tmp; \
>> +	  fi; \
>>  	 else \
>> -	  mv $@.tmp $@ ; \
>> +	  mv $@.tmp $@; \
>> +	  cp -p $@ $@.old; \
>>  	 fi
>>  
>>  defconfig:
>> -- 
>> 1.6.5
Michael S. Tsirkin - Dec. 24, 2009, 1:40 p.m.
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 <weil@mail.berlios.de>
> >>     
> >
> > 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 $< > $@.tmp, "  GEN   $@")
> >> -	@if test -f $@ ; then \
> >> -	  echo "WARNING: $@ out of date." ;\
> >> -	  echo "Run \"make defconfig\" to regenerate." ; \
> >> -	  rm $@.tmp ; \
> >> +	@if test -f $@; then \
> >> +	  if cmp -s $@.old $@ || cmp -s $@ $@.tmp; then \
> >> +	    mv $@.tmp $@; \
> >> +	    cp -p $@ $@.old; \
> >> +	  else \
> >> +	    if test -f $@.old; then \
> >> +	      echo "WARNING: $@ (user modified) out of date.";\
> >> +	    else \
> >> +	      echo "WARNING: $@ out of date.";\
> >> +	    fi; \
> >> +	    echo "Run \"make defconfig\" to regenerate."; \
> >> +	    rm $@.tmp; \
> >> +	  fi; \
> >>  	 else \
> >> -	  mv $@.tmp $@ ; \
> >> +	  mv $@.tmp $@; \
> >> +	  cp -p $@ $@.old; \
> >>  	 fi
> >>  
> >>  defconfig:
> >> -- 
> >> 1.6.5
Michael S. Tsirkin - Dec. 24, 2009, 3:06 p.m.
On Thu, Dec 24, 2009 at 04:03:17PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 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 <weil@mail.berlios.de>
> >> >>     
> >> >
> >> > 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.
> 
> You can't make your cake and have it.
> Your include file fails exactly in this very scenary:
> 
> #include default-config
> 
> local modifications
> 
> You change default-config, and you also have a different configuration.
> 
> I don't see any advantage at using includes instead of copy the file,
> and implement include directive just make things more complex IMHO.
> 
> >> > 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.
> 
> This build system is lossely bassed on kernel config files.  And in the
> kernel, once that you have your .config, it will never be overwritten.
> 
> Both options have its advantages and disadvantages.
> 
> I am looking at this patch, and was thinking about reordering the
> comparisons, but I am still not sure that my approach is better than
> this one.  Will think a bit more about it and include this one or
> propose something similar in spirit.
> 
> Later, Juan.

IMO the really best approach is not to generate any files
at all. This way we avoid confusion: if there is config
file around, user created this file.
But I don't care much, really.

Whatever you do, please make sure that
make defconfig itself does not print "run make defconfig",
this is just silly.
Stefan Weil - Jan. 5, 2010, 9:25 p.m.
Michael S. Tsirkin schrieb:
> On Thu, Dec 24, 2009 at 04:03:17PM +0100, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>> 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 <weil@mail.berlios.de>
>>>>>>
>>>>> 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.
>> You can't make your cake and have it.
>> Your include file fails exactly in this very scenary:
>>
>> #include default-config
>>
>> local modifications
>>
>> You change default-config, and you also have a different configuration.
>>
>> I don't see any advantage at using includes instead of copy the file,
>> and implement include directive just make things more complex IMHO.
>>
>>>>> 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.
>> This build system is lossely bassed on kernel config files. And in the
>> kernel, once that you have your .config, it will never be overwritten.
>>
>> Both options have its advantages and disadvantages.
>>
>> I am looking at this patch, and was thinking about reordering the
>> comparisons, but I am still not sure that my approach is better than
>> this one. Will think a bit more about it and include this one or
>> propose something similar in spirit.
>>
>> Later, Juan.
>
> IMO the really best approach is not to generate any files
> at all. This way we avoid confusion: if there is config
> file around, user created this file.
> But I don't care much, really.
>
> Whatever you do, please make sure that
> make defconfig itself does not print "run make defconfig",
> this is just silly.

Agreed, but this can be handled in a separate patch.
Only those who modify config-devices.mak manually
get this irritating message, so fixing it has lower
priority.

Antony, please send feedback if there is still something
why my patch cannot be applied as it is. It is much better
than the status quo, so I think it should be applied soon.

For a really satisfying solution, I'd prefer a build system
configured with "make menuconfig".

Kind regards,
Stefan Weil
Anthony Liguori - Jan. 8, 2010, 4:34 p.m.
On 12/20/2009 08:39 AM, 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<weil@mail.berlios.de>\
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   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 $<  >  $@.tmp, "  GEN   $@")
> -	@if test -f $@ ; then \
> -	  echo "WARNING: $@ out of date." ;\
> -	  echo "Run \"make defconfig\" to regenerate." ; \
> -	  rm $@.tmp ; \
> +	@if test -f $@; then \
> +	  if cmp -s $@.old $@ || cmp -s $@ $@.tmp; then \
> +	    mv $@.tmp $@; \
> +	    cp -p $@ $@.old; \
> +	  else \
> +	    if test -f $@.old; then \
> +	      echo "WARNING: $@ (user modified) out of date.";\
> +	    else \
> +	      echo "WARNING: $@ out of date.";\
> +	    fi; \
> +	    echo "Run \"make defconfig\" to regenerate."; \
> +	    rm $@.tmp; \
> +	  fi; \
>   	 else \
> -	  mv $@.tmp $@ ; \
> +	  mv $@.tmp $@; \
> +	  cp -p $@ $@.old; \
>   	 fi
>
>   defconfig:
>

Patch

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 $< > $@.tmp, "  GEN   $@")
-	@if test -f $@ ; then \
-	  echo "WARNING: $@ out of date." ;\
-	  echo "Run \"make defconfig\" to regenerate." ; \
-	  rm $@.tmp ; \
+	@if test -f $@; then \
+	  if cmp -s $@.old $@ || cmp -s $@ $@.tmp; then \
+	    mv $@.tmp $@; \
+	    cp -p $@ $@.old; \
+	  else \
+	    if test -f $@.old; then \
+	      echo "WARNING: $@ (user modified) out of date.";\
+	    else \
+	      echo "WARNING: $@ out of date.";\
+	    fi; \
+	    echo "Run \"make defconfig\" to regenerate."; \
+	    rm $@.tmp; \
+	  fi; \
 	 else \
-	  mv $@.tmp $@ ; \
+	  mv $@.tmp $@; \
+	  cp -p $@ $@.old; \
 	 fi
 
 defconfig: