| Message ID | 55c3a781-17f8-47f2-9629-515e1aea77aa@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | build: make sure build fails when running kconfig fails | expand |
On Tue, Feb 20, 2024 at 09:43:56AM +0100, Jan Beulich wrote: > 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. > > Fixes: 8d4c17a90b0a ("xen/build: silence make warnings about missing auto.conf*") > Reported-by: Roger Pau Monné <roger.pau@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Just to make sure, this patch is a workaround to a harmless bug in older version of GNU Make which print spurious error messages, and said bug as been fixed in GNU Make 4.2, right? Bug report: bug #102: Make prints an incorrect error for missing includes https://savannah.gnu.org/bugs/?func=detailitem&item_id=102 > --- 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 Maybe this should say "include/config/auto.conf" instead of using "$*". "syncconfig" is going to generate "auto.conf" and not "$*.conf". And it would make easier to find the "rm" command via "grep". > $(Q)$(MAKE) $(build)=tools/kconfig syncconfig > Thanks,
On 20.02.2024 12:18, Anthony PERARD wrote: > On Tue, Feb 20, 2024 at 09:43:56AM +0100, Jan Beulich wrote: >> 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. >> >> Fixes: 8d4c17a90b0a ("xen/build: silence make warnings about missing auto.conf*") >> Reported-by: Roger Pau Monné <roger.pau@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Just to make sure, this patch is a workaround to a harmless bug in older > version of GNU Make which print spurious error messages, and said bug as > been fixed in GNU Make 4.2, right? Bug report: > > bug #102: Make prints an incorrect error for missing includes > https://savannah.gnu.org/bugs/?func=detailitem&item_id=102 Assuming you mean the change referenced in Fixes: - yes, that's my understanding. (Whether the referenced make bug is the one I can't tell, though. But it looks like it is.) >> --- 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 > > Maybe this should say "include/config/auto.conf" instead of using "$*". > "syncconfig" is going to generate "auto.conf" and not "$*.conf". And it > would make easier to find the "rm" command via "grep". Generally I advocate for using $* wherever possible in pattern rules. If, however, replacing that is the only way to get an ack for this change, then I would (hesitantly) do so. Jan
On Tue, Feb 20, 2024 at 01:20:16PM +0100, Jan Beulich wrote: > On 20.02.2024 12:18, Anthony PERARD wrote: > > On Tue, Feb 20, 2024 at 09:43:56AM +0100, Jan Beulich wrote: > >> 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. > >> > >> Fixes: 8d4c17a90b0a ("xen/build: silence make warnings about missing auto.conf*") > >> Reported-by: Roger Pau Monné <roger.pau@citrix.com> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Just to make sure, this patch is a workaround to a harmless bug in older > > version of GNU Make which print spurious error messages, and said bug as > > been fixed in GNU Make 4.2, right? Bug report: > > > > bug #102: Make prints an incorrect error for missing includes > > https://savannah.gnu.org/bugs/?func=detailitem&item_id=102 > > Assuming you mean the change referenced in Fixes: - yes, that's my > understanding. (Whether the referenced make bug is the one I can't tell, > though. But it looks like it is.) > > >> --- 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 > > > > Maybe this should say "include/config/auto.conf" instead of using "$*". > > "syncconfig" is going to generate "auto.conf" and not "$*.conf". And it > > would make easier to find the "rm" command via "grep". > > Generally I advocate for using $* wherever possible in pattern rules. If, Well, it might look like a pattern rule, but it isn't one. It's a "groupes targets" rule that works on version of Make that doesn't have this brand new feature. It would be written as: include/config/auto.conf include/config/auto.conf.cmd&: $(KCONFIG_CONFIG) Second, the other command in the recipe doesn't use "$*", so it's not possible to use this rule with a value of $* other than "auto". > however, replacing that is the only way to get an ack for this change, then > I would (hesitantly) do so. Yes please, even if I don't like the patch. I hope it isn't going to byte us later if something in "tools/" happens to start depending on Kconfig (only fixdep and kconfig need to not depend on kconfig at the moment). But the patch currently works and looks ok. So, with "$*" replace by "auto", you can add my "Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>". Thanks,
On 21.02.2024 16:22, Anthony PERARD wrote: > On Tue, Feb 20, 2024 at 01:20:16PM +0100, Jan Beulich wrote: >> On 20.02.2024 12:18, Anthony PERARD wrote: >>> On Tue, Feb 20, 2024 at 09:43:56AM +0100, Jan Beulich wrote: >>>> 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. >>>> >>>> Fixes: 8d4c17a90b0a ("xen/build: silence make warnings about missing auto.conf*") >>>> Reported-by: Roger Pau Monné <roger.pau@citrix.com> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> Just to make sure, this patch is a workaround to a harmless bug in older >>> version of GNU Make which print spurious error messages, and said bug as >>> been fixed in GNU Make 4.2, right? Bug report: >>> >>> bug #102: Make prints an incorrect error for missing includes >>> https://savannah.gnu.org/bugs/?func=detailitem&item_id=102 >> >> Assuming you mean the change referenced in Fixes: - yes, that's my >> understanding. (Whether the referenced make bug is the one I can't tell, >> though. But it looks like it is.) >> >>>> --- 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 >>> >>> Maybe this should say "include/config/auto.conf" instead of using "$*". >>> "syncconfig" is going to generate "auto.conf" and not "$*.conf". And it >>> would make easier to find the "rm" command via "grep". >> >> Generally I advocate for using $* wherever possible in pattern rules. If, > > Well, it might look like a pattern rule, but it isn't one. It's a > "groupes targets" rule that works on version of Make that doesn't have > this brand new feature. It would be written as: > include/config/auto.conf include/config/auto.conf.cmd&: $(KCONFIG_CONFIG) Assuming new enough make. > Second, the other command in the recipe doesn't use "$*", so it's not > possible to use this rule with a value of $* other than "auto". True. "auto" isn't used there either, though. To emphasize this isn't really a pattern rule, how about shrinking the stem along the lines of what I've done in https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=79d635fc64ba9ca97c24c0c52c9924dba77e9527 e.g. making it just the (first) dot here? >> however, replacing that is the only way to get an ack for this change, then >> I would (hesitantly) do so. > > Yes please, even if I don't like the patch. I hope it isn't going to > byte us later if something in "tools/" happens to start depending on > Kconfig (only fixdep and kconfig need to not depend on kconfig at the > moment). But the patch currently works and looks ok. So, with "$*" > replace by "auto", you can add my "Reviewed-by: Anthony PERARD > <anthony.perard@citrix.com>". Thanks. 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,9 @@ srcdir := $(srctree)/$(src) PHONY := __build __build: --include $(objtree)/include/config/auto.conf +ifneq ($(firstword $(subst /, ,$(obj))),tools) +include $(objtree)/include/config/auto.conf +endif include $(XEN_ROOT)/Config.mk include $(srctree)/scripts/Kbuild.include
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. Fixes: 8d4c17a90b0a ("xen/build: silence make warnings about missing auto.conf*") Reported-by: Roger Pau Monné <roger.pau@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- An alternative to the %.conf rule adjustment would be to remove the file only when the sub-make there failed.