[Buildroot] [PATCH v4 1/1] New package: openvmtools

[Buildroot] [PATCH v4 1/1] New package: openvmtools

[Karoly Kasza]

New package: openvmtools

Signed-off-by: Karoly Kasza <kaszak@gmail.com>
---

Changes v3 -> v4:
  - Added patch to compile with uClibc and Buildroot toolchain

 package/Config.in                                  |    1 +
 package/openvmtools/Config.in                      |   56 ++++++++++++++
 package/openvmtools/S10vmtoolsd                    |   33 ++++++++
 .../openvmtools-01-has_bsd_printf.patch            |   26 +++++++
 package/openvmtools/openvmtools.mk                 |   81 ++++++++++++++++++++
 package/openvmtools/shutdown                       |    7 ++
 package/openvmtools/vmtoolsd.service               |    8 ++
 7 files changed, 212 insertions(+)
 create mode 100644 package/openvmtools/Config.in
 create mode 100644 package/openvmtools/S10vmtoolsd
 create mode 100644 package/openvmtools/openvmtools-01-has_bsd_printf.patch
 create mode 100644 package/openvmtools/openvmtools.mk
 create mode 100644 package/openvmtools/shutdown
 create mode 100644 package/openvmtools/vmtoolsd.service

diff --git a/package/Config.in b/package/Config.in
index 11087da..c5167ee 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -1164,6 +1164,7 @@ endif
 	source "package/ncdu/Config.in"
 	source "package/numactl/Config.in"
 	source "package/nut/Config.in"
+	source "package/openvmtools/Config.in"
 	source "package/powerpc-utils/Config.in"
 	source "package/polkit/Config.in"
 if BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
diff --git a/package/openvmtools/Config.in b/package/openvmtools/Config.in
new file mode 100644
index 0000000..b1ede00
--- /dev/null
+++ b/package/openvmtools/Config.in
@@ -0,0 +1,56 @@
+config BR2_PACKAGE_OPENVMTOOLS
+	bool "openvmtools"
+	depends on BR2_i386 || BR2_x86_64
+	depends on BR2_USE_MMU
+	depends on BR2_USE_WCHAR
+	depends on BR2_TOOLCHAIN_HAS_THREADS
+	depends on BR2_TOOLCHAIN_HAS_NATIVE_RPC
+	depends on BR2_LARGEFILE
+	depends on BR2_ENABLE_LOCALE
+	select BR2_PACKAGE_LIBGLIB2
+	help
+	  Open Virtual Machine Tools for VMware guest OS
+
+	  http://open-vm-tools.sourceforge.net/
+
+	  ICU locales and X11 tools are currently not supported.
+
+	  NOTE: Support for vmblock-fuse will be enabled in openvmtools if the
+	        libfuse package is selected.
+
+if BR2_PACKAGE_OPENVMTOOLS
+
+menu "openvmtools options"
+
+config BR2_PACKAGE_OPENVMTOOLS_PROCPS
+	bool "openvmtools procps support"
+	select BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
+	select BR2_PACKAGE_PROCPS_NG
+	help
+	  Enable support for procps / meminfo
+
+config BR2_PACKAGE_OPENVMTOOLS_DNET
+	bool "openvmtools dnet support"
+	depends on BR2_INET_IPV6
+	select BR2_PACKAGE_LIBDNET
+	help
+	  Enable support for libdnet / nicinfo
+
+comment "openvmtools dnet support needs a toolchain w/ IPv6"
+	depends on !BR2_INET_IPV6
+
+config BR2_PACKAGE_OPENVMTOOLS_PAM
+	bool "openvmtools PAM support"
+	select BR2_PACKAGE_LINUX_PAM
+	help
+	  Support for PAM in openvmtools
+
+endmenu
+
+endif
+
+comment "openvmtools needs a toolchain w/ wchar, threads, RPC, largefile, locale"
+	depends on BR2_i386 || BR2_x86_64
+	depends on BR2_USE_MMU
+	depends on !BR2_USE_WCHAR ||!BR2_TOOLCHAIN_HAS_THREADS || \
+	!BR2_TOOLCHAIN_HAS_NATIVE_RPC || !BR2_LARGEFILE || !BR2_ENABLE_LOCALE
diff --git a/package/openvmtools/S10vmtoolsd b/package/openvmtools/S10vmtoolsd
new file mode 100644
index 0000000..3ab351e
--- /dev/null
+++ b/package/openvmtools/S10vmtoolsd
@@ -0,0 +1,33 @@
+#!/bin/sh
+#
+# Starts vmtoolsd for openvmtools
+#
+
+PID=/var/run/vmtoolsd.pid
+
+case "$1" in
+  start)
+	echo -n "Starting vmtoolsd: "
+	/usr/bin/vmtoolsd -b $PID
+	if [ $? != 0 ]; then
+		echo "FAILED"
+		exit 1
+	else
+		echo "OK"
+	fi
+	;;
+  stop)
+	echo -n "Stopping vmtoolsd: "
+	kill `cat $PID`
+	echo "OK"
+	;;
+  restart|reload)
+	$0 stop
+	$0 start
+	;;
+  *)
+	echo "Usage: $0 {start|stop|restart}"
+	exit 1
+esac
+
+exit $?
diff --git a/package/openvmtools/openvmtools-01-has_bsd_printf.patch b/package/openvmtools/openvmtools-01-has_bsd_printf.patch
new file mode 100644
index 0000000..889f7d1
--- /dev/null
+++ b/package/openvmtools/openvmtools-01-has_bsd_printf.patch
@@ -0,0 +1,26 @@
+lib/misc/msgList.c: missing #ifdef
+
+This macro checks for BSD style printf(), which is not present
+when compiling for uClibc. The linked functions are unnecessary in
+this case, and they break compilation.
+
+Signed-off-by: Karoly Kasza <kaszak@gmail.com>
+
+--- open-vm-tools-9.4.6-1770165.orig/lib/misc/msgList.c	2014-07-02 00:21:14.000000000 +0200
++++ open-vm-tools-9.4.6-1770165/lib/misc/msgList.c	2014-07-29 13:40:40.000000000 +0200
+@@ -487,6 +487,7 @@
+    return messages->id;
+ }
+ 
++#ifdef HAS_BSD_PRINTF
+ 
+ /*
+  *----------------------------------------------------------------------
+@@ -566,6 +567,7 @@
+    }
+ }
+ 
++#endif
+ 
+ /*
+  *----------------------------------------------------------------------
diff --git a/package/openvmtools/openvmtools.mk b/package/openvmtools/openvmtools.mk
new file mode 100644
index 0000000..9771a1e
--- /dev/null
+++ b/package/openvmtools/openvmtools.mk
@@ -0,0 +1,81 @@
+################################################################################
+#
+# openvmtools
+#
+################################################################################
+
+OPENVMTOOLS_VERSION = 9.4.6-1770165
+OPENVMTOOLS_SOURCE = open-vm-tools-$(OPENVMTOOLS_VERSION).tar.gz
+OPENVMTOOLS_SITE = http://downloads.sourceforge.net/project/open-vm-tools/open-vm-tools/stable-9.4.x
+OPENVMTOOLS_LICENSE = LGPLv2.1
+OPENVMTOOLS_LICENSE_FILES = COPYING
+OPENVMTOOLS_AUTORECONF = YES
+OPENVMTOOLS_CONF_OPT = \
+        --without-icu \
+        --without-x \
+        --without-gtk2 \
+        --without-gtkmm \
+        --without-kernel-modules
+
+#-Wno-deprecated-declarations is a workaround for a bug in open-vm-tools
+#See http://sourceforge.net/p/open-vm-tools/mailman/message/31473171/
+OPENVMTOOLS_CONF_ENV = CFLAGS+="-Wno-deprecated-declarations"
+
+OPENVMTOOLS_DEPENDENCIES = libglib2
+
+# When libfuse is available, openvmtools can build vmblock-fuse, so
+# make sure that libfuse gets built first.
+ifeq ($(BR2_PACKAGE_LIBFUSE),y)
+OPENVMTOOLS_DEPENDENCIES += libfuse
+endif
+
+ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PROCPS),y)
+OPENVMTOOLS_CONF_ENV += CUSTOM_PROCPS_NAME=procps LDFLAGS="-L$(TARGET_DIR)/usr/lib"
+OPENVMTOOLS_CONF_OPT += --with-procps
+OPENVMTOOLS_DEPENDENCIES += procps-ng
+else
+OPENVMTOOLS_CONF_OPT += --without-procps
+endif
+
+ifeq ($(BR2_PACKAGE_OPENVMTOOLS_DNET),y)
+OPENVMTOOLS_CONF_ENV += CUSTOM_DNET_CPPFLAGS="-I$(STAGING_DIR)/usr/include"
+OPENVMTOOLS_CONF_OPT += --with-dnet
+OPENVMTOOLS_DEPENDENCIES += libdnet
+else
+OPENVMTOOLS_CONF_OPT += --without-dnet
+endif
+
+ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PAM),y)
+OPENVMTOOLS_CONF_OPT += --with-pam
+OPENVMTOOLS_MAKE_OPT += CFLAGS+="-Wno-unused-local-typedefs"
+OPENVMTOOLS_DEPENDENCIES += linux-pam
+else
+OPENVMTOOLS_CONF_OPT += --without-pam
+endif
+
+define OPENVMTOOLS_POST_INSTALL_TARGET_THINGIES
+#needed by lib/system/systemLinux.c (or will cry in /var/log/messages)
+	ln -fs os-release $(TARGET_DIR)/etc/lfs-release \
+#for Guest OS restart/shutdown
+	if [ ! -e $(TARGET_DIR)/sbin/shutdown ]; then \
+		$(INSTALL) -D -m 755 package/openvmtools/shutdown \
+			$(TARGET_DIR)/sbin/shutdown; \
+	fi
+endef
+
+OPENVMTOOLS_POST_INSTALL_TARGET_HOOKS += OPENVMTOOLS_POST_INSTALL_TARGET_THINGIES
+
+define OPENVMTOOLS_INSTALL_INIT_SYSV
+	$(INSTALL) -D -m 755 package/openvmtools/S10vmtoolsd \
+		$(TARGET_DIR)/etc/init.d/S10vmtoolsd
+endef
+
+define OPENVMTOOLS_INSTALL_INIT_SYSTEMD
+	$(INSTALL) -D -m 644 package/openvmtools/vmtoolsd.service \
+		$(TARGET_DIR)/etc/systemd/system/vmtoolsd.service
+	mkdir -p $(TARGET_DIR)/etc/systemd/system/multi-user.target.wants
+	ln -fs ../vmtoolsd.service \
+		$(TARGET_DIR)/etc/systemd/system/multi-user.target.wants/vmtoolsd.service
+endef
+
+$(eval $(autotools-package))
diff --git a/package/openvmtools/shutdown b/package/openvmtools/shutdown
new file mode 100644
index 0000000..bca9765
--- /dev/null
+++ b/package/openvmtools/shutdown
@@ -0,0 +1,7 @@
+#!/bin/sh
+#compatibility script for openvmtools
+if [ "$1" == "-r" ]; then
+/sbin/reboot
+else
+/sbin/poweroff
+fi
diff --git a/package/openvmtools/vmtoolsd.service b/package/openvmtools/vmtoolsd.service
new file mode 100644
index 0000000..b099ba3
--- /dev/null
+++ b/package/openvmtools/vmtoolsd.service
@@ -0,0 +1,8 @@
+[Unit]
+Description=vmtoolsd for openvmtools
+
+[Service]
+ExecStart=/usr/bin/vmtoolsd -b /var/run/vmtoolsd.pid
+
+[Install]
+WantedBy=multi-user.target
-- 
1.7.10.4

[Buildroot] [PATCH v4 1/1] New package: openvmtools

[Yann E. MORIN]

Karoly, All,

On 2014-07-29 13:57 +0200, Karoly Kasza spake thusly:
> New package: openvmtools

Thanks for this new package!

See below for a few comments.

> Signed-off-by: Karoly Kasza <kaszak@gmail.com>
[--SNIP--]
> diff --git a/package/openvmtools/Config.in b/package/openvmtools/Config.in
> new file mode 100644
> index 0000000..b1ede00
> --- /dev/null
> +++ b/package/openvmtools/Config.in
> @@ -0,0 +1,56 @@
> +config BR2_PACKAGE_OPENVMTOOLS
> +	bool "openvmtools"
> +	depends on BR2_i386 || BR2_x86_64
> +	depends on BR2_USE_MMU
> +	depends on BR2_USE_WCHAR
> +	depends on BR2_TOOLCHAIN_HAS_THREADS

Are all those dependencies just inherited from libglib2, or are they
real dependencies on openvmtools?

If they are only inherited from libglib2, they it is customary to
indicate the package as a comment to the dependency, like:

    depends on BR2_USE_MMU # libglib2
    depends on BR2_USE_WCHAR # libglib2

> +	depends on BR2_TOOLCHAIN_HAS_NATIVE_RPC
> +	depends on BR2_LARGEFILE
> +	depends on BR2_ENABLE_LOCALE
> +	select BR2_PACKAGE_LIBGLIB2
> +	help
> +	  Open Virtual Machine Tools for VMware guest OS
> +
> +	  http://open-vm-tools.sourceforge.net/
> +
> +	  ICU locales and X11 tools are currently not supported.
> +
> +	  NOTE: Support for vmblock-fuse will be enabled in openvmtools if the
> +	        libfuse package is selected.
> +
> +if BR2_PACKAGE_OPENVMTOOLS
> +
> +menu "openvmtools options"

Do not put it in a sub-menu, the options will be properly indented in
the frontends.

We only do sub-menus when there are a *lot* of sub-options (there is no
written rule on this, but above ~8 warants a sub-menu, otherwise
indentation is considered enough.)

> +config BR2_PACKAGE_OPENVMTOOLS_PROCPS
> +	bool "openvmtools procps support"

Do not repeat 'openvmtools' in the prompts. Since it depends on
BR2_PACKAGE_OPENVMTOOLS, the frontends wil properly indent the
sub-options, so that it is obvious they are options to openvmtools.

> +	select BR2_PACKAGE_BUSYBOX_SHOW_OTHERS

I know we alkready have one package and one systm option that select
busybox-show-others, but they are a bit special: one is systemd, the
other is sysvinit. All other packages depend on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
instead.

For openvmtools, I would use a depends rather than a select, something
like:

    config BR2_PACKAGE_OPENVMTOOLS_PROCPS
        bool "procps support"
        depends on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
        select BR2_PACKAGE_PROCPS_NG

    comment "procps support needs that BUSYBOX_SHOW_OTHERS be set"
        depends on !BR2_PACKAGE_BUSYBOX_SHOW_OTHERS

I'm not much satisfied with the comment text, however. If you find a
better wording, feel free to adapt.

> +	select BR2_PACKAGE_PROCPS_NG
> +	help
> +	  Enable support for procps / meminfo
> +
> +config BR2_PACKAGE_OPENVMTOOLS_DNET
> +	bool "openvmtools dnet support"

Ditto: do not repeat 'openvmtools' in the prompts.

> +	depends on BR2_INET_IPV6
> +	select BR2_PACKAGE_LIBDNET
> +	help
> +	  Enable support for libdnet / nicinfo
> +
> +comment "openvmtools dnet support needs a toolchain w/ IPv6"
> +	depends on !BR2_INET_IPV6
> +
> +config BR2_PACKAGE_OPENVMTOOLS_PAM
> +	bool "openvmtools PAM support"

Ditto.

> +	select BR2_PACKAGE_LINUX_PAM
> +	help
> +	  Support for PAM in openvmtools
> +
> +endmenu
> +
> +endif
> +
> +comment "openvmtools needs a toolchain w/ wchar, threads, RPC, largefile, locale"
> +	depends on BR2_i386 || BR2_x86_64
> +	depends on BR2_USE_MMU
> +	depends on !BR2_USE_WCHAR ||!BR2_TOOLCHAIN_HAS_THREADS || \
> +	!BR2_TOOLCHAIN_HAS_NATIVE_RPC || !BR2_LARGEFILE || !BR2_ENABLE_LOCALE
> diff --git a/package/openvmtools/S10vmtoolsd b/package/openvmtools/S10vmtoolsd
> new file mode 100644
> index 0000000..3ab351e
> --- /dev/null
> +++ b/package/openvmtools/S10vmtoolsd
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +#
> +# Starts vmtoolsd for openvmtools
> +#
> +
> +PID=/var/run/vmtoolsd.pid
> +
> +case "$1" in
> +  start)
> +	echo -n "Starting vmtoolsd: "

This scripts uses a mix of tabs and spaces to do the indentation; for
example, the cases lines ("start)", "stop)"...) are indented with
spaces, while the rest is indented with tabs. Please be consistent.

> +
> +exit $?

No need to exit epxlicitly, it's implicit from POSIX, that a shell
script exits with the error code from the last command. Besides, we do
not care at all about the exit status of a startup script.

[--SNIP--]
> diff --git a/package/openvmtools/openvmtools.mk b/package/openvmtools/openvmtools.mk
> new file mode 100644
> index 0000000..9771a1e
> --- /dev/null
> +++ b/package/openvmtools/openvmtools.mk
> @@ -0,0 +1,81 @@
> +################################################################################
> +#
> +# openvmtools
> +#
> +################################################################################
> +
> +OPENVMTOOLS_VERSION = 9.4.6-1770165
> +OPENVMTOOLS_SOURCE = open-vm-tools-$(OPENVMTOOLS_VERSION).tar.gz
> +OPENVMTOOLS_SITE = http://downloads.sourceforge.net/project/open-vm-tools/open-vm-tools/stable-9.4.x
> +OPENVMTOOLS_LICENSE = LGPLv2.1
> +OPENVMTOOLS_LICENSE_FILES = COPYING
> +OPENVMTOOLS_AUTORECONF = YES
> +OPENVMTOOLS_CONF_OPT = \
> +        --without-icu \
> +        --without-x \
> +        --without-gtk2 \
> +        --without-gtkmm \
> +        --without-kernel-modules

On a single line. If it is too long, just split it, like:

    OPENVMTOOLS_CONF_OPT = --without-icu --without-x \
            --without-gtk2 --without-gtkmm --without-kernel-modules

> +#-Wno-deprecated-declarations is a workaround for a bug in open-vm-tools
> +#See http://sourceforge.net/p/open-vm-tools/mailman/message/31473171/

Nit-pick: space after the '#' symbol.

> +OPENVMTOOLS_CONF_ENV = CFLAGS+="-Wno-deprecated-declarations"

This should be:

    OPENVMTOOLS_CONF_ENV = CFLAGS="$(TARGET_CFLAGS) -Wno-deprecated-declarations"

> +OPENVMTOOLS_DEPENDENCIES = libglib2
> +
> +# When libfuse is available, openvmtools can build vmblock-fuse, so
> +# make sure that libfuse gets built first.
> +ifeq ($(BR2_PACKAGE_LIBFUSE),y)
> +OPENVMTOOLS_DEPENDENCIES += libfuse

No --enable-fuse/--disable-fuse options (or the likes)?

> +endif
> +
> +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PROCPS),y)
> +OPENVMTOOLS_CONF_ENV += CUSTOM_PROCPS_NAME=procps LDFLAGS="-L$(TARGET_DIR)/usr/lib"
> +OPENVMTOOLS_CONF_OPT += --with-procps
> +OPENVMTOOLS_DEPENDENCIES += procps-ng
> +else
> +OPENVMTOOLS_CONF_OPT += --without-procps
> +endif
> +
> +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_DNET),y)
> +OPENVMTOOLS_CONF_ENV += CUSTOM_DNET_CPPFLAGS="-I$(STAGING_DIR)/usr/include"
> +OPENVMTOOLS_CONF_OPT += --with-dnet
> +OPENVMTOOLS_DEPENDENCIES += libdnet
> +else
> +OPENVMTOOLS_CONF_OPT += --without-dnet
> +endif
> +
> +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PAM),y)
> +OPENVMTOOLS_CONF_OPT += --with-pam
> +OPENVMTOOLS_MAKE_OPT += CFLAGS+="-Wno-unused-local-typedefs"
> +OPENVMTOOLS_DEPENDENCIES += linux-pam
> +else
> +OPENVMTOOLS_CONF_OPT += --without-pam
> +endif
> +
> +define OPENVMTOOLS_POST_INSTALL_TARGET_THINGIES
> +#needed by lib/system/systemLinux.c (or will cry in /var/log/messages)

Do not put the comments in the macro; put them above the macro defintion.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v4 1/1] New package: openvmtools

[Thomas Petazzoni]

Karoly, Yann, All,

On Tue, 29 Jul 2014 13:57:12 +0200, Karoly Kasza wrote:

> +# When libfuse is available, openvmtools can build vmblock-fuse, so
> +# make sure that libfuse gets built first.
> +ifeq ($(BR2_PACKAGE_LIBFUSE),y)
> +OPENVMTOOLS_DEPENDENCIES += libfuse
> +endif
> +
> +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PROCPS),y)
> +OPENVMTOOLS_CONF_ENV += CUSTOM_PROCPS_NAME=procps LDFLAGS="-L$(TARGET_DIR)/usr/lib"
> +OPENVMTOOLS_CONF_OPT += --with-procps
> +OPENVMTOOLS_DEPENDENCIES += procps-ng
> +else
> +OPENVMTOOLS_CONF_OPT += --without-procps
> +endif
> +
> +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_DNET),y)
> +OPENVMTOOLS_CONF_ENV += CUSTOM_DNET_CPPFLAGS="-I$(STAGING_DIR)/usr/include"
> +OPENVMTOOLS_CONF_OPT += --with-dnet
> +OPENVMTOOLS_DEPENDENCIES += libdnet
> +else
> +OPENVMTOOLS_CONF_OPT += --without-dnet
> +endif
> +
> +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PAM),y)
> +OPENVMTOOLS_CONF_OPT += --with-pam
> +OPENVMTOOLS_MAKE_OPT += CFLAGS+="-Wno-unused-local-typedefs"
> +OPENVMTOOLS_DEPENDENCIES += linux-pam
> +else
> +OPENVMTOOLS_CONF_OPT += --without-pam
> +endif

Here I have a question not necessarily directly for you Karoly, but for
other Buildroot developers. We have a good example here of some
dependencies being automatic (libfuse), some dependencies having their
own package sub-options (procps, dnet, pam). Do we want to have this
separation of treatment ? What is the rationale to do one or the other
in this specific case ?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH v4 1/1] New package: openvmtools

[Károly Kasza]

Hi Yann,

Thanks for reviewing my patch!


>
> > +config BR2_PACKAGE_OPENVMTOOLS
> > +     bool "openvmtools"
> > +     depends on BR2_i386 || BR2_x86_64
> > +     depends on BR2_USE_MMU
> > +     depends on BR2_USE_WCHAR
> > +     depends on BR2_TOOLCHAIN_HAS_THREADS
>
> Are all those dependencies just inherited from libglib2, or are they
> real dependencies on openvmtools?
>
> If they are only inherited from libglib2, they it is customary to
> indicate the package as a comment to the dependency, like:
>
>     depends on BR2_USE_MMU # libglib2
>     depends on BR2_USE_WCHAR # libglib2


Actually they are, I did remove the comments since the v1 of the patch,
because I misinterpreted ThomasP's recommendation.
I'll put these back.


> > +if BR2_PACKAGE_OPENVMTOOLS
> > +
> > +menu "openvmtools options"
>
> Do not put it in a sub-menu, the options will be properly indented in
> the frontends.
>
> We only do sub-menus when there are a *lot* of sub-options (there is no
> written rule on this, but above ~8 warants a sub-menu, otherwise
> indentation is considered enough.)


OK, I'll remove the sub-menu.


> > +config BR2_PACKAGE_OPENVMTOOLS_PROCPS
> > +     bool "openvmtools procps support"
>
> Do not repeat 'openvmtools' in the prompts. Since it depends on
> BR2_PACKAGE_OPENVMTOOLS, the frontends wil properly indent the
> sub-options, so that it is obvious they are options to openvmtools.
>

OK.


> > +     select BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
>
> I know we alkready have one package and one systm option that select
> busybox-show-others, but they are a bit special: one is systemd, the
> other is sysvinit. All other packages depend on
> BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
> instead.
>
> For openvmtools, I would use a depends rather than a select, something
> like:
>
>     config BR2_PACKAGE_OPENVMTOOLS_PROCPS
>         bool "procps support"
>         depends on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
>         select BR2_PACKAGE_PROCPS_NG
>
>     comment "procps support needs that BUSYBOX_SHOW_OTHERS be set"
>         depends on !BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
>
> I'm not much satisfied with the comment text, however. If you find a
> better wording, feel free to adapt.


I'll change it to something like you recommended.


> > +config BR2_PACKAGE_OPENVMTOOLS_DNET
> > +     bool "openvmtools dnet support"
>
> Ditto: do not repeat 'openvmtools' in the prompts


OK.


> > +config BR2_PACKAGE_OPENVMTOOLS_PAM
> > +     bool "openvmtools PAM support"
>
> Ditto.
>

OK.


> > diff --git a/package/openvmtools/S10vmtoolsd
> b/package/openvmtools/S10vmtoolsd
> > new file mode 100644
> > index 0000000..3ab351e
> > --- /dev/null
> > +++ b/package/openvmtools/S10vmtoolsd
> > @@ -0,0 +1,33 @@
> > +#!/bin/sh
> > +#
> > +# Starts vmtoolsd for openvmtools
> > +#
> > +
> > +PID=/var/run/vmtoolsd.pid
> > +
> > +case "$1" in
> > +  start)
> > +     echo -n "Starting vmtoolsd: "
>
> This scripts uses a mix of tabs and spaces to do the indentation; for
> example, the cases lines ("start)", "stop)"...) are indented with
> spaces, while the rest is indented with tabs. Please be consistent.
>

I used dropbear's startup script as a template, and the indentation is
exactly the same. Also with openssh, exim or ptpd (and possibly some
others). I think I should not differ from those too much?


>
> > +
> > +exit $?
>
> No need to exit epxlicitly, it's implicit from POSIX, that a shell
> script exits with the error code from the last command. Besides, we do
> not care at all about the exit status of a startup script.
>

Although you are right, it's on the end of the dropbear startup script and
26 other initscripts in the package/ subdir. I think I should leave this to
stay consistent with the others as above.


> > +OPENVMTOOLS_CONF_OPT = \
> > +        --without-icu \
> > +        --without-x \
> > +        --without-gtk2 \
> > +        --without-gtkmm \
> > +        --without-kernel-modules
>
> On a single line. If it is too long, just split it, like:
>
>     OPENVMTOOLS_CONF_OPT = --without-icu --without-x \
>             --without-gtk2 --without-gtkmm --without-kernel-modules
>

OK. I though it is more visible.


> > +#-Wno-deprecated-declarations is a workaround for a bug in open-vm-tools
> > +#See http://sourceforge.net/p/open-vm-tools/mailman/message/31473171/
>
> Nit-pick: space after the '#' symbol.
>

OK.


>
> > +OPENVMTOOLS_CONF_ENV = CFLAGS+="-Wno-deprecated-declarations"
>
> This should be:
>
>     OPENVMTOOLS_CONF_ENV = CFLAGS="$(TARGET_CFLAGS)
> -Wno-deprecated-declarations"


OK.


> > +# When libfuse is available, openvmtools can build vmblock-fuse, so
> > +# make sure that libfuse gets built first.
> > +ifeq ($(BR2_PACKAGE_LIBFUSE),y)
> > +OPENVMTOOLS_DEPENDENCIES += libfuse
>
> No --enable-fuse/--disable-fuse options (or the likes)?
>

Unfortunately not. If the "configure" script finds libfuse, it builds
vmblock-fuse, else it won't. I experimented with it a bit, and found this
the most logical solution.


>
> > +define OPENVMTOOLS_POST_INSTALL_TARGET_THINGIES
> > +#needed by lib/system/systemLinux.c (or will cry in /var/log/messages)
>
> Do not put the comments in the macro; put them above the macro defintion.
>

OK.

Thanks again, I'll send a v5 patch soon. Regarding the init script, I can
use only spaces/tabs, but then this script will differ from the others.
Should I?

Kind regards,
Karoly Kasza
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20140730/9b856b87/attachment.html>

[Buildroot] [PATCH v4 1/1] New package: openvmtools

[Károly Kasza]

Hi Thomas,

On Tue, Jul 29, 2014 at 11:45 PM, Thomas Petazzoni <
thomas.petazzoni@free-electrons.com> wrote:

> Karoly, Yann, All,
>
> On Tue, 29 Jul 2014 13:57:12 +0200, Karoly Kasza wrote:
>
> > +# When libfuse is available, openvmtools can build vmblock-fuse, so
> > +# make sure that libfuse gets built first.
> > +ifeq ($(BR2_PACKAGE_LIBFUSE),y)
> > +OPENVMTOOLS_DEPENDENCIES += libfuse
> > +endif
> > +
> > +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PROCPS),y)
> > +OPENVMTOOLS_CONF_ENV += CUSTOM_PROCPS_NAME=procps
> LDFLAGS="-L$(TARGET_DIR)/usr/lib"
> > +OPENVMTOOLS_CONF_OPT += --with-procps
> > +OPENVMTOOLS_DEPENDENCIES += procps-ng
> > +else
> > +OPENVMTOOLS_CONF_OPT += --without-procps
> > +endif
> > +
> > +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_DNET),y)
> > +OPENVMTOOLS_CONF_ENV +=
> CUSTOM_DNET_CPPFLAGS="-I$(STAGING_DIR)/usr/include"
> > +OPENVMTOOLS_CONF_OPT += --with-dnet
> > +OPENVMTOOLS_DEPENDENCIES += libdnet
> > +else
> > +OPENVMTOOLS_CONF_OPT += --without-dnet
> > +endif
> > +
> > +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PAM),y)
> > +OPENVMTOOLS_CONF_OPT += --with-pam
> > +OPENVMTOOLS_MAKE_OPT += CFLAGS+="-Wno-unused-local-typedefs"
> > +OPENVMTOOLS_DEPENDENCIES += linux-pam
> > +else
> > +OPENVMTOOLS_CONF_OPT += --without-pam
> > +endif
>
> Here I have a question not necessarily directly for you Karoly, but for
> other Buildroot developers. We have a good example here of some
> dependencies being automatic (libfuse), some dependencies having their
> own package sub-options (procps, dnet, pam). Do we want to have this
> separation of treatment ? What is the rationale to do one or the other
> in this specific case ?
>

Some explanation:
In this special case, I can not turn off building vmblock-fuse with a
configure parameter. If it finds libfuse, it builds vmblock-fuse. I think
it's a minor defect in the configure script, it doesn't have a
--without-fuse or similar switch.
It's not pretty I know.

BR
Karoly
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20140730/fa2ae8de/attachment.html>

[Buildroot] [PATCH v4 1/1] New package: openvmtools

[Yann E. MORIN]

Karoly, All,

On 2014-07-30 00:41 +0200, K?roly Kasza spake thusly:
> Thanks for reviewing my patch!

Cheers! ;-)

[--SNIP--]
> > This scripts uses a mix of tabs and spaces to do the indentation; for
> > example, the cases lines ("start)", "stop)"...) are indented with
> > spaces, while the rest is indented with tabs. Please be consistent.
> 
> I used dropbear's startup script as a template, and the indentation is
> exactly the same. Also with openssh, exim or ptpd (and possibly some
> others). I think I should not differ from those too much?

OK, I undersand. But those scripts were intially written a loooong time
ago. We prefer that new submissions be a bit more coherent. Eventually,
someone will clean up those old scripts. One day. Maybe... ;-)

> > > +
> > > +exit $?
> >
> > No need to exit epxlicitly, it's implicit from POSIX, that a shell
> > script exits with the error code from the last command. Besides, we do
> > not care at all about the exit status of a startup script.
> 
> Although you are right, it's on the end of the dropbear startup script and
> 26 other initscripts in the package/ subdir. I think I should leave this to
> stay consistent with the others as above.

I prefer we introduce sane new scripts. As said above, the others are
really old, and no longer up to nowadays standards...

> > > +OPENVMTOOLS_CONF_OPT = \
> > > +        --without-icu \
> > > +        --without-x \
> > > +        --without-gtk2 \
> > > +        --without-gtkmm \
> > > +        --without-kernel-modules
> >
> > On a single line. If it is too long, just split it, like:
> >
> >     OPENVMTOOLS_CONF_OPT = --without-icu --without-x \
> >             --without-gtk2 --without-gtkmm --without-kernel-modules
> 
> OK. I though it is more visible.

I agree, but that's not the coding rules. ;-)

> > No --enable-fuse/--disable-fuse options (or the likes)?
> 
> Unfortunately not. If the "configure" script finds libfuse, it builds
> vmblock-fuse, else it won't. I experimented with it a bit, and found this
> the most logical solution.

OK, then.

> Thanks again, I'll send a v5 patch soon. Regarding the init script, I can
> use only spaces/tabs, but then this script will differ from the others.
> Should I?

Yes, new scripts should be cleaner and saner than the old ones.

Thanks for your persistence with this patch! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v4 1/1] New package: openvmtools

[Károly Kasza]

Hi Yann,

> I used dropbear's startup script as a template, and the indentation is
> > exactly the same. Also with openssh, exim or ptpd (and possibly some
> > others). I think I should not differ from those too much?
>
> OK, I undersand. But those scripts were intially written a loooong time
> ago. We prefer that new submissions be a bit more coherent. Eventually,
> someone will clean up those old scripts. One day. Maybe... ;-)
>
> > Although you are right, it's on the end of the dropbear startup script
> and
> > 26 other initscripts in the package/ subdir. I think I should leave this
> to
> > stay consistent with the others as above.
>
> I prefer we introduce sane new scripts. As said above, the others are
> really old, and no longer up to nowadays standards...
>
> > Thanks again, I'll send a v5 patch soon. Regarding the init script, I can
> > use only spaces/tabs, but then this script will differ from the others.
> > Should I?
>
> Yes, new scripts should be cleaner and saner than the old ones.
>

OK, I'll rewrite it, and also I can start rewriting all the others later,
no problem.
Besides on consistent usage of tabs/spaces (I guess changing tabs to spaces
using 2 for indenting) and omitting the exit code, do you have any other
suggestions? Can you point me to some coding style for scripts that you
prefer?

BR
Karoly
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20140730/cc90fb03/attachment.html>