| Message ID | 20240215093002.23527-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | build/xen: fail to rebuild if Kconfig fails | expand |
On 15.02.2024 10:30, Roger Pau Monne wrote: > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -358,10 +358,10 @@ config: tools_fixdep outputmakefile FORCE > else # !config-build > > ifeq ($(need-config),y) > --include include/config/auto.conf > # Read in dependencies to all Kconfig* files, make sure to run syncconfig if > # changes are detected. > -include include/config/auto.conf.cmd > +include include/config/auto.conf With the - dropped, ... > @@ -375,6 +375,7 @@ $(KCONFIG_CONFIG): tools_fixdep > # This exploits the 'multi-target pattern rule' trick. > # The syncconfig should be executed only once to make all the targets. > include/config/%.conf include/config/%.conf.cmd: $(KCONFIG_CONFIG) > + rm -rf include/config/$*.conf > $(Q)$(MAKE) $(build)=tools/kconfig syncconfig ... is this really necessary? The error status from the sub-make is ignored only because of the -, isn't it? I also don't really follow the need to re-order the include-s above. Their ordering ought to be benign, as per make's doc stating "If an included makefile cannot be found in any of these directories it is not an immediately fatal error; processing of the makefile containing the include continues." While the description talks about this, I'm afraid I don't really understand "... the .cmd target is executed before including ...": What .cmd target are you talking about there? That's merely another include (and what is included doesn't provide any .cmd target afaics), so will be dealt with the same as the config/auto.conf one itself. Jan
On Thu, Feb 15, 2024 at 10:49:31AM +0100, Jan Beulich wrote: > On 15.02.2024 10:30, Roger Pau Monne wrote: > > --- a/xen/Makefile > > +++ b/xen/Makefile > > @@ -358,10 +358,10 @@ config: tools_fixdep outputmakefile FORCE > > else # !config-build > > > > ifeq ($(need-config),y) > > --include include/config/auto.conf > > # Read in dependencies to all Kconfig* files, make sure to run syncconfig if > > # changes are detected. > > -include include/config/auto.conf.cmd > > +include include/config/auto.conf > > With the - dropped, ... > > > @@ -375,6 +375,7 @@ $(KCONFIG_CONFIG): tools_fixdep > > # This exploits the 'multi-target pattern rule' trick. > > # The syncconfig should be executed only once to make all the targets. > > include/config/%.conf include/config/%.conf.cmd: $(KCONFIG_CONFIG) > > + rm -rf include/config/$*.conf > > $(Q)$(MAKE) $(build)=tools/kconfig syncconfig > > ... is this really necessary? The error status from the sub-make is ignored > only because of the -, isn't it? Without the `rm` the include/config/auto.conf is not removed by Kconfig on error, so the include will still succeed but use the stale auto.conf file. Keep in mind on rebuilds include/config/auto.conf is already present, so the rule is only executed for the include/config/auto.conf.cmd target. > > I also don't really follow the need to re-order the include-s above. Their > ordering ought to be benign, as per make's doc stating "If an included > makefile cannot be found in any of these directories it is not an > immediately fatal error; processing of the makefile containing the include > continues." While the description talks about this, I'm afraid I don't > really understand "... the .cmd target is executed before including ...": > What .cmd target are you talking about there? Without the reordering the include of include/config/auto.conf will always succeed on rebuilds, because the include is done before executing the include/config/%.conf.cmd target that does the `rm`. With the current order the include of include/config/%.conf.cmd that triggers the re-build of auto.conf happens after having included the file already. Thanks, Roger.
On Thu, Feb 15, 2024 at 10:30:02AM +0100, Roger Pau Monne wrote: > When doing a rebuild with an xen/include/config/auto.conf already present in > the tree, failures from Kconfig are ignored since the target is present: > > gmake -C xen install > gmake[1]: Entering directory '/root/src/xen/xen' > tools/kconfig/conf --syncconfig Kconfig > common/Kconfig:2: syntax error > common/Kconfig:1: invalid statement > gmake[2]: *** [tools/kconfig/Makefile:73: syncconfig] Error 1 > UPD include/xen/compile.h > Xen 4.19-unstable > gmake[3]: Nothing to be done for 'all'. > gmake[2]: 'arch/x86/include/asm/asm-offsets.h' is up to date. > > Ultimately leading to a successful build despite the Kconfig error. > > Fix this by first removing xen/include/config/auto.conf before attempting to > regenerate, and then also make xen/include/config/auto.conf a hard dependency > of the build process (ie: drop the leading '-') and reordering so the .cmd > target is executed before including the configuration file. Could you try to revert commit 8d4c17a90b0a ("xen/build: silence make warnings about missing auto.conf*") instead? With a much shorter message like "Don't ignore Kconfig error anymore". Cheers,
On 15.02.2024 11:32, Anthony PERARD wrote: > On Thu, Feb 15, 2024 at 10:30:02AM +0100, Roger Pau Monne wrote: >> When doing a rebuild with an xen/include/config/auto.conf already present in >> the tree, failures from Kconfig are ignored since the target is present: >> >> gmake -C xen install >> gmake[1]: Entering directory '/root/src/xen/xen' >> tools/kconfig/conf --syncconfig Kconfig >> common/Kconfig:2: syntax error >> common/Kconfig:1: invalid statement >> gmake[2]: *** [tools/kconfig/Makefile:73: syncconfig] Error 1 >> UPD include/xen/compile.h >> Xen 4.19-unstable >> gmake[3]: Nothing to be done for 'all'. >> gmake[2]: 'arch/x86/include/asm/asm-offsets.h' is up to date. >> >> Ultimately leading to a successful build despite the Kconfig error. >> >> Fix this by first removing xen/include/config/auto.conf before attempting to >> regenerate, and then also make xen/include/config/auto.conf a hard dependency >> of the build process (ie: drop the leading '-') and reordering so the .cmd >> target is executed before including the configuration file. > > Could you try to revert commit 8d4c17a90b0a ("xen/build: silence make > warnings about missing auto.conf*") instead? With a much shorter message > like "Don't ignore Kconfig error anymore". But that'll introduce the problem that was addressed there, won't it? Jan
On 15.02.2024 11:28, Roger Pau Monné wrote: > On Thu, Feb 15, 2024 at 10:49:31AM +0100, Jan Beulich wrote: >> On 15.02.2024 10:30, Roger Pau Monne wrote: >>> --- a/xen/Makefile >>> +++ b/xen/Makefile >>> @@ -358,10 +358,10 @@ config: tools_fixdep outputmakefile FORCE >>> else # !config-build >>> >>> ifeq ($(need-config),y) >>> --include include/config/auto.conf >>> # Read in dependencies to all Kconfig* files, make sure to run syncconfig if >>> # changes are detected. >>> -include include/config/auto.conf.cmd >>> +include include/config/auto.conf >> >> With the - dropped, ... >> >>> @@ -375,6 +375,7 @@ $(KCONFIG_CONFIG): tools_fixdep >>> # This exploits the 'multi-target pattern rule' trick. >>> # The syncconfig should be executed only once to make all the targets. >>> include/config/%.conf include/config/%.conf.cmd: $(KCONFIG_CONFIG) >>> + rm -rf include/config/$*.conf >>> $(Q)$(MAKE) $(build)=tools/kconfig syncconfig >> >> ... is this really necessary? The error status from the sub-make is ignored >> only because of the -, isn't it? > > Without the `rm` the include/config/auto.conf is not removed by > Kconfig on error, so the include will still succeed but use the stale > auto.conf file. > > Keep in mind on rebuilds include/config/auto.conf is already present, > so the rule is only executed for the include/config/auto.conf.cmd > target. But the sub-make ought to return failure, which ought to then stop the build process? >> I also don't really follow the need to re-order the include-s above. Their >> ordering ought to be benign, as per make's doc stating "If an included >> makefile cannot be found in any of these directories it is not an >> immediately fatal error; processing of the makefile containing the include >> continues." While the description talks about this, I'm afraid I don't >> really understand "... the .cmd target is executed before including ...": >> What .cmd target are you talking about there? > > Without the reordering the include of include/config/auto.conf will > always succeed on rebuilds, because the include is done before > executing the include/config/%.conf.cmd target that does the `rm`. That's a dual target: It also handles include/config/%.conf. I.e. because of this ... > With the current order the include of include/config/%.conf.cmd that > triggers the re-build of auto.conf happens after having included the > file already. ... either include would trigger this same rule. IOW I'm afraid I'm still not seeing what is gained by the re-ordering. I'm also unconvinced that "triggers" in the sense you use it is actually applicable. Quoting make doc again: "Once it has finished reading makefiles, make will try to remake any that are out of date or don’t exist." To me this means that first all makefile reading will finish, and then whichever included files need re-making will be re-made. Jan
On Thu, Feb 15, 2024 at 11:43:02AM +0100, Jan Beulich wrote: > On 15.02.2024 11:28, Roger Pau Monné wrote: > > On Thu, Feb 15, 2024 at 10:49:31AM +0100, Jan Beulich wrote: > >> On 15.02.2024 10:30, Roger Pau Monne wrote: > >>> --- a/xen/Makefile > >>> +++ b/xen/Makefile > >>> @@ -358,10 +358,10 @@ config: tools_fixdep outputmakefile FORCE > >>> else # !config-build > >>> > >>> ifeq ($(need-config),y) > >>> --include include/config/auto.conf > >>> # Read in dependencies to all Kconfig* files, make sure to run syncconfig if > >>> # changes are detected. > >>> -include include/config/auto.conf.cmd > >>> +include include/config/auto.conf > >> > >> With the - dropped, ... > >> > >>> @@ -375,6 +375,7 @@ $(KCONFIG_CONFIG): tools_fixdep > >>> # This exploits the 'multi-target pattern rule' trick. > >>> # The syncconfig should be executed only once to make all the targets. > >>> include/config/%.conf include/config/%.conf.cmd: $(KCONFIG_CONFIG) > >>> + rm -rf include/config/$*.conf > >>> $(Q)$(MAKE) $(build)=tools/kconfig syncconfig > >> > >> ... is this really necessary? The error status from the sub-make is ignored > >> only because of the -, isn't it? > > > > Without the `rm` the include/config/auto.conf is not removed by > > Kconfig on error, so the include will still succeed but use the stale > > auto.conf file. > > > > Keep in mind on rebuilds include/config/auto.conf is already present, > > so the rule is only executed for the include/config/auto.conf.cmd > > target. > > But the sub-make ought to return failure, which ought to then stop the > build process? For some reason it doesn't, not at least with GNU Make 4.3. It stops the build if the '-' is dropped from the include of include/config/auto.conf.cmd. But that will always fail as include/config/auto.conf.cmd is never created. Maybe there's something weird with our makefile, I certainly don't know that much, but as noted in the commit message, include/config/auto.conf.cmd failing doesn't cause the build to stop. > >> I also don't really follow the need to re-order the include-s above. Their > >> ordering ought to be benign, as per make's doc stating "If an included > >> makefile cannot be found in any of these directories it is not an > >> immediately fatal error; processing of the makefile containing the include > >> continues." While the description talks about this, I'm afraid I don't > >> really understand "... the .cmd target is executed before including ...": > >> What .cmd target are you talking about there? > > > > Without the reordering the include of include/config/auto.conf will > > always succeed on rebuilds, because the include is done before > > executing the include/config/%.conf.cmd target that does the `rm`. > > That's a dual target: It also handles include/config/%.conf. I.e. > because of this ... > > > With the current order the include of include/config/%.conf.cmd that > > triggers the re-build of auto.conf happens after having included the > > file already. > > ... either include would trigger this same rule. IOW I'm afraid I'm still > not seeing what is gained by the re-ordering. I'm also unconvinced that > "triggers" in the sense you use it is actually applicable. Quoting make > doc again: "Once it has finished reading makefiles, make will try to > remake any that are out of date or don’t exist." To me this means that > first all makefile reading will finish, and then whichever included files > need re-making will be re-made. Without the re-ordering the execution is not stopped on failure to generate include/config/auto.conf: # gmake -j8 xen clang=y gmake -C xen install gmake[1]: Entering directory '/root/src/xen/xen' rm -rf include/config/auto.conf tools/kconfig/conf --syncconfig Kconfig common/Kconfig:2: syntax error common/Kconfig:1: invalid statement gmake[2]: *** [tools/kconfig/Makefile:73: syncconfig] Error 1 gmake[1]: Failed to remake makefile 'include/config/auto.conf'. UPD include/xen/compile.h Xen 4.19-unstable gmake[3]: Nothing to be done for 'all'. [...] With the re-ordering: # gmake -j8 xen clang=y gmake -C xen install gmake[1]: Entering directory '/root/src/xen/xen' rm -rf include/config/auto.conf tools/kconfig/conf --syncconfig Kconfig common/Kconfig:2: syntax error common/Kconfig:1: invalid statement gmake[2]: *** [tools/kconfig/Makefile:73: syncconfig] Error 1 gmake[1]: *** [Makefile:379: include/config/auto.conf] Error 2 gmake[1]: Leaving directory '/root/src/xen/xen' gmake: *** [Makefile:143: install-xen] Error 2 # So the re-ordering is meaningful. Thanks, Roger.
On Thu, Feb 15, 2024 at 10:32:57AM +0000, Anthony PERARD wrote: > On Thu, Feb 15, 2024 at 10:30:02AM +0100, Roger Pau Monne wrote: > > When doing a rebuild with an xen/include/config/auto.conf already present in > > the tree, failures from Kconfig are ignored since the target is present: > > > > gmake -C xen install > > gmake[1]: Entering directory '/root/src/xen/xen' > > tools/kconfig/conf --syncconfig Kconfig > > common/Kconfig:2: syntax error > > common/Kconfig:1: invalid statement > > gmake[2]: *** [tools/kconfig/Makefile:73: syncconfig] Error 1 > > UPD include/xen/compile.h > > Xen 4.19-unstable > > gmake[3]: Nothing to be done for 'all'. > > gmake[2]: 'arch/x86/include/asm/asm-offsets.h' is up to date. > > > > Ultimately leading to a successful build despite the Kconfig error. > > > > Fix this by first removing xen/include/config/auto.conf before attempting to > > regenerate, and then also make xen/include/config/auto.conf a hard dependency > > of the build process (ie: drop the leading '-') and reordering so the .cmd > > target is executed before including the configuration file. > > Could you try to revert commit 8d4c17a90b0a ("xen/build: silence make > warnings about missing auto.conf*") instead? With a much shorter message > like "Don't ignore Kconfig error anymore". Yes that seems to solve it also. I guess the point is that for the target failure to stop execution the include that triggered it needs to be non-optional (so not - prefixed). I'm unsure about the consequences of reverting 8d4c17a90b0a, so would prefer if you could take care of that. Thanks, Roger.
On 15.02.2024 12:04, Roger Pau Monné wrote: > On Thu, Feb 15, 2024 at 11:43:02AM +0100, Jan Beulich wrote: >> On 15.02.2024 11:28, Roger Pau Monné wrote: >>> Without the reordering the include of include/config/auto.conf will >>> always succeed on rebuilds, because the include is done before >>> executing the include/config/%.conf.cmd target that does the `rm`. >> >> That's a dual target: It also handles include/config/%.conf. I.e. >> because of this ... >> >>> With the current order the include of include/config/%.conf.cmd that >>> triggers the re-build of auto.conf happens after having included the >>> file already. >> >> ... either include would trigger this same rule. IOW I'm afraid I'm still >> not seeing what is gained by the re-ordering. I'm also unconvinced that >> "triggers" in the sense you use it is actually applicable. Quoting make >> doc again: "Once it has finished reading makefiles, make will try to >> remake any that are out of date or don’t exist." To me this means that >> first all makefile reading will finish, and then whichever included files >> need re-making will be re-made. > > Without the re-ordering the execution is not stopped on failure to > generate include/config/auto.conf: > > # gmake -j8 xen clang=y > gmake -C xen install > gmake[1]: Entering directory '/root/src/xen/xen' > rm -rf include/config/auto.conf > tools/kconfig/conf --syncconfig Kconfig > common/Kconfig:2: syntax error > common/Kconfig:1: invalid statement > gmake[2]: *** [tools/kconfig/Makefile:73: syncconfig] Error 1 > gmake[1]: Failed to remake makefile 'include/config/auto.conf'. > UPD include/xen/compile.h > Xen 4.19-unstable > gmake[3]: Nothing to be done for 'all'. > [...] > > With the re-ordering: > > # gmake -j8 xen clang=y > gmake -C xen install > gmake[1]: Entering directory '/root/src/xen/xen' > rm -rf include/config/auto.conf > tools/kconfig/conf --syncconfig Kconfig > common/Kconfig:2: syntax error > common/Kconfig:1: invalid statement > gmake[2]: *** [tools/kconfig/Makefile:73: syncconfig] Error 1 > gmake[1]: *** [Makefile:379: include/config/auto.conf] Error 2 > gmake[1]: Leaving directory '/root/src/xen/xen' > gmake: *** [Makefile:143: install-xen] Error 2 > # > > So the re-ordering is meaningful. Hmm, I was about to say "no, it isn't" but then remembered to try newer make. With 3.81 all is working as intended with no re-ordering. I wonder if this isn't something with make, rather than our makefiles ... Jan
On 15.02.2024 13:11, Jan Beulich wrote: > On 15.02.2024 12:04, Roger Pau Monné wrote: >> On Thu, Feb 15, 2024 at 11:43:02AM +0100, Jan Beulich wrote: >>> On 15.02.2024 11:28, Roger Pau Monné wrote: >>>> Without the reordering the include of include/config/auto.conf will >>>> always succeed on rebuilds, because the include is done before >>>> executing the include/config/%.conf.cmd target that does the `rm`. >>> >>> That's a dual target: It also handles include/config/%.conf. I.e. >>> because of this ... >>> >>>> With the current order the include of include/config/%.conf.cmd that >>>> triggers the re-build of auto.conf happens after having included the >>>> file already. >>> >>> ... either include would trigger this same rule. IOW I'm afraid I'm still >>> not seeing what is gained by the re-ordering. I'm also unconvinced that >>> "triggers" in the sense you use it is actually applicable. Quoting make >>> doc again: "Once it has finished reading makefiles, make will try to >>> remake any that are out of date or don’t exist." To me this means that >>> first all makefile reading will finish, and then whichever included files >>> need re-making will be re-made. >> >> Without the re-ordering the execution is not stopped on failure to >> generate include/config/auto.conf: >> >> # gmake -j8 xen clang=y >> gmake -C xen install >> gmake[1]: Entering directory '/root/src/xen/xen' >> rm -rf include/config/auto.conf >> tools/kconfig/conf --syncconfig Kconfig >> common/Kconfig:2: syntax error >> common/Kconfig:1: invalid statement >> gmake[2]: *** [tools/kconfig/Makefile:73: syncconfig] Error 1 >> gmake[1]: Failed to remake makefile 'include/config/auto.conf'. >> UPD include/xen/compile.h >> Xen 4.19-unstable >> gmake[3]: Nothing to be done for 'all'. >> [...] >> >> With the re-ordering: >> >> # gmake -j8 xen clang=y >> gmake -C xen install >> gmake[1]: Entering directory '/root/src/xen/xen' >> rm -rf include/config/auto.conf >> tools/kconfig/conf --syncconfig Kconfig >> common/Kconfig:2: syntax error >> common/Kconfig:1: invalid statement >> gmake[2]: *** [tools/kconfig/Makefile:73: syncconfig] Error 1 >> gmake[1]: *** [Makefile:379: include/config/auto.conf] Error 2 >> gmake[1]: Leaving directory '/root/src/xen/xen' >> gmake: *** [Makefile:143: install-xen] Error 2 >> # >> >> So the re-ordering is meaningful. > > Hmm, I was about to say "no, it isn't" but then remembered to try newer > make. With 3.81 all is working as intended with no re-ordering. I wonder > if this isn't something with make, rather than our makefiles ... And indeed it's quite strange: The same effect can be had by dropping the - from the other include (and no re-ordering). Extending the sub-make invocation to $(Q)$(MAKE) $(build)=tools/kconfig syncconfig || { err=$$?; echo "$@: error $$err" >&2; exit $$err; } I can see that it's the latter of the includes which is reported as $@, and failure of the sub-make is conveyed only when it's the latter of the includes which has no -. When really any of the targets being subject of "include" without - should respect failure. I'll play with this some more, but considering the overall situation maybe (perhaps just partly) reverting the commit Anthony pointed out is then the least bad option, accepting the warnings with old make. Jan
On 15.02.2024 12:04, Roger Pau Monné wrote: > On Thu, Feb 15, 2024 at 11:43:02AM +0100, Jan Beulich wrote: >> On 15.02.2024 11:28, Roger Pau Monné wrote: >>> On Thu, Feb 15, 2024 at 10:49:31AM +0100, Jan Beulich wrote: >>>> On 15.02.2024 10:30, Roger Pau Monne wrote: >>>>> --- a/xen/Makefile >>>>> +++ b/xen/Makefile >>>>> @@ -358,10 +358,10 @@ config: tools_fixdep outputmakefile FORCE >>>>> else # !config-build >>>>> >>>>> ifeq ($(need-config),y) >>>>> --include include/config/auto.conf >>>>> # Read in dependencies to all Kconfig* files, make sure to run syncconfig if >>>>> # changes are detected. >>>>> -include include/config/auto.conf.cmd >>>>> +include include/config/auto.conf >>>> >>>> With the - dropped, ... >>>> >>>>> @@ -375,6 +375,7 @@ $(KCONFIG_CONFIG): tools_fixdep >>>>> # This exploits the 'multi-target pattern rule' trick. >>>>> # The syncconfig should be executed only once to make all the targets. >>>>> include/config/%.conf include/config/%.conf.cmd: $(KCONFIG_CONFIG) >>>>> + rm -rf include/config/$*.conf >>>>> $(Q)$(MAKE) $(build)=tools/kconfig syncconfig >>>> >>>> ... is this really necessary? The error status from the sub-make is ignored >>>> only because of the -, isn't it? >>> >>> Without the `rm` the include/config/auto.conf is not removed by >>> Kconfig on error, so the include will still succeed but use the stale >>> auto.conf file. >>> >>> Keep in mind on rebuilds include/config/auto.conf is already present, >>> so the rule is only executed for the include/config/auto.conf.cmd >>> target. >> >> But the sub-make ought to return failure, which ought to then stop the >> build process? > > For some reason it doesn't, not at least with GNU Make 4.3. > > It stops the build if the '-' is dropped from the include of > include/config/auto.conf.cmd. But that will always fail as > include/config/auto.conf.cmd is never created. > > Maybe there's something weird with our makefile, I certainly don't > know that much, but as noted in the commit message, > include/config/auto.conf.cmd failing doesn't cause the build to > stop. How about the below as an alternative? I'm not overly happy with the double ifneq, but I also don't see a good other option. This way the missing auto.conf is detected slightly later, but this may well be good enough. Then again I might be overlooking yet something else that this breaks ... Jan --- a/xen/Makefile +++ b/xen/Makefile @@ -375,6 +375,7 @@ $(KCONFIG_CONFIG): tools_fixdep # This exploits the 'multi-target pattern rule' trick. # The syncconfig should be executed only once to make all the targets. include/config/%.conf include/config/%.conf.cmd: $(KCONFIG_CONFIG) + $(Q)rm -f include/config/$*.conf $(Q)$(MAKE) $(build)=tools/kconfig syncconfig ifeq ($(CONFIG_DEBUG),y) --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -15,7 +15,11 @@ srcdir := $(srctree)/$(src) PHONY := __build __build: --include $(objtree)/include/config/auto.conf +ifneq ($(obj),tools) +ifneq ($(obj),tools/kconfig) +include $(objtree)/include/config/auto.conf +endif +endif include $(XEN_ROOT)/Config.mk include $(srctree)/scripts/Kbuild.include
On Thu, Feb 15, 2024 at 02:02:41PM +0100, Jan Beulich wrote: > On 15.02.2024 12:04, Roger Pau Monné wrote: > > On Thu, Feb 15, 2024 at 11:43:02AM +0100, Jan Beulich wrote: > >> On 15.02.2024 11:28, Roger Pau Monné wrote: > >>> On Thu, Feb 15, 2024 at 10:49:31AM +0100, Jan Beulich wrote: > >>>> On 15.02.2024 10:30, Roger Pau Monne wrote: > >>>>> --- a/xen/Makefile > >>>>> +++ b/xen/Makefile > >>>>> @@ -358,10 +358,10 @@ config: tools_fixdep outputmakefile FORCE > >>>>> else # !config-build > >>>>> > >>>>> ifeq ($(need-config),y) > >>>>> --include include/config/auto.conf > >>>>> # Read in dependencies to all Kconfig* files, make sure to run syncconfig if > >>>>> # changes are detected. > >>>>> -include include/config/auto.conf.cmd > >>>>> +include include/config/auto.conf > >>>> > >>>> With the - dropped, ... > >>>> > >>>>> @@ -375,6 +375,7 @@ $(KCONFIG_CONFIG): tools_fixdep > >>>>> # This exploits the 'multi-target pattern rule' trick. > >>>>> # The syncconfig should be executed only once to make all the targets. > >>>>> include/config/%.conf include/config/%.conf.cmd: $(KCONFIG_CONFIG) > >>>>> + rm -rf include/config/$*.conf > >>>>> $(Q)$(MAKE) $(build)=tools/kconfig syncconfig > >>>> > >>>> ... is this really necessary? The error status from the sub-make is ignored > >>>> only because of the -, isn't it? > >>> > >>> Without the `rm` the include/config/auto.conf is not removed by > >>> Kconfig on error, so the include will still succeed but use the stale > >>> auto.conf file. > >>> > >>> Keep in mind on rebuilds include/config/auto.conf is already present, > >>> so the rule is only executed for the include/config/auto.conf.cmd > >>> target. > >> > >> But the sub-make ought to return failure, which ought to then stop the > >> build process? > > > > For some reason it doesn't, not at least with GNU Make 4.3. > > > > It stops the build if the '-' is dropped from the include of > > include/config/auto.conf.cmd. But that will always fail as > > include/config/auto.conf.cmd is never created. > > > > Maybe there's something weird with our makefile, I certainly don't > > know that much, but as noted in the commit message, > > include/config/auto.conf.cmd failing doesn't cause the build to > > stop. > > How about the below as an alternative? I'm not overly happy with the > double ifneq, but I also don't see a good other option. Hm, yes, having the checks against specific paths is IMO not ideal. > This way the missing auto.conf is detected slightly later, but this > may well be good enough. Then again I might be overlooking yet > something else that this breaks ... > > Jan > > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -375,6 +375,7 @@ $(KCONFIG_CONFIG): tools_fixdep > # This exploits the 'multi-target pattern rule' trick. > # The syncconfig should be executed only once to make all the targets. > include/config/%.conf include/config/%.conf.cmd: $(KCONFIG_CONFIG) > + $(Q)rm -f include/config/$*.conf > $(Q)$(MAKE) $(build)=tools/kconfig syncconfig > > ifeq ($(CONFIG_DEBUG),y) > --- a/xen/Rules.mk > +++ b/xen/Rules.mk > @@ -15,7 +15,11 @@ srcdir := $(srctree)/$(src) > PHONY := __build > __build: > > --include $(objtree)/include/config/auto.conf > +ifneq ($(obj),tools) > +ifneq ($(obj),tools/kconfig) > +include $(objtree)/include/config/auto.conf > +endif > +endif Trying to understand this, I assume it's to avoid an infinite dependency loop that generating include/config/auto.conf requires some tools that are build using xen/Rules.mk? Thanks, Roger.
On 15.02.2024 17:08, Roger Pau Monné wrote: > On Thu, Feb 15, 2024 at 02:02:41PM +0100, Jan Beulich wrote: >> --- a/xen/Rules.mk >> +++ b/xen/Rules.mk >> @@ -15,7 +15,11 @@ srcdir := $(srctree)/$(src) >> PHONY := __build >> __build: >> >> --include $(objtree)/include/config/auto.conf >> +ifneq ($(obj),tools) >> +ifneq ($(obj),tools/kconfig) >> +include $(objtree)/include/config/auto.conf >> +endif >> +endif > > Trying to understand this, I assume it's to avoid an infinite > dependency loop that generating include/config/auto.conf requires some > tools that are build using xen/Rules.mk? The file has dependencies only in xen/Makefile. This is about the file simply not being there when initially building. Perhaps the patch description helps that I've written in the meantime: "Because of using "-include", failure to (re)build auto.conf (with auto.conf.cmd produced as a secondary target) won't stop make from continuing the build. Arrange for it being possible to drop the - from Rules.mk, requiring that the include be skipped for tools-only targets. Note that relying on the inclusion in those cases wouldn't be correct anyway, as it might be a stale file (yet to be rebuilt) which would be included, while during initial build, the file would be absent altogether." Jan
On Thu, Feb 15, 2024 at 05:22:00PM +0100, Jan Beulich wrote: > On 15.02.2024 17:08, Roger Pau Monné wrote: > > On Thu, Feb 15, 2024 at 02:02:41PM +0100, Jan Beulich wrote: > >> --- a/xen/Rules.mk > >> +++ b/xen/Rules.mk > >> @@ -15,7 +15,11 @@ srcdir := $(srctree)/$(src) > >> PHONY := __build > >> __build: > >> > >> --include $(objtree)/include/config/auto.conf > >> +ifneq ($(obj),tools) > >> +ifneq ($(obj),tools/kconfig) > >> +include $(objtree)/include/config/auto.conf > >> +endif > >> +endif > > > > Trying to understand this, I assume it's to avoid an infinite > > dependency loop that generating include/config/auto.conf requires some > > tools that are build using xen/Rules.mk? > > The file has dependencies only in xen/Makefile. This is about the > file simply not being there when initially building. Perhaps the > patch description helps that I've written in the meantime: > > "Because of using "-include", failure to (re)build auto.conf (with > auto.conf.cmd produced as a secondary target) won't stop make from > continuing the build. Arrange for it being possible to drop the - from > Rules.mk, requiring that the include be skipped for tools-only targets. Wouldn't it be more reliable if we skipped the include for any paths in $(obj) that start with 'tools', rather than hardcoding 'tools' and 'tools/kconfig'? Thanks, Roger.
On 15.02.2024 18:23, Roger Pau Monné wrote: > On Thu, Feb 15, 2024 at 05:22:00PM +0100, Jan Beulich wrote: >> On 15.02.2024 17:08, Roger Pau Monné wrote: >>> On Thu, Feb 15, 2024 at 02:02:41PM +0100, Jan Beulich wrote: >>>> --- a/xen/Rules.mk >>>> +++ b/xen/Rules.mk >>>> @@ -15,7 +15,11 @@ srcdir := $(srctree)/$(src) >>>> PHONY := __build >>>> __build: >>>> >>>> --include $(objtree)/include/config/auto.conf >>>> +ifneq ($(obj),tools) >>>> +ifneq ($(obj),tools/kconfig) >>>> +include $(objtree)/include/config/auto.conf >>>> +endif >>>> +endif >>> >>> Trying to understand this, I assume it's to avoid an infinite >>> dependency loop that generating include/config/auto.conf requires some >>> tools that are build using xen/Rules.mk? >> >> The file has dependencies only in xen/Makefile. This is about the >> file simply not being there when initially building. Perhaps the >> patch description helps that I've written in the meantime: >> >> "Because of using "-include", failure to (re)build auto.conf (with >> auto.conf.cmd produced as a secondary target) won't stop make from >> continuing the build. Arrange for it being possible to drop the - from >> Rules.mk, requiring that the include be skipped for tools-only targets. > > Wouldn't it be more reliable if we skipped the include for any paths > in $(obj) that start with 'tools', rather than hardcoding 'tools' and > 'tools/kconfig'? I was first meaning to do so, but the expression would end up more complex than I'd like (for it needing to be an exact match of "tools" and a prefix match of "tools/"). Thinking of it, ifneq ($(obj),tools) ifneq ($(patsubst tools/%,$(obj)),) might do (and not be as complex as I first thought, when intending to put all in a single "if"). Yet then it's not entirely impossible that we might gain a build tool which is actually to be built after .config was (re)generated, i.e. in the know of the target configuration. Thoughts? Jan
On Fri, Feb 16, 2024 at 11:04:46AM +0100, Jan Beulich wrote: > On 15.02.2024 18:23, Roger Pau Monné wrote: > > On Thu, Feb 15, 2024 at 05:22:00PM +0100, Jan Beulich wrote: > >> On 15.02.2024 17:08, Roger Pau Monné wrote: > >>> On Thu, Feb 15, 2024 at 02:02:41PM +0100, Jan Beulich wrote: > >>>> --- a/xen/Rules.mk > >>>> +++ b/xen/Rules.mk > >>>> @@ -15,7 +15,11 @@ srcdir := $(srctree)/$(src) > >>>> PHONY := __build > >>>> __build: > >>>> > >>>> --include $(objtree)/include/config/auto.conf > >>>> +ifneq ($(obj),tools) > >>>> +ifneq ($(obj),tools/kconfig) > >>>> +include $(objtree)/include/config/auto.conf > >>>> +endif > >>>> +endif > >>> > >>> Trying to understand this, I assume it's to avoid an infinite > >>> dependency loop that generating include/config/auto.conf requires some > >>> tools that are build using xen/Rules.mk? > >> > >> The file has dependencies only in xen/Makefile. This is about the > >> file simply not being there when initially building. Perhaps the > >> patch description helps that I've written in the meantime: > >> > >> "Because of using "-include", failure to (re)build auto.conf (with > >> auto.conf.cmd produced as a secondary target) won't stop make from > >> continuing the build. Arrange for it being possible to drop the - from > >> Rules.mk, requiring that the include be skipped for tools-only targets. > > > > Wouldn't it be more reliable if we skipped the include for any paths > > in $(obj) that start with 'tools', rather than hardcoding 'tools' and > > 'tools/kconfig'? > > I was first meaning to do so, but the expression would end up more > complex than I'd like (for it needing to be an exact match of "tools" > and a prefix match of "tools/"). Thinking of it, > > ifneq ($(obj),tools) > ifneq ($(patsubst tools/%,$(obj)),) > > might do (and not be as complex as I first thought, when intending to > put all in a single "if"). Would something like the rune below work? ifneq ($(word 1, $(subst /, ,$(obj))),tools) That should allow to have a single condition, and should match both 'tools' and 'tools/*' Thanks, Roger.
On 16.02.2024 11:51, Roger Pau Monné wrote: > On Fri, Feb 16, 2024 at 11:04:46AM +0100, Jan Beulich wrote: >> On 15.02.2024 18:23, Roger Pau Monné wrote: >>> On Thu, Feb 15, 2024 at 05:22:00PM +0100, Jan Beulich wrote: >>>> On 15.02.2024 17:08, Roger Pau Monné wrote: >>>>> On Thu, Feb 15, 2024 at 02:02:41PM +0100, Jan Beulich wrote: >>>>>> --- a/xen/Rules.mk >>>>>> +++ b/xen/Rules.mk >>>>>> @@ -15,7 +15,11 @@ srcdir := $(srctree)/$(src) >>>>>> PHONY := __build >>>>>> __build: >>>>>> >>>>>> --include $(objtree)/include/config/auto.conf >>>>>> +ifneq ($(obj),tools) >>>>>> +ifneq ($(obj),tools/kconfig) >>>>>> +include $(objtree)/include/config/auto.conf >>>>>> +endif >>>>>> +endif >>>>> >>>>> Trying to understand this, I assume it's to avoid an infinite >>>>> dependency loop that generating include/config/auto.conf requires some >>>>> tools that are build using xen/Rules.mk? >>>> >>>> The file has dependencies only in xen/Makefile. This is about the >>>> file simply not being there when initially building. Perhaps the >>>> patch description helps that I've written in the meantime: >>>> >>>> "Because of using "-include", failure to (re)build auto.conf (with >>>> auto.conf.cmd produced as a secondary target) won't stop make from >>>> continuing the build. Arrange for it being possible to drop the - from >>>> Rules.mk, requiring that the include be skipped for tools-only targets. >>> >>> Wouldn't it be more reliable if we skipped the include for any paths >>> in $(obj) that start with 'tools', rather than hardcoding 'tools' and >>> 'tools/kconfig'? >> >> I was first meaning to do so, but the expression would end up more >> complex than I'd like (for it needing to be an exact match of "tools" >> and a prefix match of "tools/"). Thinking of it, >> >> ifneq ($(obj),tools) >> ifneq ($(patsubst tools/%,$(obj)),) >> >> might do (and not be as complex as I first thought, when intending to >> put all in a single "if"). > > Would something like the rune below work? > > ifneq ($(word 1, $(subst /, ,$(obj))),tools) > > That should allow to have a single condition, and should match both > 'tools' and 'tools/*' Hmm, yes, that works. $(subst ...) is something I usually try to avoid, in favor of $(patsubst ...). Jan
diff --git a/xen/Makefile b/xen/Makefile index 21832d640225..7e6860a58a1d 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -358,10 +358,10 @@ config: tools_fixdep outputmakefile FORCE else # !config-build ifeq ($(need-config),y) --include include/config/auto.conf # Read in dependencies to all Kconfig* files, make sure to run syncconfig if # changes are detected. -include include/config/auto.conf.cmd +include include/config/auto.conf # Allow people to just run `make` as before and not force them to configure # Only run defconfig if $(KCONFIG_CONFIG) is missing @@ -375,6 +375,7 @@ $(KCONFIG_CONFIG): tools_fixdep # This exploits the 'multi-target pattern rule' trick. # The syncconfig should be executed only once to make all the targets. include/config/%.conf include/config/%.conf.cmd: $(KCONFIG_CONFIG) + rm -rf include/config/$*.conf $(Q)$(MAKE) $(build)=tools/kconfig syncconfig ifeq ($(CONFIG_DEBUG),y)
When doing a rebuild with an xen/include/config/auto.conf already present in the tree, failures from Kconfig are ignored since the target is present: gmake -C xen install gmake[1]: Entering directory '/root/src/xen/xen' tools/kconfig/conf --syncconfig Kconfig common/Kconfig:2: syntax error common/Kconfig:1: invalid statement gmake[2]: *** [tools/kconfig/Makefile:73: syncconfig] Error 1 UPD include/xen/compile.h Xen 4.19-unstable gmake[3]: Nothing to be done for 'all'. gmake[2]: 'arch/x86/include/asm/asm-offsets.h' is up to date. Ultimately leading to a successful build despite the Kconfig error. Fix this by first removing xen/include/config/auto.conf before attempting to regenerate, and then also make xen/include/config/auto.conf a hard dependency of the build process (ie: drop the leading '-') and reordering so the .cmd target is executed before including the configuration file. This leads to the build properly failing if the config file cannot be re-generated: gmake -C xen install gmake[1]: Entering directory '/root/src/xen/xen' rm -rf include/config/auto.conf tools/kconfig/conf --syncconfig Kconfig common/Kconfig:2: syntax error common/Kconfig:1: invalid statement gmake[2]: *** [tools/kconfig/Makefile:73: syncconfig] Error 1 gmake[1]: *** [Makefile:379: include/config/auto.conf] Error 2 gmake[1]: Leaving directory '/root/src/xen/xen' gmake: *** [Makefile:143: install-xen] Error 2 Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Maybe there's a better way for dealing with all this, my makefile foo is very limited. --- xen/Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)