Re: [XEN PATCH 36/57] tools/xenstore: Cleanup makefile

[Andrew Cooper <amc96@xxxxxxxx>]

On 06/12/2021 17:02, Anthony PERARD wrote:
> Regroup *FLAGS together, use $(LDLIBS).
>
> Remove $(LDLIBS_xenstored) which was the wrong name name as it doesn't
> decribe how to link to a potential libxenstored.so, instead add the
> value to $(LDLIBS) of xenstored.
>
> Add SYSTEMD_LIBS into $(LDLIBS) instead of $(LDFLAGS).
>
> Remove the "-I." from $(CFLAGS), it shouldn't be needed.
>
> Removed $(CFLAGS-y) and $(LDFLAGS-y). $(CFLAGS-y) is already included
> in $(CFLAGS) and both aren't used anyway.
>
> Rename ALL_TARGETS to TARGETS.
> Only add programmes we want to build in $(TARGETS), not phony-targets
> (replace "clients").
>
> Store all `xenstored` objs into $(XENSTORED_OBJS-y).
>
> Replace one $< by $^ even if there's only one dependency,
> (xenstore-control).
>
> clean: "init-xenstore-domain" isn't built here, so stop trying to
> remove it, remove $(TARGETS).
>
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
>  tools/xenstore/Makefile | 81 ++++++++++++++++++++++-------------------
>  1 file changed, 44 insertions(+), 37 deletions(-)
>
> diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
> index 292b478fa1..7fe1d9c1e2 100644
> --- a/tools/xenstore/Makefile
> +++ b/tools/xenstore/Makefile
> @@ -5,7 +5,6 @@ MAJOR = 3.0
>  MINOR = 3

Actually, shouldn't this MAJOR/MINOR be dropped now that libxenstore
moved to tools/libs ?

It's definitely wrong now, seeing as we bumped to 4.0 for Xen 4.16.

>  
>  CFLAGS += -Werror
> -CFLAGS += -I.
>  # Include configure output (config.h)
>  CFLAGS += -include $(XEN_ROOT)/tools/config.h
>  CFLAGS += -I./include
> @@ -16,36 +15,53 @@ CFLAGS += $(CFLAGS_libxentoolcore)
>  CFLAGS += -DXEN_LIB_STORED="\"$(XEN_LIB_STORED)\""
>  CFLAGS += -DXEN_RUN_STORED="\"$(XEN_RUN_STORED)\""
>  
> -CFLAGS  += $(CFLAGS-y)
> -LDFLAGS += $(LDFLAGS-y)
> +ifdef CONFIG_STUBDOM
> +CFLAGS += -DNO_SOCKETS=1
> +endif
>  
> -CLIENTS := xenstore-exists xenstore-list xenstore-read xenstore-rm 
> xenstore-chmod
> -CLIENTS += xenstore-write xenstore-ls xenstore-watch
> +XENSTORED_OBJS-y := xenstored_core.o xenstored_watch.o xenstored_domain.o
> +XENSTORED_OBJS-y += xenstored_transaction.o xenstored_control.o
> +XENSTORED_OBJS-y += xs_lib.o talloc.o utils.o tdb.o hashtable.o
>  
> -XENSTORED_OBJS = xenstored_core.o xenstored_watch.o xenstored_domain.o
> -XENSTORED_OBJS += xenstored_transaction.o xenstored_control.o
> -XENSTORED_OBJS += xs_lib.o talloc.o utils.o tdb.o hashtable.o
> +XENSTORED_OBJS-$(CONFIG_Linux) += xenstored_posix.o
> +XENSTORED_OBJS-$(CONFIG_SunOS) += xenstored_solaris.o xenstored_posix.o 
> xenstored_probes.o
> +XENSTORED_OBJS-$(CONFIG_NetBSD) += xenstored_posix.o
> +XENSTORED_OBJS-$(CONFIG_FreeBSD) += xenstored_posix.o
> +XENSTORED_OBJS-$(CONFIG_MiniOS) += xenstored_minios.o
>  
> -XENSTORED_OBJS_$(CONFIG_Linux) = xenstored_posix.o
> -XENSTORED_OBJS_$(CONFIG_SunOS) = xenstored_solaris.o xenstored_posix.o 
> xenstored_probes.o
> -XENSTORED_OBJS_$(CONFIG_NetBSD) = xenstored_posix.o
> -XENSTORED_OBJS_$(CONFIG_FreeBSD) = xenstored_posix.o
> -XENSTORED_OBJS_$(CONFIG_MiniOS) = xenstored_minios.o
> +$(XENSTORED_OBJS-y): CFLAGS += $(CFLAGS_libxengnttab)
>  
> -XENSTORED_OBJS += $(XENSTORED_OBJS_y)
> -LDLIBS_xenstored += -lrt
> +xenstored: LDLIBS += $(LDLIBS_libxenevtchn)
> +xenstored: LDLIBS += $(LDLIBS_libxengnttab)
> +xenstored: LDLIBS += $(LDLIBS_libxenctrl)
> +xenstored: LDLIBS += -lrt
> +xenstored: LDLIBS += $(SOCKET_LIBS)
>  
> -ALL_TARGETS = clients
> -ifeq ($(XENSTORE_XENSTORED),y)
> -ALL_TARGETS += xs_tdb_dump xenstored
> +ifeq ($(CONFIG_SYSTEMD),y)
> +$(XENSTORED_OBJS-y): CFLAGS += $(SYSTEMD_CFLAGS)
> +xenstored: LDLIBS += $(SYSTEMD_LIBS)
>  endif
>  
> -ifdef CONFIG_STUBDOM
> -CFLAGS += -DNO_SOCKETS=1
> +xenstore: LDLIBS += $(LDLIBS_libxenstore)
> +xenstore: LDLIBS += $(LDLIBS_libxentoolcore)
> +xenstore: LDLIBS += $(SOCKET_LIBS)
> +
> +xenstore-control: LDLIBS += $(LDLIBS_libxenstore)
> +xenstore-control: LDLIBS += $(LDLIBS_libxenctrl)
> +xenstore-control: LDLIBS += $(LDLIBS_libxenguest)
> +xenstore-control: LDLIBS += $(LDLIBS_libxentoolcore)
> +xenstore-control: LDLIBS += $(SOCKET_LIBS)
> +
> +CLIENTS := xenstore-exists xenstore-list xenstore-read xenstore-rm 
> xenstore-chmod
> +CLIENTS += xenstore-write xenstore-ls xenstore-watch
> +
> +TARGETS := xenstore $(CLIENTS) xenstore-control
> +ifeq ($(XENSTORE_XENSTORED),y)
> +TARGETS += xs_tdb_dump xenstored
>  endif
>  
>  .PHONY: all
> -all: $(ALL_TARGETS)
> +all: $(TARGETS)
>  
>  .PHONY: clients
>  clients: xenstore $(CLIENTS) xenstore-control
> @@ -62,37 +78,28 @@ xenstored_probes.o: xenstored_solaris.o

Urgh - there's a mess in here.

the `dtrace` line has trailing whitespace, but xenstored_probes.d is
going to be clobbered by the dependency file logic.

Given this is utterly broken, perhaps better to drop the CONFIG_SunOS
chunk entirely?

>  CFLAGS += -DHAVE_DTRACE=1
>  endif
>  
> -ifeq ($(CONFIG_SYSTEMD),y)
> -$(XENSTORED_OBJS): CFLAGS += $(SYSTEMD_CFLAGS)
> -xenstored: LDFLAGS += $(SYSTEMD_LIBS)
> -endif
> -
> -$(XENSTORED_OBJS): CFLAGS += $(CFLAGS_libxengnttab)
> -
> -xenstored: $(XENSTORED_OBJS)
> -     $(CC) $^ $(LDFLAGS) $(LDLIBS_libxenevtchn) $(LDLIBS_libxengnttab) 
> $(LDLIBS_libxenctrl) $(LDLIBS_xenstored) $(SOCKET_LIBS) -o $@ 
> $(APPEND_LDFLAGS)
> +xenstored: $(XENSTORED_OBJS-y)
> +     $(CC) $(LDFLAGS) $^ $(LDLIBS) -o $@ $(APPEND_LDFLAGS)
>  
> -xenstored.a: $(XENSTORED_OBJS)
> +xenstored.a: $(XENSTORED_OBJS-y)
>       $(AR) cr $@ $^
>  
>  $(CLIENTS): xenstore
>       ln -f xenstore $@
>  
>  xenstore: xenstore_client.o xs_lib.o
> -     $(CC) $^ $(LDFLAGS) $(LDLIBS_libxenstore) $(LDLIBS_libxentoolcore) 
> $(SOCKET_LIBS) -o $@ $(APPEND_LDFLAGS)
> +     $(CC) $(LDFLAGS) $^ $(LDLIBS) -o $@ $(APPEND_LDFLAGS)
>  
>  xenstore-control: xenstore_control.o
> -     $(CC) $< $(LDFLAGS) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) 
> $(LDLIBS_libxenguest) $(LDLIBS_libxentoolcore) $(SOCKET_LIBS) -o $@ 
> $(APPEND_LDFLAGS)
> +     $(CC) $(LDFLAGS) $^ $(LDLIBS) -o $@ $(APPEND_LDFLAGS)
>  
>  xs_tdb_dump: xs_tdb_dump.o utils.o tdb.o talloc.o
> -     $(CC) $^ $(LDFLAGS) -o $@ $(APPEND_LDFLAGS)
> +     $(CC) $(LDFLAGS) $^ -o $@ $(APPEND_LDFLAGS)
>  
>  .PHONY: clean
>  clean:
>       rm -f *.a *.o xenstored_probes.h

This header file looks wonky to be cleaning.

> -     rm -f xenstored
> -     rm -f xs_tdb_dump xenstore-control init-xenstore-domain
> -     rm -f xenstore $(CLIENTS)
> +     rm -f $(TARGETS)
>       $(RM) $(DEPS_RM)
>  
>  .PHONY: distclean

There is some abuse lower down too.  Another local TAGS, but also a
tarball rule too, and we probably don't want to be using .SECONDARY:
right at the bottom.

~Andrew