| Message ID | 20211125134006.1076646-28-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: Build system improvements, now with out-of-tree build! | expand |
On 25.11.2021 14:39, Anthony PERARD wrote: > Rather than preparing the efi source file, we will make the symbolic > link as needed from the build location. > > The `ln` command is run every time to allow to update the link in case > the source tree change location. Btw, since symlinks aren't being liked, would there be a way to make things work using vpath? > This patch also introduce "efi_common.mk" which allow to reuse the > common make instructions without having to duplicate them into each > arch. > > And now that we have a list of common source file, we can start to > remove the links to the source files on clean. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > > Notes: > v8: > - use symbolic link instead of making a copy of the source > - introduce efi_common.mk > - remove links to source file on clean > - use -iquote for "efi.h" headers in common/efi > > xen/Makefile | 5 ----- > xen/arch/arm/efi/Makefile | 4 ++-- > xen/arch/x86/Makefile | 1 + > xen/arch/x86/efi/Makefile | 5 +---- > xen/common/efi/efi_common.mk | 12 ++++++++++++ Could I talk you into avoiding _ when - is suitable, which is the case not only for (non-exported) make variables, but also file names? > --- a/xen/arch/arm/efi/Makefile > +++ b/xen/arch/arm/efi/Makefile > @@ -1,4 +1,4 @@ > -CFLAGS-y += -fshort-wchar > +include $(srctree)/common/efi/efi_common.mk > > -obj-y += boot.init.o pe.init.o ebmalloc.o runtime.o > +obj-y += $(EFIOBJ-y) > obj-$(CONFIG_ACPI) += efi-dom0.init.o > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile > index e8151bf4b111..eabd8d3919a4 100644 > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -79,6 +79,7 @@ endif > > # Allows "clean" to descend into boot/ > subdir- += boot > +subdir- += efi This renders the comment stale - please generalize it. Also, any reason a similar adjustment isn't needed for Arm? Perhaps this could even move into xen/Makefile: subdir- += $(wildcard efi/) > --- /dev/null > +++ b/xen/common/efi/efi_common.mk > @@ -0,0 +1,12 @@ > +EFIOBJ-y := boot.init.o pe.init.o ebmalloc.o runtime.o > +EFIOBJ-$(CONFIG_COMPAT) += compat.o > + > +CFLAGS-y += -fshort-wchar > +CFLAGS-y += -iquote $(srctree)/common/efi > + > +$(obj)/%.c: $(abs_srctree)/common/efi/%.c FORCE > + $(Q)ln -nfs $< $@ Like was the case before, I think it would be better if the links were relative ones, at least when srctree == objtree (but ideally always). > +clean-files += $(patsubst %.o,%.c, $(EFIOBJ-y:.init.o=.o) $(EFIOBJ-)) Nit: Please be consistent (at least within a single line) about blanks following commas. Jan
On Tue, Dec 21, 2021 at 02:53:49PM +0100, Jan Beulich wrote: > On 25.11.2021 14:39, Anthony PERARD wrote: > > Rather than preparing the efi source file, we will make the symbolic > > link as needed from the build location. > > > > The `ln` command is run every time to allow to update the link in case > > the source tree change location. > > Btw, since symlinks aren't being liked, would there be a way to make > things work using vpath? No, that's not possible. With "vpath = /build/xen/common/efi", make would look at path "/build/xen/common/efi/arch/x86/efi/runtime.c" to build "arch/x86/efi/runtime.o". > > This patch also introduce "efi_common.mk" which allow to reuse the > > common make instructions without having to duplicate them into each > > arch. > > > > And now that we have a list of common source file, we can start to > > remove the links to the source files on clean. > > > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > --- > > > > Notes: > > v8: > > - use symbolic link instead of making a copy of the source > > - introduce efi_common.mk > > - remove links to source file on clean > > - use -iquote for "efi.h" headers in common/efi > > > > xen/Makefile | 5 ----- > > xen/arch/arm/efi/Makefile | 4 ++-- > > xen/arch/x86/Makefile | 1 + > > xen/arch/x86/efi/Makefile | 5 +---- > > xen/common/efi/efi_common.mk | 12 ++++++++++++ > > Could I talk you into avoiding _ when - is suitable, which is the case not > only for (non-exported) make variables, but also file names? I'll try. I'll change this one. > > --- a/xen/arch/arm/efi/Makefile > > +++ b/xen/arch/arm/efi/Makefile > > @@ -1,4 +1,4 @@ > > -CFLAGS-y += -fshort-wchar > > +include $(srctree)/common/efi/efi_common.mk > > > > -obj-y += boot.init.o pe.init.o ebmalloc.o runtime.o > > +obj-y += $(EFIOBJ-y) > > obj-$(CONFIG_ACPI) += efi-dom0.init.o > > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile > > index e8151bf4b111..eabd8d3919a4 100644 > > --- a/xen/arch/x86/Makefile > > +++ b/xen/arch/x86/Makefile > > @@ -79,6 +79,7 @@ endif > > > > # Allows "clean" to descend into boot/ > > subdir- += boot > > +subdir- += efi > > This renders the comment stale - please generalize it. > > Also, any reason a similar adjustment isn't needed for Arm? Yes, there's already "obj- += efi/" on the arm side. On x86, efi/ is only in ALL_OBJS which make.clean doesn't use. > > --- /dev/null > > +++ b/xen/common/efi/efi_common.mk > > @@ -0,0 +1,12 @@ > > +EFIOBJ-y := boot.init.o pe.init.o ebmalloc.o runtime.o > > +EFIOBJ-$(CONFIG_COMPAT) += compat.o > > + > > +CFLAGS-y += -fshort-wchar > > +CFLAGS-y += -iquote $(srctree)/common/efi > > + > > +$(obj)/%.c: $(abs_srctree)/common/efi/%.c FORCE > > + $(Q)ln -nfs $< $@ > > Like was the case before, I think it would be better if the links were > relative ones, at least when srctree == objtree (but ideally always). I can give it a try. > > +clean-files += $(patsubst %.o,%.c, $(EFIOBJ-y:.init.o=.o) $(EFIOBJ-)) > > Nit: Please be consistent (at least within a single line) about blanks > following commas. You mean to never put spaces after a comma because they are sometime significant? That's the only way I know how to be consistent. Thanks,
On 18.01.2022 12:06, Anthony PERARD wrote: > On Tue, Dec 21, 2021 at 02:53:49PM +0100, Jan Beulich wrote: >> On 25.11.2021 14:39, Anthony PERARD wrote: >>> Rather than preparing the efi source file, we will make the symbolic >>> link as needed from the build location. >>> >>> The `ln` command is run every time to allow to update the link in case >>> the source tree change location. >> >> Btw, since symlinks aren't being liked, would there be a way to make >> things work using vpath? > > No, that's not possible. With "vpath = /build/xen/common/efi", make > would look at path "/build/xen/common/efi/arch/x86/efi/runtime.c" to > build "arch/x86/efi/runtime.o". But /build/xen/common/efi/arch/x86/efi/runtime.c doesn't exist (and never will afaict), so wouldn't make go on finding the file elsewhere? >>> +clean-files += $(patsubst %.o,%.c, $(EFIOBJ-y:.init.o=.o) $(EFIOBJ-)) >> >> Nit: Please be consistent (at least within a single line) about blanks >> following commas. > > You mean to never put spaces after a comma because they are sometime > significant? That's the only way I know how to be consistent. When spaces are significant, they of course cannot be stripped. But here they aren't (afaict), and hence clean-files += $(patsubst %.o,%.c,$(EFIOBJ-y:.init.o=.o) $(EFIOBJ-)) or clean-files += $(patsubst %.o, %.c, $(EFIOBJ-y:.init.o=.o) $(EFIOBJ-)) are the two consistent (in my eyes) forms. Jan
On Tue, Jan 18, 2022 at 11:06:38AM +0000, Anthony PERARD wrote: > On Tue, Dec 21, 2021 at 02:53:49PM +0100, Jan Beulich wrote: > > On 25.11.2021 14:39, Anthony PERARD wrote: > > > +$(obj)/%.c: $(abs_srctree)/common/efi/%.c FORCE > > > + $(Q)ln -nfs $< $@ > > > > Like was the case before, I think it would be better if the links were > > relative ones, at least when srctree == objtree (but ideally always). > > I can give it a try. How about: ln -nfs $(subst $(space),/,$(patsubst %,..,$(subst /, ,$(obj))))/$(if $(building_out_of_srctree),source/)common/efi/$(<F) $@ This will result in the relative path "../../../common/efi/runtime.c" for in-tree builds, and "../../../source/common/efi/runtime.c" for out-of-tree builds. There's a "source" symlink that can be used which point to the source tree when doing out-of-tree builds. The part: $(subst $(space),/,$(patsubst %,..,$(subst /, ,$(obj)))) means that if $(obj) were to be something other than "arch/*/efi", the command would still works and give the right number of "../". It does this steps to "arch/x86/efi": arch x86 efi .. .. .. ../../.. The added "source/" depends on whether we do out-of-tree build or not.
On Tue, Jan 18, 2022 at 02:49:37PM +0100, Jan Beulich wrote: > On 18.01.2022 12:06, Anthony PERARD wrote: > > On Tue, Dec 21, 2021 at 02:53:49PM +0100, Jan Beulich wrote: > >> On 25.11.2021 14:39, Anthony PERARD wrote: > >>> Rather than preparing the efi source file, we will make the symbolic > >>> link as needed from the build location. > >>> > >>> The `ln` command is run every time to allow to update the link in case > >>> the source tree change location. > >> > >> Btw, since symlinks aren't being liked, would there be a way to make > >> things work using vpath? > > > > No, that's not possible. With "vpath = /build/xen/common/efi", make > > would look at path "/build/xen/common/efi/arch/x86/efi/runtime.c" to > > build "arch/x86/efi/runtime.o". > > But /build/xen/common/efi/arch/x86/efi/runtime.c doesn't exist (and > never will afaict), so wouldn't make go on finding the file elsewhere? If we have the implicit rule %.o: %.c and vpath set. When trying to build the target "arch/x86/efi/runtime.o", make will try our above rule and look for "arch/x86/efi/runtime.c" as make will not remove the directory part from the path. If that prerequisite isn't found, make will also try to prepend $(VPATH) to it. I don't think it's not possible to tell make to substitute a directory for vpath, in a target or prerequisite. Cheers,
On 18.01.2022 17:21, Anthony PERARD wrote: > On Tue, Jan 18, 2022 at 11:06:38AM +0000, Anthony PERARD wrote: >> On Tue, Dec 21, 2021 at 02:53:49PM +0100, Jan Beulich wrote: >>> On 25.11.2021 14:39, Anthony PERARD wrote: >>>> +$(obj)/%.c: $(abs_srctree)/common/efi/%.c FORCE >>>> + $(Q)ln -nfs $< $@ >>> >>> Like was the case before, I think it would be better if the links were >>> relative ones, at least when srctree == objtree (but ideally always). >> >> I can give it a try. > > How about: > ln -nfs $(subst $(space),/,$(patsubst %,..,$(subst /, ,$(obj))))/$(if $(building_out_of_srctree),source/)common/efi/$(<F) $@ > > This will result in the relative path "../../../common/efi/runtime.c" > for in-tree builds, and "../../../source/common/efi/runtime.c" for > out-of-tree builds. There's a "source" symlink that can be used which > point to the source tree when doing out-of-tree builds. > > The part: > $(subst $(space),/,$(patsubst %,..,$(subst /, ,$(obj)))) > means that if $(obj) were to be something other than "arch/*/efi", the > command would still works and give the right number of "../". > It does this steps to "arch/x86/efi": > arch x86 efi > .. .. .. > ../../.. Looks good to me. > The added "source/" depends on whether we do out-of-tree build or not. Well, I guess we're free to add a "source => ." symlink even in the in-tree build case, deviating slightly from what Linux does? That would then slightly simplify your construct. Jan
On Wed, Jan 19, 2022 at 08:46:09AM +0100, Jan Beulich wrote: > On 18.01.2022 17:21, Anthony PERARD wrote: > > The added "source/" depends on whether we do out-of-tree build or not. > > Well, I guess we're free to add a "source => ." symlink even in the > in-tree build case, deviating slightly from what Linux does? That > would then slightly simplify your construct. I'm not sure about that, but I can see only one issue with that: if one was to use "source/" in a make rules (in target or prerequisite), make would potentially have two different path leading to the same file when running out-of-tree. But that shouldn't happen has we have "$(srctree)". Currently, Kbuild doesn't remove "source" on distclean, but I guess we can do that, which would avoid the potential issue. I don't see other reason a link "source -> ." would be an issue, so I'll add that to the patch "build: adding out-of-tree support to the xen build". Thanks,
diff --git a/xen/Makefile b/xen/Makefile index 90e8191f51ce..2a809d577fc3 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -447,11 +447,6 @@ $(TARGET).gz: $(TARGET) $(TARGET): FORCE $(Q)$(MAKE) $(build)=tools $(Q)$(MAKE) $(build)=. include/xen/compile.h - [ -e arch/$(TARGET_ARCH)/efi ] && for f in $$(cd common/efi; echo *.[ch]); \ - do test -r arch/$(TARGET_ARCH)/efi/$$f || \ - ln -nsf ../../../common/efi/$$f arch/$(TARGET_ARCH)/efi/; \ - done; \ - true $(Q)$(MAKE) $(build)=include all $(Q)$(MAKE) $(build)=arch/$(TARGET_ARCH) include $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h diff --git a/xen/arch/arm/efi/Makefile b/xen/arch/arm/efi/Makefile index 1b1ed06feddc..57616a17cb03 100644 --- a/xen/arch/arm/efi/Makefile +++ b/xen/arch/arm/efi/Makefile @@ -1,4 +1,4 @@ -CFLAGS-y += -fshort-wchar +include $(srctree)/common/efi/efi_common.mk -obj-y += boot.init.o pe.init.o ebmalloc.o runtime.o +obj-y += $(EFIOBJ-y) obj-$(CONFIG_ACPI) += efi-dom0.init.o diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index e8151bf4b111..eabd8d3919a4 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -79,6 +79,7 @@ endif # Allows "clean" to descend into boot/ subdir- += boot +subdir- += efi extra-y += asm-macros.i extra-y += xen.lds diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile index ac815f02cb5e..81fda12a70ea 100644 --- a/xen/arch/x86/efi/Makefile +++ b/xen/arch/x86/efi/Makefile @@ -1,4 +1,4 @@ -CFLAGS-y += -fshort-wchar +include $(srctree)/common/efi/efi_common.mk quiet_cmd_objcopy_o_ihex = OBJCOPY $@ cmd_objcopy_o_ihex = $(OBJCOPY) -I ihex -O binary $< $@ @@ -8,9 +8,6 @@ $(obj)/%.o: $(src)/%.ihex FORCE $(obj)/boot.init.o: $(obj)/buildid.o -EFIOBJ-y := boot.init.o pe.init.o ebmalloc.o runtime.o -EFIOBJ-$(CONFIG_COMPAT) += compat.o - $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4) $(addprefix $(obj)/,$(EFIOBJ-y)): CFLAGS-stack-boundary := $(cflags-stack-boundary) diff --git a/xen/common/efi/efi_common.mk b/xen/common/efi/efi_common.mk new file mode 100644 index 000000000000..d2845fd6b3c8 --- /dev/null +++ b/xen/common/efi/efi_common.mk @@ -0,0 +1,12 @@ +EFIOBJ-y := boot.init.o pe.init.o ebmalloc.o runtime.o +EFIOBJ-$(CONFIG_COMPAT) += compat.o + +CFLAGS-y += -fshort-wchar +CFLAGS-y += -iquote $(srctree)/common/efi + +$(obj)/%.c: $(abs_srctree)/common/efi/%.c FORCE + $(Q)ln -nfs $< $@ + +clean-files += $(patsubst %.o,%.c, $(EFIOBJ-y:.init.o=.o) $(EFIOBJ-)) + +.PRECIOUS: $(obj)/%.c
Rather than preparing the efi source file, we will make the symbolic link as needed from the build location. The `ln` command is run every time to allow to update the link in case the source tree change location. This patch also introduce "efi_common.mk" which allow to reuse the common make instructions without having to duplicate them into each arch. And now that we have a list of common source file, we can start to remove the links to the source files on clean. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Notes: v8: - use symbolic link instead of making a copy of the source - introduce efi_common.mk - remove links to source file on clean - use -iquote for "efi.h" headers in common/efi xen/Makefile | 5 ----- xen/arch/arm/efi/Makefile | 4 ++-- xen/arch/x86/Makefile | 1 + xen/arch/x86/efi/Makefile | 5 +---- xen/common/efi/efi_common.mk | 12 ++++++++++++ 5 files changed, 16 insertions(+), 11 deletions(-) create mode 100644 xen/common/efi/efi_common.mk