Re: [XEN PATCH v8 27/47] build: grab common EFI source files in arch specific dir

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> > 
> > 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,

-- 
Anthony PERARD