| Message ID | 1313018232.2989.114.camel@i7.infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, Aug 10, 2011 at 7:16 PM, David Woodhouse <dwmw2@infradead.org> wrote: > On Wed, 2011-08-10 at 18:33 -0400, Arnaud Lacombe wrote: >> > We're not enabling anything that we're later going to break. I can't see >> > many people *depending* on the fact that 'make CONFIG_SATA_MV=y >> > oldconfig' actually does *nothing* in some cases. >> > >> you are wrong, you ends up with half-baked compile-time dependency, >> which break the build: > > s/*nothing*/nothing useful/, for crying out loud. > > The point remains that we're not enabling anything which we're later > going to break. Nobody is going to come to *depend* on this behaviour. > > And anyway, this behaviour exists even *before* my patches, as Michal > pointed out in a far more helpful and constructive fashion in > <CACqU3MU4wJb3ij6skod-ZiM+Q0OMTXNdbJ+qWjJW8VZNEP+x1g@mail.gmail.com> > earlier today. > > It's simple enough to fix, too: > > diff --git a/Makefile b/Makefile > index 1fc5172..6cc7f7b 100644 > --- a/Makefile > +++ b/Makefile > @@ -474,6 +474,9 @@ ifeq ($(KBUILD_EXTMOD),) > endif > endif > > +# This could probably be simpler? > +CONFIG_OVERRIDES := $(patsubst line:%,%,$(filter line:%,$(foreach var, $(filter CONFIG_%,$(.VARIABLES)), $(origin $(var)):$(var)))) > + > ifeq ($(mixed-targets),1) > # =========================================================================== > # We're called with mixed targets (*config and build targets). > @@ -507,6 +510,10 @@ else > # Build targets only - this includes vmlinux, arch specific targets, clean > # targets and others. In general all targets except *config targets. > > +ifneq ($(CONFIG_OVERRIDES),) > +$(error Cannot perform build targets with CONFIG symbols overridden) > +endif > + > ifeq ($(KBUILD_EXTMOD),) > # Additional helpers built in scripts/ > # Carefully list dependencies so we do not try to build scripts twice > how is that supposed to work with your other patches ? % vi mail.txt [strip all lines until 'diff --git a/Makefile b/Makefile'] % git apply mail.txt % make CONFIG_NET=y allyesconfig drivers/ata/ [...] scripts/kconfig/conf --allyesconfig Kconfig # # CONFIG_NET set to y from environment # # # configuration written to .config # Makefile:504: *** Cannot perform build targets with CONFIG symbols overridden. Stop. make: *** [prepare] Error 2 - Arnaud -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2011-08-10 at 23:29 -0400, Arnaud Lacombe wrote: > how is that supposed to work with your other patches ? You cannot perform build targets with CONFIG symbols overridden. > % make CONFIG_NET=y allyesconfig drivers/ata/ ^^^^^^^ override ^^^^^^^^^^ build target > Makefile:504: *** Cannot perform build targets with CONFIG symbols > overridden. Stop. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error message which told you that, if you'd been paying the slightest bit of attention rather than just making pointless noise.
On 11.8.2011 10:42, David Woodhouse wrote: > On Wed, 2011-08-10 at 23:29 -0400, Arnaud Lacombe wrote: >> how is that supposed to work with your other patches ? > > You cannot perform build targets with CONFIG symbols overridden. > >> % make CONFIG_NET=y allyesconfig drivers/ata/ > ^^^^^^^ override ^^^^^^^^^^ build target > >> Makefile:504: *** Cannot perform build targets with CONFIG symbols >> overridden. Stop. > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error message which > told you that, if you'd been paying the slightest bit of attention > rather than just making pointless noise. I would much rather see include/config/auto.conf reset all configuration variables to match what kconfig computed. I.e. use 'undefine CONFIG_FOO' instead of '# CONFIG_FOO is not set'. Additionally print this for any CONFIG_* variable found in the environment, even if the symbol is not visible in the current configuration. The include/config/auto.conf file is only used internally, so it should be safe to change the format. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-08-11 at 10:58 +0200, Michal Marek wrote: > I would much rather see include/config/auto.conf reset all configuration > variables to match what kconfig computed. I.e. use 'undefine CONFIG_FOO' > instead of '# CONFIG_FOO is not set'. That would be cute, but I'm not sure how to undefine something set on the command line: $ cat > Makefile <<EOF undefine BAR foo: echo $(BAR) EOF $ make foo BAR=hh echo hh hh Arguably, if someone *does* try something like Arnaud's "make CONFIG_FOO_BAR=y oldconfig bzImage" .. and it *wasn't* able to set CONFIG_FOO_BAR, then the nicest behaviour would be to fail, rather than to attempt to build it. So perhaps we should clean up only those settings inherited from the environment, and still (as in the patch I sent earlier) refuse to allow build targets in conjunction with CONFIG overrides on the command line?
David Woodhouse <dwmw2@infradead.org> writes: > That would be cute, but I'm not sure how to undefine something set on > the command line: > > $ cat > Makefile <<EOF > undefine BAR override undefine BAR Andreas.
On Thu, 2011-08-11 at 13:15 +0200, Andreas Schwab wrote: > > That would be cute, but I'm not sure how to undefine something set on > > the command line: > > > > $ cat > Makefile <<EOF > > undefine BAR > > override undefine BAR Thanks. So we don't really need to change the auto.conf syntax; we could just do: $(foreach var, $(filter CONFIG_%,$(.VARIABLES)), $(eval override undefine $(var))) But still I suspect we might *not* want that for options set on the command line. We *don't* want 'make CONFIG_FOO=y oldconfig bzImage' to say 'you can't enable CONFIG_FOO' and then build the bzImage anyway. I'll look at making it work only if what's on the command line matches the output of kconfig, but I'm also relatively content with saying "no config overrides on the command line for build targets"
On 11.8.2011 13:40, David Woodhouse wrote: > On Thu, 2011-08-11 at 13:15 +0200, Andreas Schwab wrote: >>> That would be cute, but I'm not sure how to undefine something set on >>> the command line: >>> >>> $ cat > Makefile <<EOF >>> undefine BAR >> >> override undefine BAR > > Thanks. So we don't really need to change the auto.conf syntax; we could > just do: > > $(foreach var, $(filter CONFIG_%,$(.VARIABLES)), $(eval override undefine $(var))) > > But still I suspect we might *not* want that for options set on the > command line. We *don't* want 'make CONFIG_FOO=y oldconfig bzImage' to > say 'you can't enable CONFIG_FOO' and then build the bzImage anyway. If you do $ echo 'CONFIG_FOO=y' >all.config $ make allnoconfig bzImage then it will also build bzImage even if kconfig wasn't able to set CONFIG_FOO=y. IMO printing a warning that CONFIG_FOO could not be set is sufficient, as long as CONFIG_FOO is consistently unset. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-08-11 at 13:56 +0200, Michal Marek wrote: > > If you do > $ echo 'CONFIG_FOO=y' >all.config > $ make allnoconfig bzImage > then it will also build bzImage even if kconfig wasn't able to set > CONFIG_FOO=y. IMO printing a warning that CONFIG_FOO could not be set is > sufficient, as long as CONFIG_FOO is consistently unset. The thing is, such a warning is likely to be lost in the scrollback. There's definitely something to be said for: [dwmw2@i7 linux-2.6]$ make CONFIG_SATA_MV=y bzImage Makefile:572: *** CONFIG_SATA_MV could not be set to "y" as requested. Stop.
Hi, On Thu, Aug 11, 2011 at 7:56 AM, Michal Marek <mmarek@suse.cz> wrote: > On 11.8.2011 13:40, David Woodhouse wrote: >> On Thu, 2011-08-11 at 13:15 +0200, Andreas Schwab wrote: >>>> That would be cute, but I'm not sure how to undefine something set on >>>> the command line: >>>> >>>> $ cat > Makefile <<EOF >>>> undefine BAR >>> >>> override undefine BAR >> >> Thanks. So we don't really need to change the auto.conf syntax; we could >> just do: >> >> $(foreach var, $(filter CONFIG_%,$(.VARIABLES)), $(eval override undefine $(var))) >> >> But still I suspect we might *not* want that for options set on the >> command line. We *don't* want 'make CONFIG_FOO=y oldconfig bzImage' to >> say 'you can't enable CONFIG_FOO' and then build the bzImage anyway. > > If you do > $ echo 'CONFIG_FOO=y' >all.config > $ make allnoconfig bzImage > then it will also build bzImage even if kconfig wasn't able to set > CONFIG_FOO=y. IMO printing a warning that CONFIG_FOO could not be set is > sufficient, as long as CONFIG_FOO is consistently unset. > FWIW, this is the broken behavior I have been pointing all along... If kconfig hard failed on such case, we would not need such Kbuild black-magic. From my point of view, if environment override there should be, it should behave the same as the all.config logic and hard fail when an override has not been met. Code wise, this would translate as backend code path being the same. - Arnaud -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-08-11 at 10:57 -0400, Arnaud Lacombe wrote: > > FWIW, this is the broken behavior I have been pointing all along... > > If kconfig hard failed on such case, we would not need such Kbuild > black-magic. > > From my point of view, if environment override there should be, it > should behave the same as the all.config logic and hard fail when an > override has not been met. > Code wise, this would translate as backend code path being the same. The patches I have so far *do* behave the same as the all.config logic, because the back end code *is* fairly much the same. I was looking at the all.config logic when I wrote the patch to kconfig. It *doesn't* currently hard fail. But I'm more than happy to make it do so. I think you're right; that makes most sense. I've just been looking at ways to allow real build targets to proceed *only* if any config options specified on the command line *did* get honoured by kconfig. But that gets a bit messy since you also want to automatically trigger an 'oldconfig' run if anything was specified on the command line. So you end up with one automatic oldconfig run in a sub-make, then the *second* time around it when the supposedly identical submake is invoked for the real build target, it would have to behave differently. I'm much happier with automatically triggering a reconfig if options are given on the command line, and a hard fail if they can't be honoured. That means that 'make CONFIG_FOO=y bzImage' will work 'properly', which IMO is either to do as it was asked, or fail.
On 11.8.2011 17:07, David Woodhouse wrote: > On Thu, 2011-08-11 at 10:57 -0400, Arnaud Lacombe wrote: >> >> FWIW, this is the broken behavior I have been pointing all along... >> >> If kconfig hard failed on such case, we would not need such Kbuild >> black-magic. >> >> From my point of view, if environment override there should be, it >> should behave the same as the all.config logic and hard fail when an >> override has not been met. >> Code wise, this would translate as backend code path being the same. > > > The patches I have so far *do* behave the same as the all.config logic, > because the back end code *is* fairly much the same. I was looking at > the all.config logic when I wrote the patch to kconfig. > > It *doesn't* currently hard fail. But I'm more than happy to make it do > so. I think you're right; that makes most sense. One of my use cases for all.config is $ cp .../older-working-config all.config $ make KCONFIG_ALLCONFIG=1 allmodconfig i.e. reuse what is possible from an older config and enable all new options. Surely it sometimes results in suboptimal configuration with unwanted debug options enabled, but most of the time it works. I won't be happy if you make it hard fail because of renamed or removed config options. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-08-11 at 17:24 +0200, Michal Marek wrote: > > One of my use cases for all.config is > > $ cp .../older-working-config all.config > $ make KCONFIG_ALLCONFIG=1 allmodconfig > > i.e. reuse what is possible from an older config and enable all new > options. Surely it sometimes results in suboptimal configuration with > unwanted debug options enabled, but most of the time it works. I won't > be happy if you make it hard fail because of renamed or removed config > options. On the other hand, 'make CONFIG_FOO=y oldconfig' probably *should* fail. Perhaps it should set what it *can*, if you try to set multiple options at once — but if it can't set all of them, it should exit with failure status... which will mean that no subsequent build target will happen.
diff --git a/Makefile b/Makefile index 1fc5172..6cc7f7b 100644 --- a/Makefile +++ b/Makefile @@ -474,6 +474,9 @@ ifeq ($(KBUILD_EXTMOD),) endif endif +# This could probably be simpler? +CONFIG_OVERRIDES := $(patsubst line:%,%,$(filter line:%,$(foreach var, $(filter CONFIG_%,$(.VARIABLES)), $(origin $(var)):$(var)))) + ifeq ($(mixed-targets),1) # =========================================================================== # We're called with mixed targets (*config and build targets). @@ -507,6 +510,10 @@ else # Build targets only - this includes vmlinux, arch specific targets, clean # targets and others. In general all targets except *config targets. +ifneq ($(CONFIG_OVERRIDES),) +$(error Cannot perform build targets with CONFIG symbols overridden) +endif + ifeq ($(KBUILD_EXTMOD),) # Additional helpers built in scripts/ # Carefully list dependencies so we do not try to build scripts twice