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

From: Juergen Gross <jgross@suse.com>
To: Andrew Cooper <amc96@srcf.net>,
	Anthony PERARD <anthony.perard@citrix.com>,
	xen-devel@lists.xenproject.org
Cc: Anthony PERARD <anthony.perard@gmail.com>,
	Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>,
	Julien Grall <julien@xen.org>
Subject: Re: [XEN PATCH 36/57] tools/xenstore: Cleanup makefile
Date: Fri, 17 Dec 2021 06:24:52 +0100	[thread overview]
Message-ID: <ed48b603-b484-4544-f12a-1e816b6c007d@suse.com> (raw)
In-Reply-To: <42048d21-c02e-9fea-b228-7ffc60009be9@srcf.net>

[-- Attachment #1.1.1: Type: text/plain, Size: 5704 bytes --]

On 16.12.21 19:34, Andrew Cooper wrote:
> 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@citrix.com>
>> ---
>>   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 ?

Yes, you are right. They should be dropped.

> 
> 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?

I've basically asking to do so since 5 years now. Last try wast last
year and the DilOS reply was:

   we have plans for XEN on 2021 year, but all depends on business needs
   and investments."

So the same as in 2016 (not there yet, but planning to do so):

   i have plans try to return back and look at latest Xen.

At least running xenstored in SunOS has been broken since years now, and
I'd like to suggest removing the CONFIG_SunOS parts from it (again).

Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]