| Message ID | 1261319943-9224-1-git-send-email-weil@mail.berlios.de |
|---|---|
| State | New |
| Headers | show |
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 > >
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
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
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.
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
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: >
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:
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(-)