[PATCH] build/xen: fail to rebuild if Kconfig fails

[PATCH] build/xen: fail to rebuild if Kconfig fails

[Roger Pau Monne]

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(-)

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)
-- 
2.43.0

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Jan Beulich]

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

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Roger Pau Monné]

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.

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Jan Beulich]

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

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Roger Pau Monné]

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.

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Jan Beulich]

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

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Jan Beulich]

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

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Jan Beulich]

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

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Roger Pau Monné]

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.

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Jan Beulich]

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

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Roger Pau Monné]

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.

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Jan Beulich]

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

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Roger Pau Monné]

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.

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Jan Beulich]

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

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Anthony PERARD]

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,

-- 
Anthony PERARD

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Jan Beulich]

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

Re: [PATCH] build/xen: fail to rebuild if Kconfig fails

[Roger Pau Monné]

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.