Re: [Buildroot] [PATCH 1/1] package/open-iscsi: new package

From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: TIAN Yuanhao <tianyuanhao3@163.com>
Cc: buildroot@buildroot.org
Subject: Re: [Buildroot] [PATCH 1/1] package/open-iscsi: new package
Date: Mon, 26 Sep 2022 18:27:02 +0200	[thread overview]
Message-ID: <20220926182702.3f4d6df8@windsurf> (raw)
In-Reply-To: <20220926154320.35494-1-tianyuanhao3@163.com>

Hello,

On Mon, 26 Sep 2022 08:43:20 -0700
TIAN Yuanhao <tianyuanhao3@163.com> wrote:

> Signed-off-by: TIAN Yuanhao <tianyuanhao3@163.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Thanks for this new patch! See comments below.

> +OPEN_ISCSI_CONF_OPTS = -Ddbroot=/var/lib/iscsi
> +
> +ifeq ($(BR2_PACKAGE_SYSTEMD),y)
> +OPEN_ISCSI_DEPENDENCIES += systemd
> +OPEN_ISCSI_CONF_OPTS += -Dno_systemd=false
> +else
> +OPEN_ISCSI_CONF_OPTS += -Dno_systemd=true
> +endif

Weird option "no_systemd". Why didn't they make that positive logic?

> +
> +ifeq ($(BR2_PACKAGE_OPEN_ISNS),)
> +define OPEN_ISCSI_DISABLE_ISNS
> +	$(SED) "/'isns'/s/^/#/" $(@D)/meson.build
> +	$(SED) "/'iscsid'/s/^/#/" $(@D)/usr/meson.build
> +	$(SED) "/'iscsiadm'/s/^/#/" $(@D)/usr/meson.build
> +endef

Isn't a bit annoying to have this kind of mess. Could you instead add
an option in meson.build to enable/disable isns support, and contribute
this improvement upstream?

> +OPEN_ISCSI_PRE_CONFIGURE_HOOKS += OPEN_ISCSI_DISABLE_ISNS
> +else
> +OPEN_ISCSI_DEPENDENCIES += open-isns
> +endif
> +
> +define OPEN_ISCSI_INSTALL_TARGET_CMDS
> +	$(INSTALL) -d -m 755 $(TARGET_DIR)/usr/lib
> +	cp -dpf $(@D)/build/libopeniscsiusr.so{,.0,.0.2.0} \
> +		$(TARGET_DIR)/usr/lib/
> +	$(OPEN_ISCSI_INSTALL_ISCSID)
> +	$(OPEN_ISCSI_INSTALL_ISCSISTART)
> +endef
> +
> +ifeq ($(BR2_PACKAGE_OPEN_ISCSI_ISCSID),y)
> +define OPEN_ISCSI_INSTALL_ISCSID
> +	$(INSTALL) -d -m 755 $(TARGET_DIR)/usr/sbin
> +	$(INSTALL) -m 755 -t $(TARGET_DIR)/usr/sbin \
> +		$(@D)/build/{iscsi-iname,iscsiadm,iscsid}
> +	$(INSTALL) -D -m 644 {$(@D)/etc,$(TARGET_DIR)/etc/iscsi}/iscsid.conf
> +endef
> +
> +define OPEN_ISCSI_INSTALL_INIT_SYSTEMD
> +	$(INSTALL) -d -m 755 $(TARGET_DIR)/usr/lib/systemd/system
> +	$(INSTALL) -m 644 -t $(TARGET_DIR)/usr/lib/systemd/system \
> +		$(@D)/build/{iscsi,iscsi-init,iscsid}.service \
> +		$(@D)/etc/systemd/iscsid.socket

Why isn't that installed automatically by the meson build system when
systemd support is enabled?

> +endef
> +endif
> +
> +ifeq ($(BR2_PACKAGE_OPEN_ISCSI_ISCSISTART),y)
> +define OPEN_ISCSI_INSTALL_ISCSISTART
> +	$(INSTALL) -D -m 755 {$(@D)/build,$(TARGET_DIR)/usr/sbin}/iscsistart
> +endef
> +endif

It's also a bit annoying that you have to reimplement all the
installation logic. What about improving the meson.build with
additional options, and contribute these improvements upstream?

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot