| Message ID | 20211213190449.4830-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/libs: Don't recursively expand MAJOR ?= $(shell ...) | expand |
On 13.12.2021 20:04, Andrew Cooper wrote: > --- a/tools/libs/libs.mk > +++ b/tools/libs/libs.mk > @@ -6,7 +6,10 @@ > # MINOR: minor version of lib (0 if empty) > > LIBNAME := $(notdir $(CURDIR)) > -MAJOR ?= $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile) > + > +ifeq ($(origin MAJOR), undefined) > +MAJOR := $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile) > +endif > MINOR ?= 0 > > SHLIB_LDFLAGS += -Wl,--version-script=libxen$(LIBNAME).map Wouldn't it be better to move the "endif" past the setting of MINOR (which then could use := as well)? Libraries with their own versioning would imo better specify both rather than relying on getting 0 from here (which at present none of them does). Would require an adjustment to the comment at the top of libs.mk, though. And further, since you're switching to $(origin ...), wouldn't this be an opportunity to avoid stray inheriting of values from the environment, by switching to "ifneq ($(origin MAJOR), file)"? Or is there an intention of allowing such control via the environment (which would then override the versions for all libraries not explicitly setting them)? In turn I would then further wonder whether command line overrides are intended, but I guess people doing so ought to indeed get what they have asked for (all libraries versioned identically, assuming both MAJOR and MINOR get defined that way). Jan
On Mon, Dec 13, 2021 at 07:04:49PM +0000, Andrew Cooper wrote: > Something slightly weird is going on. Before this, the exact number of hits > that verson.sh gets isn't stable, even when running repeat incremental builds. > I suspect this means we've got a lurking parallel build issue. It could simply be that `make` have decided that one of the Makefile have been updated and thus `make` need to reexecute. And I'm pretty sure it would be because of the generation of those .*.d2 files from .*.d dependency files.
On 14/12/2021 08:17, Jan Beulich wrote: > On 13.12.2021 20:04, Andrew Cooper wrote: >> --- a/tools/libs/libs.mk >> +++ b/tools/libs/libs.mk >> @@ -6,7 +6,10 @@ >> # MINOR: minor version of lib (0 if empty) >> >> LIBNAME := $(notdir $(CURDIR)) >> -MAJOR ?= $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile) >> + >> +ifeq ($(origin MAJOR), undefined) >> +MAJOR := $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile) >> +endif >> MINOR ?= 0 >> >> SHLIB_LDFLAGS += -Wl,--version-script=libxen$(LIBNAME).map > Wouldn't it be better to move the "endif" past the setting of MINOR > (which then could use := as well)? Libraries with their own versioning > would imo better specify both rather than relying on getting 0 from > here (which at present none of them does). Would require an > adjustment to the comment at the top of libs.mk, though. I considered that, but decided against it. Absolutely nothing good can come of having a mix/match of whether MAJOR and MINOR are set, and the whole point of this logic is to provide a safe default when things are unspecified. > > And further, since you're switching to $(origin ...), wouldn't this > be an opportunity to avoid stray inheriting of values from the > environment, by switching to "ifneq ($(origin MAJOR), file)"? No. Not because I think setting MAJOR on the command line is sensible, but because it fails the principle of lease surprise. Basically all variables are editable on the command line and the environment. Prohibiting this one alone is bizarre, unnecessary, and fragile in the case where if it is encountered, it's probably someone who knows exactly what they're doing, trying to debug the build system. ~Andrew
On 14.12.2021 13:52, Andrew Cooper wrote: > On 14/12/2021 08:17, Jan Beulich wrote: >> On 13.12.2021 20:04, Andrew Cooper wrote: >>> --- a/tools/libs/libs.mk >>> +++ b/tools/libs/libs.mk >>> @@ -6,7 +6,10 @@ >>> # MINOR: minor version of lib (0 if empty) >>> >>> LIBNAME := $(notdir $(CURDIR)) >>> -MAJOR ?= $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile) >>> + >>> +ifeq ($(origin MAJOR), undefined) >>> +MAJOR := $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile) >>> +endif >>> MINOR ?= 0 >>> >>> SHLIB_LDFLAGS += -Wl,--version-script=libxen$(LIBNAME).map >> Wouldn't it be better to move the "endif" past the setting of MINOR >> (which then could use := as well)? Libraries with their own versioning >> would imo better specify both rather than relying on getting 0 from >> here (which at present none of them does). Would require an >> adjustment to the comment at the top of libs.mk, though. > > I considered that, but decided against it. > > Absolutely nothing good can come of having a mix/match of whether MAJOR > and MINOR are set, and the whole point of this logic is to provide a > safe default when things are unspecified. > >> >> And further, since you're switching to $(origin ...), wouldn't this >> be an opportunity to avoid stray inheriting of values from the >> environment, by switching to "ifneq ($(origin MAJOR), file)"? > > No. Not because I think setting MAJOR on the command line is sensible, > but because it fails the principle of lease surprise. > > Basically all variables are editable on the command line and the > environment. Prohibiting this one alone is bizarre, unnecessary, and > fragile in the case where if it is encountered, it's probably someone > who knows exactly what they're doing, trying to debug the build system. And then there's that someone else who ends up having MAJOR or MINOR set in the environment from whatever was done previously in a shell. The two variables are simply of too generic name to sensibly be communicated via the environment (and I specifically separate the variant where they're specified on the make command line). Anyway - none of what I've said is an objection. I was merely hoping we could get the whole thing a little less fragile at this occasion. Jan
On Mon, Dec 13, 2021 at 07:04:49PM +0000, Andrew Cooper wrote: > ?= is a deferred assignment. Switch to an alternative form which lets us use > an immediate assignment. > > Before, version.sh gets run anywhere between 46 and 88 times, with 50 on a > `clean`. After, 6 times, invariant of main rune, and whether it is an Instead of just 6, you probably mean between 6 and 12 times, "make clean; make; make", the last make would run version.sh 12 times. > incremental build or not. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> Thanks,
diff --git a/tools/libs/libs.mk b/tools/libs/libs.mk index dfbbef4080f6..b21e0bf083a0 100644 --- a/tools/libs/libs.mk +++ b/tools/libs/libs.mk @@ -6,7 +6,10 @@ # MINOR: minor version of lib (0 if empty) LIBNAME := $(notdir $(CURDIR)) -MAJOR ?= $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile) + +ifeq ($(origin MAJOR), undefined) +MAJOR := $(shell $(XEN_ROOT)/version.sh $(XEN_ROOT)/xen/Makefile) +endif MINOR ?= 0 SHLIB_LDFLAGS += -Wl,--version-script=libxen$(LIBNAME).map
?= is a deferred assignment. Switch to an alternative form which lets us use an immediate assignment. Before, version.sh gets run anywhere between 46 and 88 times, with 50 on a `clean`. After, 6 times, invariant of main rune, and whether it is an incremental build or not. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Juergen Gross <jgross@suse.com> CC: Wei Liu <wl@xen.org> CC: Anthony PERARD <anthony.perard@citrix.com> The identity transform comes from the docs https://www.gnu.org/software/make/manual/make.html#Flavors (final paragraph). Something slightly weird is going on. Before this, the exact number of hits that verson.sh gets isn't stable, even when running repeat incremental builds. I suspect this means we've got a lurking parallel build issue. --- tools/libs/libs.mk | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)