[Buildroot] [PATCH v2] package/zabbix: new package

Arnout Vandecappelle arnout at mind.be
Sun May 5 21:51:02 UTC 2019


 Hi Alexey,

On 28/04/2019 13:12, Alexey Lukyanchuk wrote:
> Signed-off-by: Alexey Lukyanchuk <skif at skif-web.ru>

 When you send a second (or later) iteration of a patch, please add a patch
changelog below your SoB, separated by ---, like this:

---
v2:
- Added DEVELOPERS entry (Thomas)
- ...

 It is also useful to add the reviewers of the previous version in Cc
explicitly, to gather their updated feedback (or Acks).

[snip]
> diff --git a/package/zabbix/0001-fix-netsnmp-path.patch b/package/zabbix/0001-fix-netsnmp-path.patch
> new file mode 100644
> index 0000000000..235bec3c18
> --- /dev/null
> +++ b/package/zabbix/0001-fix-netsnmp-path.patch
> @@ -0,0 +1,16 @@
> +Signed-off-by: Alexey Lukyanchuk <skif at skif-web.ru>

 A patch requires a proper commit message that explain why it is needed and what
the chosen solution is. 'fix netsnmp' is not enough.

 We usually prefer git-formatted patches (i.e. generated by git format-patch),
but since upstream doesn't provide a git repo, it's not needed in this case. We
also prefer patches to be upstreamable, but upstream doesn't give any indication
that they accept patches...

> +
> +--- a/m4/netsnmp.m4	2019-04-28 12:13:09.978661351 +0300
> ++++ b/m4/netsnmp.m4	2019-04-28 12:13:16.218598128 +0300
> +@@ -88,7 +88,10 @@
> + 				esac
> + 			done
> + 		fi
> +-
> ++	

 You're introducing a whitespace error here.

> ++  SNMP_CFLAGS="-I. -I$STAGING_DIR/usr/bin/../../usr/include"

 Why is this needed? This is just the default search path...

> ++  SNMP_LDFLAGS="-L$STAGING_DIR/usr/lib"

 Same here...

> ++  SNMP_LIBS="-lnetsnmp"
> + 		_save_netsnmp_cflags="$CFLAGS"
> + 		_save_netsnmp_ldflags="$LDFLAGS"
> + 		_save_netsnmp_libs="$LIBS"
> diff --git a/package/zabbix/Config.in b/package/zabbix/Config.in
> new file mode 100644
> index 0000000000..ca6ab56468
> --- /dev/null
> +++ b/package/zabbix/Config.in
> @@ -0,0 +1,51 @@
> +config BR2_PACKAGE_ZABBIX
> +	bool "zabbix"
> +	select BR2_PACKAGE_LIBCURL
> +	select BR2_PACKAGE_LIBEVENT
> +	select BR2_PACKAGE_PCRE

 I don't think it makes sense to build zabbix without either server or client,
right? In that case, we do:

	select BR2_PACKAGE_ZABBIX_CLIENT if !BR2_PACKAGE_ZABBIX_SERVER

> +	help
> +	  zabbix monitoring system
> +
> +if BR2_PACKAGE_ZABBIX

 Add an empty line here.

> +comment "zabbix server need nls support"

 This should only be shown if NLS is not available, but it is available in
general on the architecture, i.e. you should add

	depends on !BR2_BINFMT_FLAT
	depends on BR2_USE_MMU
	depends on !BR2_SYSTEM_ENABLE_NLS

> +
> +config BR2_PACKAGE_ZABBIX_SERVER
> +	bool "zabbix server"
> +	depends on !BR2_BINFMT_FLAT

 You should add where this dependency comes from, i.e.

	depends on !BR2_BINFMT_FLAT # php
	depends on BR2_USE_MMU # netsnmp

(note that you missed that dependency; I think test-pkg would have discovered
that, did you run test-pkg?)

> +	depends on BR2_SYSTEM_ENABLE_NLS
> +	select BR2_PACKAGE_PHP

 Is this an actual build time dependency?

> +	select BR2_PACKAGE_PHP_EXT_CTYPE

 These, for sure, are not. By convention, we add a comment in that case (to each
of them):

	select BR2_PACKAGE_PHP_EXT_CTYPE # runtime


> +	select BR2_PACKAGE_PHP_EXT_GETTEXT
> +	select BR2_PACKAGE_PHP_EXT_BCMATH
> +	select BR2_PACKAGE_PHP_EXT_MBSTRING
> +	select BR2_PACKAGE_PHP_EXT_SOCKETS
> +	select BR2_PACKAGE_PHP_EXT_MYSQLI
> +	select BR2_PACKAGE_PHP_EXT_PDO_MYSQL

 Mysql is needed even if postgres is chosen as backend?

> +	select BR2_PACKAGE_PHP_EXT_PDO
> +	select BR2_PACKAGE_PHP_EXT_GD
> +	select BR2_PACKAGE_PHP_EXT_LIBXML2
> +	select BR2_PACKAGE_PHP_EXT_XMLREADER
> +	select BR2_PACKAGE_PHP_EXT_XMLWRITER
> +	select BR2_PACKAGE_PHP_EXT_SESSION
> +	select BR2_PACKAGE_NETSNMP
> +	select BR2_PACKAGE_LIBXML2

 libxml2 is needed both for build and through PHP?

 Finally, please keep all dependencies alphabetically ordered.


> +	help
> +	  Zabbix monitoring server
> +
> +if BR2_PACKAGE_ZABBIX_SERVER
> +choice
> +	prompt "zabbix server database backend"
> +	default BR2_PACKAGE_ZABBIX_SERVER_MYSQL
> +config BR2_PACKAGE_ZABBIX_SERVER_MYSQL
> +	bool "mysql"
> +	depends on BR2_PACKAGE_MYSQL

 This should probably be select, not depends. Also, I wonder if it is really
needed. The database is not necessarily running on the same machine, so
typically you need just a database *connection*, and if you're using PHP, you'd
typically use PDO for that, not mysql (client) itself.

 If you really do need the mysql or postgresql client, then the corresponding
dependencies have to be added to the BR2_PACKAGE_ZABBIX_SERVER.

> +config BR2_PACKAGE_ZABBIX_SERVER_POSTGRESQL
> +	bool "posgresql"
> +	depends on BR2_PACKAGE_POSTGRESQL
> +endchoice
> +endif
> +
> +config BR2_PACKAGE_ZABBIX_CLIENT
> +	bool "zabbix client"

 Shouldn't this be called agent instead of client?

> +	default n

 Not needed.

> +endif
> diff --git a/package/zabbix/zabbix-agent.service b/package/zabbix/zabbix-agent.service
> new file mode 100644
> index 0000000000..31c8f81798
> --- /dev/null
> +++ b/package/zabbix/zabbix-agent.service
> @@ -0,0 +1,13 @@
> +Description=Zabbix Agent Daemon
> +After=syslog.target network.target
> + 
> +[Service]
> +Type=forking
> +ExecStart=/usr/sbin/zabbix_agentd
> +ExecReload=/usr/local/sbin/zabbix_agentd -R config_cache_reload
> +PIDFile=/tmp/zabbix_agentd.pid

 PIDFiles are normally put in /run... But I guess this is where the default
zabbix config puts it?


> +User=zabbix
> +Group=zabbix
> + 
> +[Install]
> +WantedBy=multi-user.target
> \ No newline at end of file

 Please add a newline.

> diff --git a/package/zabbix/zabbix-server.service b/package/zabbix/zabbix-server.service
> new file mode 100644
> index 0000000000..30a906e551
> --- /dev/null
> +++ b/package/zabbix/zabbix-server.service
> @@ -0,0 +1,14 @@
> +Description=Zabbix Server Daemon
> +Requires=
> +After=syslog.target network.target
> + 
> +[Service]
> +Type=forking
> +ExecStart=/usr/sbin/zabbix_server
> +ExecReload=/usr/sbin/zabbix_server -R config_cache_reload
> +PIDFile=/tmp/zabbix_server.pid
> +User=zabbix
> +Group=zabbix
> + 
> +[Install]
> +WantedBy=multi-user.target
> \ No newline at end of file

 Newline.

> diff --git a/package/zabbix/zabbix.hash b/package/zabbix/zabbix.hash
> new file mode 100644
> index 0000000000..56b1bf88f0
> --- /dev/null
> +++ b/package/zabbix/zabbix.hash
> @@ -0,0 +1 @@
> +sha256	4915d52352163e40398ca9b56b8176ea369dfd475e291087c50a3d9ae2459076	zabbix-4.2.1.tar.gz
> diff --git a/package/zabbix/zabbix.mk b/package/zabbix/zabbix.mk
> new file mode 100644
> index 0000000000..9da48514d9
> --- /dev/null
> +++ b/package/zabbix/zabbix.mk
> @@ -0,0 +1,78 @@
> +################################################################################
> +#
> +#zabbix
> +#
> +################################################################################
> +
> +ZABBIX_VERSION = 4.2.1
> +ZABBIX_SITE = https://sourceforge.net/projects/zabbix/files
> +
> +ZABBIX_INSTALL_STAGING = YES

 So, it installs a library as well?

 In that case, maybe it *does* make sense to install without client or server?


> +ZABBIX_DEPENDENCIES = pcre libcurl libevent netsnmp libxml2

 netsnmp and libxml2 are (in Config.in) only selected for the server.


> +ZABBIX_CONF_OPTS = --with-libcurl --with-libevent --with-libxml2 --with-net-snmp=$(STAGING_DIR)/usr/bin/

 I haven't checked the configure script, but usually things just work when you
do --with-net-snmp.

> +
> +ifeq ($(BR2_INIT_SYSTEMD),y)
> +ifeq ($(BR2_PACKAGE_ZABBIX_CLIENT),y)
> +ZABBIX_SYSTEMD_UNITS += zabbix-agent.service
> +endif
> +ifeq ($(BR2_PACKAGE_ZABBIX_SERVER),y)
> +ZABBIX_SYSTEMD_UNITS += zabbix-server.service
> +endif
> +ZABBIX_POST_INSTALL_TARGET_HOOKS += ZABBIX_INSTALL_INIT_SYSTEMD

 This isn't needed: the ZABBIX_INSTALL_INIT_SYSTEMD commands will be called
automatically when systemd is enabled. Because of this, also the outer systemd
condition is not needed.

> +endif
> +
> +ifeq ($(BR2_PACKAGE_ZABBIX_SERVER),y)
> +ZABBIX_CONF_OPTS += --enable-server
> +ZABBIX_DEPENDENCIES += php
> +ZABBIX_POST_INSTALL_TARGET_HOOKS += ZABBIX_SERVER_COPY_FRONTEND

 We normally put the definitions of variables before their use, so move the
COPY_FRONTEND definition higher up. Also, we usually keep it withint the same
condition. So it would go after the update of CONF_OPTS and DEPENDENCIES, and
before the update of POST_INSTALL_TARGET_HOOKS.

> +
> +ifeq ($(BR2_PACKAGE_ZABBIX_SERVER_MYSQL),y)
> +ZABBIX_DEPENDENCIES += mysql
> +ZABBIX_CONF_OPTS += --with-mysql=$(STAGING_DIR)/usr/bin/mysql_config
> +ZABBIX_POST_INSTALL_TARGET_HOOKS += ZABBIX_SERVER_PREPARE_MYSQL
> +endif
> +
> +ifeq ($(BR2_PACKAGE_ZABBIX_SERVER_POSTGRESQL),y)
> +ZABBIX_DEPENDENCIES += postgresql
> +ZABBIX_CONF_OPTS += --with-postgresql=$(STAGING_DIR)/usr/bin/pg_config
> +ZABBIX_POST_INSTALL_TARGET_HOOKS += ZABBIX_SERVER_PREPARE_POSTGRESQL
> +endif
> +
> +endif
> +
> +ifeq ($(BR2_PACKAGE_ZABBIX_CLIENT),y)
> +ZABBIX_CONF_OPTS += --enable-agent
> +endif
> +
> +define ZABBIX_SERVER_COPY_FRONTEND
> +	mkdir -p $(TARGET_DIR)/usr/zabbix/php-frontend/
> +	cp -r $(@D)/frontends/php/* $(TARGET_DIR)/usr/zabbix/php-frontend/

 I suppose the PHP frontend is supposed to be exposed by a web server? Document
that in the help text, including the path on target where it resides (i.e.
/usr/zabbix/php-frontend, I suppose).

> +endef
> +
> +define ZABBIX_SERVER_PREPARE_POSTGRESQL
> +	mkdir -p $(TARGET_DIR)/usr/zabbix/postgresql_schema
> +	cp -r $(@D)/database/postgresql/*\.sql $(TARGET_DIR)/usr/zabbix/postgresql_schema/

 Does zabbix know automatically which server to connect to (and which schema to
use)?

 I guess the database must be initialised somehow as well?

> +	sed -i '/^Requires=/ s/$$/postgresql.service/' $(TARGET_DIR)/usr/lib/systemd/system/zabbix-server.service
> +	sed -i '/^After=/ s/$$/ postgresql.service/' $(TARGET_DIR)/usr/lib/systemd/system/zabbix-server.service

 Hm, indeed, the most common case would be that you run the database on the same
device, and in that case you'd need this dependency.

> +endef
> +
> +define ZABBIX_SERVER_PREPARE_MYSQL
> +	mkdir -p $(TARGET_DIR)/usr/zabbix/mysql_schema/
> +	cp -r $(@D)/database/mysql/*\.sql $(TARGET_DIR)/usr/zabbix/mysql_schema/
> +	sed -i '/^Requires=/ s/$$/ mysql.service/' $(TARGET_DIR)/usr/lib/systemd/system/zabbix-server.service
> +	sed -i '/^After=/ s/$$/ mysql.service/' $(TARGET_DIR)/usr/lib/systemd/system/zabbix-server.service
> +endef
> +
> +define ZABBIX_INSTALL_INIT_SYSTEMD
> +	$(foreach unit,$(ZABBIX_SYSTEMD_UNITS),\

 Very elegant solution, constructing SYSTEMD_UNITS and then using foreach.

> +		echo UNIT=$(unit) && \

	I don't think this echo is useful, since all commands are printed anyway.

> +		$(INSTALL) -D -m 644 package/zabbix/$(unit) $(TARGET_DIR)/usr/lib/systemd/system/$(unit) && \

 I think it's a little more elegant by doing it with two $(foreach) calls, i.e.

	$(foreach unit,$(ZABBIX_SYSTEMD_UNITS),\
		$(INSTALL) -D -m 644 package/zabbix/$(unit)
$(TARGET_DIR)/usr/lib/systemd/system/$(unit)
	mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants
	$(foreach unit,$(ZABBIX_SYSTEMD_UNITS),\
		ln -fs ../../../../usr/lib/systemd/system/$(unit)
$(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/$(unit)


 Regards,
 Arnout

> +		mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants && \
> +		ln -fs ../../../../usr/lib/systemd/system/$(unit) $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/$(unit) ;)
> +endef
> +
> +define ZABBIX_USERS
> +	zabbix -1 zabbix -1 !- /var/lib/zabbix - zabbix zabbix user
> +endef
> +
> +$(eval $(autotools-package))
> 


More information about the buildroot mailing list