| Message ID | 1396599134-2411-1-git-send-email-kaszak@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Dear Karoly Kasza, Thanks a lot for working on this, much appreciated! Here are a few comments below. On Fri, 4 Apr 2014 10:12:13 +0200, Karoly Kasza wrote: > Hi, > > this patch adds the latest stable open-vm-tools to buildroot. > (http://open-vm-tools.sourceforge.net/) > It should be compatible with all 3 libc implementations. > Also Contains a small patch for the source, a SysV init script and > a small shutdown script to make the VM power options usable from > the host OS. > Addresses Bugzilla bug 3991. > > BR > Karoly This part of your e-mail would become part of the git commit log forever, so it shouldn't contain "personal" informations. If you want to include personal informations, they should either be in an introduction e-mail separate from the patch itself (see "git send-email --cover"), or below the "---" sign after your Signed-off-by line. > > Signed-off-by: Karoly Kasza <kaszak@gmail.com> > --- > package/Config.in | 1 + > package/openvmtools/Config.in | 92 +++++++++++++++++++ > package/openvmtools/S98vmtoolsd | 31 +++++++ > .../openvmtools-01-has_bsd_printf.patch | 26 ++++++ > package/openvmtools/openvmtools.mk | 93 ++++++++++++++++++++ > package/openvmtools/shutdown | 7 ++ > 6 files changed, 250 insertions(+) > create mode 100644 package/openvmtools/Config.in > create mode 100755 package/openvmtools/S98vmtoolsd > create mode 100644 package/openvmtools/openvmtools-01-has_bsd_printf.patch > create mode 100644 package/openvmtools/openvmtools.mk > create mode 100755 package/openvmtools/shutdown > > diff --git a/package/Config.in b/package/Config.in > index e816603..907cb23 100644 > --- a/package/Config.in > +++ b/package/Config.in > @@ -328,6 +328,7 @@ source "package/on2-8170-modules/Config.in" > source "package/open2300/Config.in" > source "package/openocd/Config.in" > source "package/openpowerlink/Config.in" > +source "package/openvmtools/Config.in" Is openvmtools really related to Hardware handling? It should probably be under "System tools" or something like that. > source "package/owl-linux/Config.in" > source "package/parted/Config.in" > source "package/pciutils/Config.in" > diff --git a/package/openvmtools/Config.in b/package/openvmtools/Config.in > new file mode 100644 > index 0000000..fd9486b > --- /dev/null > +++ b/package/openvmtools/Config.in > @@ -0,0 +1,92 @@ > +config BR2_PACKAGE_OPENVMTOOLS > + bool "openvmtools" > + depends on BR2_i386 || BR2_x86_64 > + depends on BR2_USE_MMU #libglib2 > + depends on BR2_USE_WCHAR #libglib2 > + depends on BR2_TOOLCHAIN_HAS_THREADS #libglib2 One space after "#" > + depends on BR2_TOOLCHAIN_HAS_NATIVE_RPC #needed by an include in lib/include/vmxrpc.h For some reason, we generally don't document why these toolchain dependencies are needed. > + depends on BR2_LARGEFILE #needed by an include in lib/include/vmxrpc.h > + depends on BR2_ENABLE_LOCALE #needed by __locale_t in lib/misc/codesetOld.c Ditto. > + select BR2_PACKAGE_LIBGLIB2 #always needed No need for "#always needed". > + help > + Open Virtual Machine Tools for VMware guest OS > + > + http://open-vm-tools.sourceforge.net/ > + > +if BR2_PACKAGE_OPENVMTOOLS > + > +menu "openvmtools options" > + > +#This can't be disabled by configure, will compile itself if libfuse is found, > +#so we warn the user > +comment "openvmtools vmblock-fuse is disabled because these is no libfuse!" > + depends on !BR2_PACKAGE_LIBFUSE > + > +comment "openvmtools vmblock-fuse is enabled because libfuse is found!" > + depends on BR2_PACKAGE_LIBFUSE Such comments are not necessary. You may want to include some explanation in the help text of the main option, such as: Support for vmblock-fuse will be enabled in openvmtools if the libfuse package is selected. > +config BR2_PACKAGE_OPENVMTOOLS_PROCPS > + bool "openvmtools procps support" > + depends on BR2_PACKAGE_PROCPS Why a "depends on" here? > + help > + Enable support for procps / meminfo > + > +comment "openvmtools procps support needs procps" > + depends on !BR2_PACKAGE_PROCPS > + > +config BR2_PACKAGE_OPENVMTOOLS_DNET > + bool "openvmtools dnet support" > + depends on BR2_INET_IPV6 #needed by services/plugins/guestInfo/getlib/guestInfoPosix.c > + select BR2_PACKAGE_LIBDNET And a select here? Also, this is a bit inconsistent with the choice you've made for the libfuse dependency: * Either you add explicit sub-options under the openvmtools options, for the various optional features, and you make them "select" the appropriate dependencies. * Or you make these optional features automatically enabled when the necessary set of dependencies are available, as you did for vmblock-fuse above. > + 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 > + > +config BR2_PACKAGE_OPENVMTOOLS_MODULES > + bool "openvmtools older kernel modules - SEE HELP!" > + depends on BR2_LINUX_KERNEL > + help > + Compile older Linux kernel modules for openvmtools > + This is absolutely NOT NEEDED for recent kernels, as they already > + include open-vm-tools modules. > + Better to choose this in a current Linux kernel's config, not here. > + Only compile this if you are positive that you need it! > + Also, this is highly untested, so probably you'll need to hack it. > + > +comment "openvmtools older kernel modules needs a Linux kernel to be built" > + depends on !BR2_LINUX_KERNEL > + > +comment "openvmtools ICU locales are not supported" Don't put comment for things that are not supported. > + > +#Does not compile, also needs some hacks in configure... > +#config BR2_PACKAGE_OPENVMTOOLS_ICU > +# bool "openvmtools icu locales suport" > +# depends on BR2_INSTALL_LIBSTDCPP #needed by icu > +# select BR2_PACKAGE_ICU > +# help > +# Use icu for openvmtools locales > +# I would only recommend to use this only if you already has icu, > +# as it is large and requires a C++ compiler > + > +#comment "openvmtools icu locales support needs a toolchain w/ C++" > +# depends on !BR2_INSTALL_LIBSTDCPP Don't keep commented code. > + > +comment "openvmtools X11 tools are not supported" Same as above. > + > +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/S98vmtoolsd b/package/openvmtools/S98vmtoolsd > new file mode 100755 > index 0000000..4b51e0a > --- /dev/null > +++ b/package/openvmtools/S98vmtoolsd > @@ -0,0 +1,31 @@ > +#!/bin/sh > +# > +# Start vmtoolsd > +# > + > +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" I believe it would be better to use start-stop-daemon here. Have a look at package/connman/S45connman for an example. > diff --git a/package/openvmtools/openvmtools.mk b/package/openvmtools/openvmtools.mk > new file mode 100644 > index 0000000..1278dae > --- /dev/null > +++ b/package/openvmtools/openvmtools.mk > @@ -0,0 +1,93 @@ > +############################################################# > +# > +# openvmtools > +# > +############################################################# There should be 80 # signs for this header, and an empty line between the header and the first variable. > +OPENVMTOOLS_VERSION = 9.4.0-1280544 > +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_CONF_OPT += --disable-docs Not needed, already passed by the autotools-package infrastructure. > +OPENVMTOOLS_MAKE_OPT += "CFLAGS+=-Wno-unused-local-typedefs" This should be part of _CONF_ENV, as follows: # A comment that explains why this is needed. OPENVMTOOLS_CONF_ENV = \ CFLAGS="$(TARGET_CFLAGS) -Wno-unused-local-typedefs" > +OPENVMTOOLS_DEPENDENCIES += libglib2 > +OPENVMTOOLS_POST_INSTALL_TARGET_HOOKS += OPENVMTOOLS_POST_INSTALL_TARGET_THINGIES The registration of a hook should be close (usually right after) the hook definition. > + > +#If libfuse is found, vmblock-fuse is always compiled, we just set a sequence "set a sequence" is a bit confusing, and is not part of the typical Buildroot terminology. Instead this could be something like: # 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_OPT += --with-procps LDFLAGS="-L$(TARGET_DIR)/lib -L$(TARGET_DIR)/usr/lib" No: you should instead change the procps package (as part of a separate patch) to make it install its libraries to the staging directory. Then you won't need those special LDFLAGS. > +OPENVMTOOLS_DEPENDENCIES += procps > +else > +OPENVMTOOLS_CONF_OPT += --without-procps > +endif > + > +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_DNET),y) > +OPENVMTOOLS_CONF_OPT += --with-dnet CUSTOM_DNET_CPPFLAGS=-I$(TARGET_DIR)/usr/include libdnet headers are installed to the staging directory, so there should be no need for these CUSTOM_DNET_CPPFLAGS, and if they are really needed, they should point to $(STAGING_DIR). > +OPENVMTOOLS_DEPENDENCIES += libdnet > +OPENVMTOOLS_PRE_CONFIGURE_HOOKS += OPENVMTOOLS_PRE_CONFIGURE_DNET_FIX > +else > +OPENVMTOOLS_CONF_OPT += --without-dnet > +endif > + > +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PAM),y) > +OPENVMTOOLS_CONF_OPT += --with-pam > +OPENVMTOOLS_DEPENDENCIES += linux-pam > +else > +OPENVMTOOLS_CONF_OPT += --without-pam > +endif > + > +#For older kernels only! > +#See http://sourceforge.net/p/open-vm-tools/mailman/message/30113760/ for some clue > +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_MODULES),y) > +OPENVMTOOLS_CONF_OPT += --with-kernel-modules --with-linuxdir=$(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION) Use $(LINUX_DIR) instead of $(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION). Especially since $(LINUX_HEADERS_VERSION) is the version of the kernel headers, not the version of the kernel being built. > +OPENVMTOOLS_DEPENDENCIES += linux > +OPENVMTOOLS_PRE_CONFIGURE_HOOKS += OPENVMTOOLS_PRE_CONFIGURE_LINUX_FIX > +else > +OPENVMTOOLS_CONF_OPT += --without-kernel-modules > +endif > + > +#Does not configure or compile, needs hacks, also not really needed... > +#ifeq ($(BR2_PACKAGE_OPENVMTOOLS_ICU),y) > +#OPENVMTOOLS_CONF_OPT += --with-icu > +#OPENVMTOOLS_DEPENDENCIES = icu > +#else > +OPENVMTOOLS_CONF_OPT += --without-icu > +#endif Don't keep commented code. > + > +#No X11 suport (yet) > +OPENVMTOOLS_CONF_OPT += --without-x > +OPENVMTOOLS_CONF_OPT += --without-gtk2 > +OPENVMTOOLS_CONF_OPT += --without-gtkmm Add these at the top of the file, like: OPENVMTOOLS_CONF_OPT = \ --without-x \ --without-gtk2 \ --without-gtkmm > +#create a "build" directory symlink if building kernel modules > +define OPENVMTOOLS_PRE_CONFIGURE_LINUX_FIX > + if [ ! -e $(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION)/build ]; then \ > + ln -s . $(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION)/build; \ Huh, why? > + fi > +endef > + > +define OPENVMTOOLS_PRE_CONFIGURE_DNET_FIX > + cp -f $(BUILD_DIR)/libdnet-$(LIBDNET_VERSION)/dnet-config $(TARGET_DIR)/usr/bin Arrggh, no! It is absolutely horrible for package A to install files from package B into the target directory. In addition to this, dnet-config is already installed into $(STAGING_DIR)/usr/bin/. Normally, the openvmtools configure script should take an environment variable that allows to set the location of the dnet-config script. > +endef > + > +define OPENVMTOOLS_POST_INSTALL_TARGET_THINGIES > +#needed by lib/system/systemLinux.c (or will cry in /var/log/messages) > + if [ ! -e $(TARGET_DIR)/etc/lfs-release ]; then \ > + ln -s os-release $(TARGET_DIR)/etc/lfs-release; \ > + fi > +#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 > + > +define OPENVMTOOLS_INSTALL_INIT_SYSV > + $(INSTALL) -D -m 755 package/openvmtools/S98vmtoolsd \ > + $(TARGET_DIR)/etc/init.d/S98vmtoolsd > +endef > + > +$(eval $(autotools-package)) > diff --git a/package/openvmtools/shutdown b/package/openvmtools/shutdown > new file mode 100755 > 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 Thanks! Thomas
Hello Thomas, On Sun, Apr 6, 2014 at 3:23 PM, Thomas Petazzoni < thomas.petazzoni@free-electrons.com> wrote: > Dear Karoly Kasza, > > Thanks a lot for working on this, much appreciated! Here are a few > comments below. > > On Fri, 4 Apr 2014 10:12:13 +0200, Karoly Kasza wrote: > > Hi, > > > > this patch adds the latest stable open-vm-tools to buildroot. > > (http://open-vm-tools.sourceforge.net/) > > It should be compatible with all 3 libc implementations. > > Also Contains a small patch for the source, a SysV init script and > > a small shutdown script to make the VM power options usable from > > the host OS. > > Addresses Bugzilla bug 3991. > > > > BR > > Karoly > > This part of your e-mail would become part of the git commit log > forever, so it shouldn't contain "personal" informations. If you want > to include personal informations, they should either be in an > introduction e-mail separate from the patch itself (see "git send-email > --cover"), or below the "---" sign after your Signed-off-by line. > > OK, I'm new to git. > > > > Signed-off-by: Karoly Kasza <kaszak@gmail.com> > > --- > > package/Config.in | 1 + > > package/openvmtools/Config.in | 92 > +++++++++++++++++++ > > package/openvmtools/S98vmtoolsd | 31 +++++++ > > .../openvmtools-01-has_bsd_printf.patch | 26 ++++++ > > package/openvmtools/openvmtools.mk | 93 > ++++++++++++++++++++ > > package/openvmtools/shutdown | 7 ++ > > 6 files changed, 250 insertions(+) > > create mode 100644 package/openvmtools/Config.in > > create mode 100755 package/openvmtools/S98vmtoolsd > > create mode 100644 > package/openvmtools/openvmtools-01-has_bsd_printf.patch > > create mode 100644 package/openvmtools/openvmtools.mk > > create mode 100755 package/openvmtools/shutdown > > > > diff --git a/package/Config.in b/package/Config.in > > index e816603..907cb23 100644 > > --- a/package/Config.in > > +++ b/package/Config.in > > @@ -328,6 +328,7 @@ source "package/on2-8170-modules/Config.in" > > source "package/open2300/Config.in" > > source "package/openocd/Config.in" > > source "package/openpowerlink/Config.in" > > +source "package/openvmtools/Config.in" > > Is openvmtools really related to Hardware handling? It should probably > be under "System tools" or something like that. > This is a decision to make. I put it there, because it is basically a set of virtual hardware handling tools. If you like it better at System tools, I'll put it there, no problem. Should I replace it? > > > source "package/owl-linux/Config.in" > > source "package/parted/Config.in" > > source "package/pciutils/Config.in" > > diff --git a/package/openvmtools/Config.in > b/package/openvmtools/Config.in > > new file mode 100644 > > index 0000000..fd9486b > > --- /dev/null > > +++ b/package/openvmtools/Config.in > > @@ -0,0 +1,92 @@ > > +config BR2_PACKAGE_OPENVMTOOLS > > + bool "openvmtools" > > + depends on BR2_i386 || BR2_x86_64 > > + depends on BR2_USE_MMU #libglib2 > > + depends on BR2_USE_WCHAR #libglib2 > > + depends on BR2_TOOLCHAIN_HAS_THREADS #libglib2 > > One space after "#" > > > + depends on BR2_TOOLCHAIN_HAS_NATIVE_RPC #needed by an include in > lib/include/vmxrpc.h > > For some reason, we generally don't document why these > toolchain dependencies are needed. > I think it would be a good idea for later reference. I can of course remove them. > > > + depends on BR2_LARGEFILE #needed by an include in > lib/include/vmxrpc.h > > + depends on BR2_ENABLE_LOCALE #needed by __locale_t in > lib/misc/codesetOld.c > > Ditto. > > > + select BR2_PACKAGE_LIBGLIB2 #always needed > > No need for "#always needed". > > > + help > > + Open Virtual Machine Tools for VMware guest OS > > + > > + http://open-vm-tools.sourceforge.net/ > > + > > +if BR2_PACKAGE_OPENVMTOOLS > > + > > +menu "openvmtools options" > > + > > +#This can't be disabled by configure, will compile itself if libfuse is > found, > > +#so we warn the user > > +comment "openvmtools vmblock-fuse is disabled because these is no > libfuse!" > > + depends on !BR2_PACKAGE_LIBFUSE > > + > > +comment "openvmtools vmblock-fuse is enabled because libfuse is found!" > > + depends on BR2_PACKAGE_LIBFUSE > > Such comments are not necessary. You may want to include some > explanation in the help text of the main option, such as: > > Support for vmblock-fuse will be enabled in openvmtools if the > libfuse package is selected. > OK > > > +config BR2_PACKAGE_OPENVMTOOLS_PROCPS > > + bool "openvmtools procps support" > > + depends on BR2_PACKAGE_PROCPS > > Why a "depends on" here? > > > + help > > + Enable support for procps / meminfo > > + > > +comment "openvmtools procps support needs procps" > > + depends on !BR2_PACKAGE_PROCPS > > + > > +config BR2_PACKAGE_OPENVMTOOLS_DNET > > + bool "openvmtools dnet support" > > + depends on BR2_INET_IPV6 #needed by > services/plugins/guestInfo/getlib/guestInfoPosix.c > > + select BR2_PACKAGE_LIBDNET > > And a select here? > > Also, this is a bit inconsistent with the choice you've made for the > libfuse dependency: > > * Either you add explicit sub-options under the openvmtools options, > for the various optional features, and you make them "select" the > appropriate dependencies. > > * Or you make these optional features automatically enabled when the > necessary set of dependencies are available, as you did for > vmblock-fuse above. > I tried to use these as stated in Buildroot manual Section 7.2.2 "Choosing depends on or select" Select for libraries - like libdnet, and depends on for packages the user should be aware of. - Procps is a package that is only enabled if you select "Show packages that are also provided by busybox". I can of course make both selected, but is it a good idea? > > > + 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 > > + > > +config BR2_PACKAGE_OPENVMTOOLS_MODULES > > + bool "openvmtools older kernel modules - SEE HELP!" > > + depends on BR2_LINUX_KERNEL > > + help > > + Compile older Linux kernel modules for openvmtools > > + This is absolutely NOT NEEDED for recent kernels, as they already > > + include open-vm-tools modules. > > + Better to choose this in a current Linux kernel's config, not > here. > > + Only compile this if you are positive that you need it! > > + Also, this is highly untested, so probably you'll need to hack > it. > > + > > +comment "openvmtools older kernel modules needs a Linux kernel to be > built" > > + depends on !BR2_LINUX_KERNEL > > + > > +comment "openvmtools ICU locales are not supported" > > Don't put comment for things that are not supported. > OK, it was for later reference. > > > + > > +#Does not compile, also needs some hacks in configure... > > +#config BR2_PACKAGE_OPENVMTOOLS_ICU > > +# bool "openvmtools icu locales suport" > > +# depends on BR2_INSTALL_LIBSTDCPP #needed by icu > > +# select BR2_PACKAGE_ICU > > +# help > > +# Use icu for openvmtools locales > > +# I would only recommend to use this only if you already has icu, > > +# as it is large and requires a C++ compiler > > + > > +#comment "openvmtools icu locales support needs a toolchain w/ C++" > > +# depends on !BR2_INSTALL_LIBSTDCPP > > Don't keep commented code. > It looked like a good idea if anybody else wants to add the support in the future. > > > + > > +comment "openvmtools X11 tools are not supported" > > Same as above. > > > + > > +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/S98vmtoolsd > b/package/openvmtools/S98vmtoolsd > > new file mode 100755 > > index 0000000..4b51e0a > > --- /dev/null > > +++ b/package/openvmtools/S98vmtoolsd > > @@ -0,0 +1,31 @@ > > +#!/bin/sh > > +# > > +# Start vmtoolsd > > +# > > + > > +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" > > I believe it would be better to use start-stop-daemon here. Have a look > at package/connman/S45connman for an example. > I know start-stop-daemon, but I don't really see its advantage over this simple script. Stopping only occurs when shutting down the VM, and the daemon creates its PID when starting in bg mode. I can rewrite it, but is it needed? > > > > diff --git a/package/openvmtools/openvmtools.mk b/package/openvmtools/ > openvmtools.mk > > new file mode 100644 > > index 0000000..1278dae > > --- /dev/null > > +++ b/package/openvmtools/openvmtools.mk > > @@ -0,0 +1,93 @@ > > +############################################################# > > +# > > +# openvmtools > > +# > > +############################################################# > > There should be 80 # signs for this header, and an empty line between > the header and the first variable. > I copied it from some other package, may be it was outdated. I'll correct this. > > > +OPENVMTOOLS_VERSION = 9.4.0-1280544 > > +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_CONF_OPT += --disable-docs > > Not needed, already passed by the autotools-package infrastructure. > True. > > > +OPENVMTOOLS_MAKE_OPT += "CFLAGS+=-Wno-unused-local-typedefs" > > This should be part of _CONF_ENV, as follows: > OK. > > # A comment that explains why this is needed. > OPENVMTOOLS_CONF_ENV = \ > CFLAGS="$(TARGET_CFLAGS) -Wno-unused-local-typedefs" > > > +OPENVMTOOLS_DEPENDENCIES += libglib2 > > +OPENVMTOOLS_POST_INSTALL_TARGET_HOOKS += > OPENVMTOOLS_POST_INSTALL_TARGET_THINGIES > > The registration of a hook should be close (usually right after) the > hook definition. > I'll replace it. > > > + > > +#If libfuse is found, vmblock-fuse is always compiled, we just set a > sequence > > "set a sequence" is a bit confusing, and is not part of the typical > Buildroot terminology. Instead this could be something like: > > # When libfuse is available, openvmtools can build vmblock-fuse, so > # make sure that libfuse gets built first. > OK. > > > +ifeq ($(BR2_PACKAGE_LIBFUSE),y) > > +OPENVMTOOLS_DEPENDENCIES += libfuse > > +endif > > + > > +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PROCPS),y) > > +OPENVMTOOLS_CONF_OPT += --with-procps LDFLAGS="-L$(TARGET_DIR)/lib > -L$(TARGET_DIR)/usr/lib" > > No: you should instead change the procps package (as part of a separate > patch) to make it install its libraries to the staging directory. Then > you won't need those special LDFLAGS. > I didn't want to patch anybody else's package just to make mine more comfortable. I think the special LDFLAGS exists for a reason, and this can be. If you want me to do it, I can create a patch for procps. I'd stay with LDFLAGS, or should I create a procps patch? > > > +OPENVMTOOLS_DEPENDENCIES += procps > > +else > > +OPENVMTOOLS_CONF_OPT += --without-procps > > +endif > > + > > +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_DNET),y) > > +OPENVMTOOLS_CONF_OPT += --with-dnet > CUSTOM_DNET_CPPFLAGS=-I$(TARGET_DIR)/usr/include > > libdnet headers are installed to the staging directory, so there should > be no need for these CUSTOM_DNET_CPPFLAGS, and if they are really > needed, they should point to $(STAGING_DIR). > They are needed, for some reason configure in open-vm-tools won't find libdnet any other way. I'll try it with $(STAGING_DIR). > > +OPENVMTOOLS_DEPENDENCIES += libdnet > > +OPENVMTOOLS_PRE_CONFIGURE_HOOKS += OPENVMTOOLS_PRE_CONFIGURE_DNET_FIX > > +else > > +OPENVMTOOLS_CONF_OPT += --without-dnet > > +endif > > + > > +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PAM),y) > > +OPENVMTOOLS_CONF_OPT += --with-pam > > +OPENVMTOOLS_DEPENDENCIES += linux-pam > > +else > > +OPENVMTOOLS_CONF_OPT += --without-pam > > +endif > > + > > +#For older kernels only! > > +#See http://sourceforge.net/p/open-vm-tools/mailman/message/30113760/for some clue > > +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_MODULES),y) > > +OPENVMTOOLS_CONF_OPT += --with-kernel-modules > --with-linuxdir=$(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION) > > Use $(LINUX_DIR) instead of > $(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION). Especially since > $(LINUX_HEADERS_VERSION) is the version of the kernel headers, not the > version of the kernel being built. > OK. > > > +OPENVMTOOLS_DEPENDENCIES += linux > > +OPENVMTOOLS_PRE_CONFIGURE_HOOKS += OPENVMTOOLS_PRE_CONFIGURE_LINUX_FIX > > +else > > +OPENVMTOOLS_CONF_OPT += --without-kernel-modules > > +endif > > + > > +#Does not configure or compile, needs hacks, also not really needed... > > +#ifeq ($(BR2_PACKAGE_OPENVMTOOLS_ICU),y) > > +#OPENVMTOOLS_CONF_OPT += --with-icu > > +#OPENVMTOOLS_DEPENDENCIES = icu > > +#else > > +OPENVMTOOLS_CONF_OPT += --without-icu > > +#endif > > Don't keep commented code. > > + > > +#No X11 suport (yet) > > +OPENVMTOOLS_CONF_OPT += --without-x > > +OPENVMTOOLS_CONF_OPT += --without-gtk2 > > +OPENVMTOOLS_CONF_OPT += --without-gtkmm > > Add these at the top of the file, like: > > OPENVMTOOLS_CONF_OPT = \ > --without-x \ > --without-gtk2 \ > --without-gtkmm > OK. > > > +#create a "build" directory symlink if building kernel modules > > +define OPENVMTOOLS_PRE_CONFIGURE_LINUX_FIX > > + if [ ! -e $(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION)/build ]; > then \ > > + ln -s . $(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION)/build; > \ > > Huh, why? > Configure didn't find it other whence. I'll check it with $(LINUX_DIR). > > > + fi > > +endef > > + > > +define OPENVMTOOLS_PRE_CONFIGURE_DNET_FIX > > + cp -f $(BUILD_DIR)/libdnet-$(LIBDNET_VERSION)/dnet-config > $(TARGET_DIR)/usr/bin > > Arrggh, no! It is absolutely horrible for package A to install files > from package B into the target directory. In addition to this, > dnet-config is already installed into $(STAGING_DIR)/usr/bin/. > Normally, the openvmtools configure script should take an environment > variable that allows to set the location of the dnet-config script. > OK, I don't know if such environment variable exists. I'll look for it. What should I do if no such variable exist? Patch libdnet or this package? > > > +endef > > + > > +define OPENVMTOOLS_POST_INSTALL_TARGET_THINGIES > > +#needed by lib/system/systemLinux.c (or will cry in /var/log/messages) > > + if [ ! -e $(TARGET_DIR)/etc/lfs-release ]; then \ > > + ln -s os-release $(TARGET_DIR)/etc/lfs-release; \ > > + fi > > +#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 > > + > > +define OPENVMTOOLS_INSTALL_INIT_SYSV > > + $(INSTALL) -D -m 755 package/openvmtools/S98vmtoolsd \ > > + $(TARGET_DIR)/etc/init.d/S98vmtoolsd > > +endef > > + > > +$(eval $(autotools-package)) > > diff --git a/package/openvmtools/shutdown b/package/openvmtools/shutdown > > new file mode 100755 > > 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 > > Thanks! > > Thomas > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com Allow me a few comments: - buildroot is not an easy system to work with, and the _expected_ structure of *.mk and config.in files are very undocumented. For example the $(LINUX_DIR) macro isn't mentioned in the user manual. This makes it hard to develop onto and most of your remarks are resulting from this. - I don't know why you don't like comments in source code, as they make it more easy to modify for outsiders. That's why I used them - for the future reference. Also, documenting for the dependencies should be a good idea. It is really the purpose of the # tag. Please answer my few in-line questions then I'll send a v2 patch in a few days! Kind regards, Karoly
diff --git a/package/Config.in b/package/Config.in index e816603..907cb23 100644 --- a/package/Config.in +++ b/package/Config.in @@ -328,6 +328,7 @@ source "package/on2-8170-modules/Config.in" source "package/open2300/Config.in" source "package/openocd/Config.in" source "package/openpowerlink/Config.in" +source "package/openvmtools/Config.in" source "package/owl-linux/Config.in" source "package/parted/Config.in" source "package/pciutils/Config.in" diff --git a/package/openvmtools/Config.in b/package/openvmtools/Config.in new file mode 100644 index 0000000..fd9486b --- /dev/null +++ b/package/openvmtools/Config.in @@ -0,0 +1,92 @@ +config BR2_PACKAGE_OPENVMTOOLS + bool "openvmtools" + depends on BR2_i386 || BR2_x86_64 + depends on BR2_USE_MMU #libglib2 + depends on BR2_USE_WCHAR #libglib2 + depends on BR2_TOOLCHAIN_HAS_THREADS #libglib2 + depends on BR2_TOOLCHAIN_HAS_NATIVE_RPC #needed by an include in lib/include/vmxrpc.h + depends on BR2_LARGEFILE #needed by an include in lib/include/vmxrpc.h + depends on BR2_ENABLE_LOCALE #needed by __locale_t in lib/misc/codesetOld.c + select BR2_PACKAGE_LIBGLIB2 #always needed + help + Open Virtual Machine Tools for VMware guest OS + + http://open-vm-tools.sourceforge.net/ + +if BR2_PACKAGE_OPENVMTOOLS + +menu "openvmtools options" + +#This can't be disabled by configure, will compile itself if libfuse is found, +#so we warn the user +comment "openvmtools vmblock-fuse is disabled because these is no libfuse!" + depends on !BR2_PACKAGE_LIBFUSE + +comment "openvmtools vmblock-fuse is enabled because libfuse is found!" + depends on BR2_PACKAGE_LIBFUSE + +config BR2_PACKAGE_OPENVMTOOLS_PROCPS + bool "openvmtools procps support" + depends on BR2_PACKAGE_PROCPS + help + Enable support for procps / meminfo + +comment "openvmtools procps support needs procps" + depends on !BR2_PACKAGE_PROCPS + +config BR2_PACKAGE_OPENVMTOOLS_DNET + bool "openvmtools dnet support" + depends on BR2_INET_IPV6 #needed by services/plugins/guestInfo/getlib/guestInfoPosix.c + 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 + +config BR2_PACKAGE_OPENVMTOOLS_MODULES + bool "openvmtools older kernel modules - SEE HELP!" + depends on BR2_LINUX_KERNEL + help + Compile older Linux kernel modules for openvmtools + This is absolutely NOT NEEDED for recent kernels, as they already + include open-vm-tools modules. + Better to choose this in a current Linux kernel's config, not here. + Only compile this if you are positive that you need it! + Also, this is highly untested, so probably you'll need to hack it. + +comment "openvmtools older kernel modules needs a Linux kernel to be built" + depends on !BR2_LINUX_KERNEL + +comment "openvmtools ICU locales are not supported" + +#Does not compile, also needs some hacks in configure... +#config BR2_PACKAGE_OPENVMTOOLS_ICU +# bool "openvmtools icu locales suport" +# depends on BR2_INSTALL_LIBSTDCPP #needed by icu +# select BR2_PACKAGE_ICU +# help +# Use icu for openvmtools locales +# I would only recommend to use this only if you already has icu, +# as it is large and requires a C++ compiler + +#comment "openvmtools icu locales support needs a toolchain w/ C++" +# depends on !BR2_INSTALL_LIBSTDCPP + +comment "openvmtools X11 tools are not supported" + +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/S98vmtoolsd b/package/openvmtools/S98vmtoolsd new file mode 100755 index 0000000..4b51e0a --- /dev/null +++ b/package/openvmtools/S98vmtoolsd @@ -0,0 +1,31 @@ +#!/bin/sh +# +# Start vmtoolsd +# + +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) + ;; + *) + 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..a33b67b --- /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> + +--- openvmtools-9.4.0-1280544.orig/lib/misc/msgList.c 2013-09-23 17:51:10.000000000 +0200 ++++ openvmtools-9.4.0-1280544/lib/misc/msgList.c 2014-04-03 13:42:14.138500061 +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..1278dae --- /dev/null +++ b/package/openvmtools/openvmtools.mk @@ -0,0 +1,93 @@ +############################################################# +# +# openvmtools +# +############################################################# +OPENVMTOOLS_VERSION = 9.4.0-1280544 +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_CONF_OPT += --disable-docs +OPENVMTOOLS_MAKE_OPT += "CFLAGS+=-Wno-unused-local-typedefs" +OPENVMTOOLS_DEPENDENCIES += libglib2 +OPENVMTOOLS_POST_INSTALL_TARGET_HOOKS += OPENVMTOOLS_POST_INSTALL_TARGET_THINGIES + +#If libfuse is found, vmblock-fuse is always compiled, we just set a sequence +ifeq ($(BR2_PACKAGE_LIBFUSE),y) +OPENVMTOOLS_DEPENDENCIES += libfuse +endif + +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PROCPS),y) +OPENVMTOOLS_CONF_OPT += --with-procps LDFLAGS="-L$(TARGET_DIR)/lib -L$(TARGET_DIR)/usr/lib" +OPENVMTOOLS_DEPENDENCIES += procps +else +OPENVMTOOLS_CONF_OPT += --without-procps +endif + +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_DNET),y) +OPENVMTOOLS_CONF_OPT += --with-dnet CUSTOM_DNET_CPPFLAGS=-I$(TARGET_DIR)/usr/include +OPENVMTOOLS_DEPENDENCIES += libdnet +OPENVMTOOLS_PRE_CONFIGURE_HOOKS += OPENVMTOOLS_PRE_CONFIGURE_DNET_FIX +else +OPENVMTOOLS_CONF_OPT += --without-dnet +endif + +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_PAM),y) +OPENVMTOOLS_CONF_OPT += --with-pam +OPENVMTOOLS_DEPENDENCIES += linux-pam +else +OPENVMTOOLS_CONF_OPT += --without-pam +endif + +#For older kernels only! +#See http://sourceforge.net/p/open-vm-tools/mailman/message/30113760/ for some clue +ifeq ($(BR2_PACKAGE_OPENVMTOOLS_MODULES),y) +OPENVMTOOLS_CONF_OPT += --with-kernel-modules --with-linuxdir=$(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION) +OPENVMTOOLS_DEPENDENCIES += linux +OPENVMTOOLS_PRE_CONFIGURE_HOOKS += OPENVMTOOLS_PRE_CONFIGURE_LINUX_FIX +else +OPENVMTOOLS_CONF_OPT += --without-kernel-modules +endif + +#Does not configure or compile, needs hacks, also not really needed... +#ifeq ($(BR2_PACKAGE_OPENVMTOOLS_ICU),y) +#OPENVMTOOLS_CONF_OPT += --with-icu +#OPENVMTOOLS_DEPENDENCIES = icu +#else +OPENVMTOOLS_CONF_OPT += --without-icu +#endif + +#No X11 suport (yet) +OPENVMTOOLS_CONF_OPT += --without-x +OPENVMTOOLS_CONF_OPT += --without-gtk2 +OPENVMTOOLS_CONF_OPT += --without-gtkmm + +#create a "build" directory symlink if building kernel modules +define OPENVMTOOLS_PRE_CONFIGURE_LINUX_FIX + if [ ! -e $(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION)/build ]; then \ + ln -s . $(BUILD_DIR)/linux-$(LINUX_HEADERS_VERSION)/build; \ + fi +endef + +define OPENVMTOOLS_PRE_CONFIGURE_DNET_FIX + cp -f $(BUILD_DIR)/libdnet-$(LIBDNET_VERSION)/dnet-config $(TARGET_DIR)/usr/bin +endef + +define OPENVMTOOLS_POST_INSTALL_TARGET_THINGIES +#needed by lib/system/systemLinux.c (or will cry in /var/log/messages) + if [ ! -e $(TARGET_DIR)/etc/lfs-release ]; then \ + ln -s os-release $(TARGET_DIR)/etc/lfs-release; \ + fi +#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 + +define OPENVMTOOLS_INSTALL_INIT_SYSV + $(INSTALL) -D -m 755 package/openvmtools/S98vmtoolsd \ + $(TARGET_DIR)/etc/init.d/S98vmtoolsd +endef + +$(eval $(autotools-package)) diff --git a/package/openvmtools/shutdown b/package/openvmtools/shutdown new file mode 100755 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
Hi, this patch adds the latest stable open-vm-tools to buildroot. (http://open-vm-tools.sourceforge.net/) It should be compatible with all 3 libc implementations. Also Contains a small patch for the source, a SysV init script and a small shutdown script to make the VM power options usable from the host OS. Addresses Bugzilla bug 3991. BR Karoly Signed-off-by: Karoly Kasza <kaszak@gmail.com> --- package/Config.in | 1 + package/openvmtools/Config.in | 92 +++++++++++++++++++ package/openvmtools/S98vmtoolsd | 31 +++++++ .../openvmtools-01-has_bsd_printf.patch | 26 ++++++ package/openvmtools/openvmtools.mk | 93 ++++++++++++++++++++ package/openvmtools/shutdown | 7 ++ 6 files changed, 250 insertions(+) create mode 100644 package/openvmtools/Config.in create mode 100755 package/openvmtools/S98vmtoolsd create mode 100644 package/openvmtools/openvmtools-01-has_bsd_printf.patch create mode 100644 package/openvmtools/openvmtools.mk create mode 100755 package/openvmtools/shutdown