Re: [XEN PATCH] libs: Fix unstable libs build on FreeBSD, auto-generate version-script

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Feb 15, 2023 at 04:30:43PM +0100, Jan Beulich wrote:
> On 15.02.2023 16:21, Anthony PERARD wrote:
> > @@ -13,6 +14,10 @@ MAJOR := $(shell $(XEN_ROOT)/version.sh 
> > $(XEN_ROOT)/xen/Makefile)
> >  endif
> >  MINOR ?= 0
> >  
> > +ifeq ($(origin version-script), undefined)
> > +version-script := libxen$(LIBNAME).map.tmp
> > +endif
> 
> Such a use of $(origin ...) is pretty fragile. Maybe better use ?= ?

I'm not sure why I used $(origin), I've written that more than 6 month
ago, but this way of using it is actually described in the manual, when
documenting ?= but with a = instead of := .
So, maybe I wanted to have ?= but with immediate evaluation rather than
deferred. Maybe I had issue with $(version-script) evaluating to
different values.

If that ok, I'd like to keep using these runes, since ?:= doesn't exist.

> > @@ -72,6 +77,10 @@ headers.lst: FORCE
> >     @{ set -e; $(foreach h,$(LIBHEADERS),echo $(h);) } > $@.tmp
> >     @$(call move-if-changed,$@.tmp,$@)
> >  
> > +libxen$(LIBNAME).map.tmp: FORCE
> > +   echo 'VERS_$(MAJOR).$(MINOR) { global: *; };' >.$@.tmp
> > +   $(call move-if-changed,.$@.tmp,$@)
> 
> Isn't this going to get in the way of your "build everything from root"
> effort, where $@ will include a path? Also do we really need .tmp.tmp
> files?

Good call, I'll replace that with $(@D)/.$(@F), that will be one
less thing to deal with, Also, I guess just adding a dot to the filename
would be enough, and we would have `mv .libxen.map.tmp libxen.map.tmp`,
and no more .tmp.tmp files.

> >  lib$(LIB_FILE_NAME).a: $(OBJS-y)
> 
> Seeing this right adjacent in context - any reason you use libxen$(LIBNAME)
> and not the same lib$(LIB_FILE_NAME) for the base file name?

That was the name used before, I guess it would be fine to rename. I
just need to set $(version-script) later as $(LIB_FILE_NAME) would not
be defined yet.

> > @@ -120,7 +129,7 @@ TAGS:
> >  clean::
> >     rm -rf $(TARGETS) *~ $(DEPS_RM) $(OBJS-y) $(PIC_OBJS)
> >     rm -f lib$(LIB_FILE_NAME).so.$(MAJOR).$(MINOR) 
> > lib$(LIB_FILE_NAME).so.$(MAJOR)
> > -   rm -f headers.chk headers.lst
> > +   rm -f headers.chk headers.lst libxen*.map.tmp
> 
> If I hadn't checked, I'd have assumed that *.tmp are removed without
> being named explicitly. So yes, I see the need for the addition, but then
> I wonder why you don't also remove the .*.tmp.tmp file, which may be left
> around if the build is interrupted at exactly the "right" time.

I forgot because $(move-if-changed) remove the temporary file. I'll
clean the .*.tmp file.

Thanks,

-- 
Anthony PERARD