| Message ID | 9e63c6e5-11cb-9f0e-b33e-0247b17e3785@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | build: out-of-tree building adjustments | expand |
On Wed, Mar 29, 2023 at 12:22:16PM +0200, Jan Beulich wrote: > I don't view a variable of this name as suitable for exporting, the more We could rename it. > that it carries entirely redundant information. The reasons for its The patch replace building_out_of_srctree with abs_objtree and abs_srctree which also carries redundant informations. abs_objtree can probably be replaced by $(CURDIR), abs_srctree can be recalculated from $(srctree). > introduction in Linux commit 051f278e9d81 ("kbuild: replace > KBUILD_SRCTREE with boolean building_out_of_srctree") also don't apply > to us. Ditch exporting of the variable, replacing uses suitably. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> This patch feels like obfuscation of the intended test. Instead of reading a test for "out_of_tree", we now have to guess why the two paths are been compared. Anyway, there isn't that many use outside of the main Makefile, so I guess the patch is kind of ok: Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> > For further reasons (besides the similar redundancy aspect) exporting > VPATH looks also suspicious: Its name being all uppercase makes it a > "non application private" variable, i.e. it or its (pre-existing) value > may have a purpose/use elsewhere. And exporting it looks to be easily This sounds like you don't know what VPATH is for, but I'm pretty sure you do. If there's an pre-existing value, we just ignore it. If VPATH is used by a program that our Makefile used and that program is intended to be used by build system then that a bug in that program for not knowing about makes' VPATH. So I don't think we need to worried about it been exported. > avoidable: Instead of setting it in xen/Makefile, it looks like it could > be set in xen/scripts/Kbuild.include. Thoughts? I'd rather not make that change unless there's a real issue with exporting VPATH. We are more likely to introduce a bug than to avoid one. Thanks,
On 04.05.2023 16:49, Anthony PERARD wrote: > On Wed, Mar 29, 2023 at 12:22:16PM +0200, Jan Beulich wrote: >> I don't view a variable of this name as suitable for exporting, the more > > We could rename it. > >> that it carries entirely redundant information. The reasons for its > > The patch replace building_out_of_srctree with abs_objtree and > abs_srctree which also carries redundant informations. abs_objtree can > probably be replaced by $(CURDIR), abs_srctree can be > recalculated from $(srctree). > >> introduction in Linux commit 051f278e9d81 ("kbuild: replace >> KBUILD_SRCTREE with boolean building_out_of_srctree") also don't apply >> to us. Ditch exporting of the variable, replacing uses suitably. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > This patch feels like obfuscation of the intended test. Instead of > reading a test for "out_of_tree", we now have to guess why the two paths > are been compared. Hmm, it's quite the other way around for me: I view the variable as obfuscating, as it hides what it actually expresses (or better based on what properties it is actually set). > Anyway, there isn't that many use outside of the main Makefile, so I > guess the patch is kind of ok: > Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> Thanks. >> For further reasons (besides the similar redundancy aspect) exporting >> VPATH looks also suspicious: Its name being all uppercase makes it a >> "non application private" variable, i.e. it or its (pre-existing) value >> may have a purpose/use elsewhere. And exporting it looks to be easily > > This sounds like you don't know what VPATH is for, but I'm pretty sure > you do. If there's an pre-existing value, we just ignore it. If VPATH is > used by a program that our Makefile used and that program is intended to > be used by build system then that a bug in that program for not knowing > about makes' VPATH. So I don't think we need to worried about it been > exported. We may use programs from our build system which aren't aware they might be used that way. No matter that I know what VPATH is for, I consider its name to violate the shell spec. >> avoidable: Instead of setting it in xen/Makefile, it looks like it could >> be set in xen/scripts/Kbuild.include. Thoughts? > > I'd rather not make that change unless there's a real issue with > exporting VPATH. We are more likely to introduce a bug than to avoid > one. Well, okay, it's surely (hopefully) a highly theoretical consideration anyway. Jan
--- a/xen/Makefile +++ b/xen/Makefile @@ -208,7 +208,7 @@ endif objtree := . VPATH := $(srctree) -export building_out_of_srctree srctree objtree VPATH +export srctree objtree VPATH export XEN_ROOT := $(abs_srctree)/.. --- a/xen/arch/x86/boot/Makefile +++ b/xen/arch/x86/boot/Makefile @@ -14,7 +14,7 @@ $(obj)/head.o: $(head-bin-objs:.o=.bin) CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS)) $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS)) CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -ifdef building_out_of_srctree +ifneq ($(abs_objtree),$(abs_srctree)) CFLAGS_x86_32 += -I$(objtree)/include endif CFLAGS_x86_32 += -I$(srctree)/include --- a/xen/scripts/Makefile.host +++ b/xen/scripts/Makefile.host @@ -88,7 +88,7 @@ _hostcxx_flags = $(HOSTCXXFLAGS) $(HOST_ $(HOSTCXXFLAGS_$(target-stem).o) # $(objtree)/$(obj) for including generated headers from checkin source files -ifdef building_out_of_srctree +ifneq ($(abs_objtree),$(abs_srctree)) _hostc_flags += -I $(objtree)/$(obj) _hostcxx_flags += -I $(objtree)/$(obj) endif
I don't view a variable of this name as suitable for exporting, the more that it carries entirely redundant information. The reasons for its introduction in Linux commit 051f278e9d81 ("kbuild: replace KBUILD_SRCTREE with boolean building_out_of_srctree") also don't apply to us. Ditch exporting of the variable, replacing uses suitably. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: New. --- For further reasons (besides the similar redundancy aspect) exporting VPATH looks also suspicious: Its name being all uppercase makes it a "non application private" variable, i.e. it or its (pre-existing) value may have a purpose/use elsewhere. And exporting it looks to be easily avoidable: Instead of setting it in xen/Makefile, it looks like it could be set in xen/scripts/Kbuild.include. Thoughts?