[Libvir] using gnulib: starting with the physmem and getaddrinfo modules
Recently, I heard of two tricky portability problems in libvirt that are easy to solve with gnulib. Of course, gnulib provides a lot more, and is not exactly lightweight if you count "lines of code imported", but once the framework (this patch) is installed, adding an additional module is as easy as adding the module name to a list. Keep in mind that what matters with a portability library is that it stay out of your way -- and of course do its job well. To that end, gnulib has many per-module unit tests. Since a large part of its job is portability, and insulating your code from bugs (be they bugs in the very latest glibc printf functions, or in the regexp library, or just protection against that crufty old SunOS4 free function that can't deal with NULL), it provides a mechanism to let you conveniently include its self tests in your own package and run them when users run "make check" for your project. This is pretty important, so that if someone builds your package on an unusual or poorly-configured system, often the gnulib tests will fail right away, and that will save you the trouble of digging through your own package's code. For those of you who don't know, gnulib is the GNU Portability Library. http://www.gnu.org/software/gnulib/ and it is used in many widely-used packages: http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=blob;f=users.txt;hb=HEAD Of course, there's no guarantee that the code is bug-free, but in practice, the few bugs being fixed these days involve portability to relatively unusual systems. With so many widely-used packages using gnulib, you can imagine that if there are bugs, they will be encountered, reported, and fixed pretty quickly. One of the biggest advantages of using gnulib is that you can add new modules and get all the benefits with *no* additional tweaking. I.e., you don't have to write .m4 macros, modify configure.ac or any Makefile.am. It just works. If something doesn't, or if the documentation is unclear, you can report it to bug-gnulib@gnu.org and usually get a fix or an explanation pretty quickly. ---------- Enough of the hype. The first portability problem was to determine the total physical memory available on the current system. Currently the code works only on Linux-like systems that have /proc/meminfo of an expected form. However, the gnulib physmem module handles 13 distinct types of systems and is well tested: http://www.gnu.org/software/gnulib/MODULES.html#module=physmem The second portability problem was to find a robust and LGPL-compatible getaddrinfo function to be used on systems lacking it. Here's the gnulib module: http://www.gnu.org/software/gnulib/MODULES.html#module=getaddrinfo Once the few gnulib hooks are in your configure.ac and Makefile.am files, there is very little extra work required to use the new functions. In the case of physmem, just use the function and include "physmem.h". For getaddrinfo, merely include "getaddrinfo.h" from the two files that use the function. This change brings in a lot of code, but many of the lib/.[ch] files are used only on systems that lack some required functionality. For example, the getaddrinfo.c file isn't even compiled when it's not needed. In the patch below, I've included a new script called bootstrap. It is a wrapper around gnulib-tool that pulls into libvirt the files selected by the (currently two) modules in use. Those new files go in three places: m4/*.m4 lib/*.[ch] and a few template .h.in files gl-tests/ for unit test C programs and Bourne shell scripts However, note that gettextize and libtoolize (run by autogen.sh) also deposit many *.m4 files in m4. I compared and found that 8 of the files that are already pulled in by the *ize programs are also pulled in (potentially newer versions) from gnulib. But currently, using gettext-0.16.1 or gettext-0.17, there is no difference in any of the overlapping files. Re Licenses: the two modules (and all of their dependent modules) are LGPL-compatible. This is enforced by running gnulib-tool with the --lgpl option. If you were to request a module with an incompatible license (say GPL or LGPLv3), it would fail. ---------------------- Here's the patch that shows what existing parts of libvirt have to be modified to use these two new modules. To try it out, just apply the patch and then run this: ./autogen.sh && ./bootstrap && make && make check Running bootstrap creates the new lib/ and gl-tests/ directories. Personally, I prefer not to add generated files to version control systems, because it can lead to problems with version skew if all developers don't use the same releases of the tools that do the generating. Perhaps more importantly, when there are massive diffs in the generated files, that can obscure real changes in non-generated parts of the code. That already happens to me whenever the .po files change. But if people prefer to add all of these imported files to CVS, just say the word and I'll prepare the patch. If so, do you guys want the gettextize- and libtoolize-added files to be version-controlled, now, too? Use gnulib, starting with physmem and getaddrinfo modules. * bootstrap: A wrapper around gnulib-tool. * configure.in: Invoke gl_EARLY and gl_INIT, being careful to put gl_EARLY before any macro that uses AC_COMPILE_IFELSE. (AC_OUTPUT): Add lib/Makefile and gl-tests/Makefile. Remove m4/Makefile. * Makefile.am (SUBDIRS): Add lib and gl-tests. Remove m4. * m4/Makefile.am: Remove file. Not needed. * src/Makefile.am (INCLUDES): Add -I$(top_srcdir)/lib -I../lib. (LDADDS, libvirt_la_LIBADD): Add ../lib/libgnu.la. * src/nodeinfo.c: Include "physmem.h". * qemud/qemud.c, src/remote_internal.c: Include "getaddrinfo.h". (MEMINFO_PATH, linuxNodeInfoMemPopulate): Remove definitions. (virNodeInfoPopulate): Use physmem_total, not linuxNodeInfoMemPopulate. * tests/Makefile.am (INCLUDES): Add -I$(top_srcdir)/lib -I../lib. (LDADDS): Add ../lib/libgnu.la. * qemud/Makefile.am (libvirtd_LDADD): Add ../lib/libgnu.la. * tests/nodeinfotest.c (linuxTestCompareFiles): No longer read total memory from a file. Update expected output not to include "Memory: NNNN" * tests/nodeinfodata/linux-nodeinfo-1.txt: * tests/nodeinfodata/linux-nodeinfo-2.txt: * tests/nodeinfodata/linux-nodeinfo-3.txt: * tests/nodeinfodata/linux-nodeinfo-4.txt: * tests/nodeinfodata/linux-nodeinfo-5.txt: * tests/nodeinfodata/linux-nodeinfo-6.txt: * src/test.c [WITH_TEST]: Remove definition of _GNU_SOURCE that would conflict with the one now in "config.h". Signed-off-by: Jim Meyering <meyering@redhat.com> --- Makefile.am | 3 +- bootstrap | 90 +++++++++++++++++++++++++++++++ configure.in | 11 +++- m4/Makefile.am | 3 - qemud/Makefile.am | 3 +- qemud/qemud.c | 1 + src/Makefile.am | 8 ++- src/nodeinfo.c | 55 +++---------------- src/remote_internal.c | 1 + src/test.c | 2 - tests/Makefile.am | 2 + tests/nodeinfodata/linux-nodeinfo-1.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-2.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-3.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-4.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-5.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-6.txt | 2 +- tests/nodeinfotest.c | 23 ++------ 18 files changed, 131 insertions(+), 83 deletions(-) create mode 100755 bootstrap delete mode 100644 m4/Makefile.am diff --git a/Makefile.am b/Makefile.am index eaa204e..fcf0eb6 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1,6 +1,7 @@ ## Process this file with automake to produce Makefile.in -SUBDIRS = src qemud proxy include docs @PYTHON_SUBDIR@ tests po m4 scripts +SUBDIRS = lib src qemud proxy include docs @PYTHON_SUBDIR@ \ + gl-tests tests po scripts ACLOCAL_AMFLAGS = -I m4 diff --git a/bootstrap b/bootstrap new file mode 100755 index 0000000..b49378d --- /dev/null +++ b/bootstrap @@ -0,0 +1,90 @@ +#!/bin/sh +# Run this after autogen.sh, to pull in all of the gnulib-related bits. +# It's important to run *after* autogen.sh, since it updates some of +# the same files autogen.sh does, yet those from gnulib are newer, +# and match the tests. So if a gnulib bug has been fixed since the +# snapshot taken for whatever gettext release you're using, yet you +# run "make check" against the wrong version, the corresponding unit +# test in gl-tests/ may well fail. + +usage() { + echo >&2 "\ +Usage: $0 [OPTION]... +Bootstrap this package from the checked-out sources. + +Options: + --gnulib-srcdir=DIRNAME Specify the local directory where gnulib + sources reside. Use this if you already + have gnulib sources on your machine, and + do not want to waste your bandwidth downloading + them again. + +If the file bootstrap.conf exists in the current working directory, its +contents are read as shell variables to configure the bootstrap. + +Running without arguments will suffice in most cases. +" +} + +for option +do + case $option in + --help) + usage + exit;; + --gnulib-srcdir=*) + GNULIB_SRCDIR=`expr "$option" : '--gnulib-srcdir=\(.*\)'`;; + *) + echo >&2 "$0: $option: unknown option" + exit 1;; + esac +done + +case ${GNULIB_SRCDIR--} in +-) + if [ ! -d gnulib ]; then + echo "$0: getting gnulib files..." + + trap cleanup_gnulib 1 2 13 15 + + git clone --depth 1 git://git.sv.gnu.org/gnulib || + cleanup_gnulib + + trap - 1 2 13 15 + fi + GNULIB_SRCDIR=gnulib +esac + +gnulib_tool=$GNULIB_SRCDIR/gnulib-tool +<$gnulib_tool || exit + +# Tell gnulib to: +# put tests in new gl-tests/ dir +# put m4/*.m4 files in existing m4/ dir +# lib/*.[ch] files in new lib/ dir. +# With --avoid=snprintf, we pull in about 30 fewer files +# With the current gnulib and gettext-0.17, the following +# files are added to m4/ by both. But gnulib is stable enough +# and gettext-0.16.1 is new enough that they are identical. +# compiler-flags.m4 +# inttypes_h.m4 +# longlong.m4 +# size_max.m4 +# stdint_h.m4 +# wchar_t.m4 +# wint_t.m4 +# xsize.m4 + +# Note that if we don't exclude the snprintf module, there are two tests +# that have incompatible licenses, so we would have to exclude them. Even +# excluding those two test modules, find reports a total of 94 added files. +# Yes, snprintf has some heavy-duty dependents. +# --avoid=snprintf-tests +# --avoid=vasnprintf-tests + +$gnulib_tool \ + --lgpl \ + --avoid=snprintf \ + --with-tests \ + --tests-base=gl-tests \ + --import physmem getaddrinfo diff --git a/configure.in b/configure.in index a845720..18a533f 100644 --- a/configure.in +++ b/configure.in @@ -28,11 +28,15 @@ AVAHI_REQUIRED="0.6.0" dnl Checks for C compiler. AC_PROG_CC -AM_PROG_CC_STDC -AC_C_CONST AC_PROG_INSTALL AC_PROG_CPP +gl_EARLY +gl_INIT + +AM_PROG_CC_STDC +AC_C_CONST + dnl Make sure we have an ANSI compiler AM_C_PROTOTYPES test "x$U" != "x" && AC_MSG_ERROR(Compiler not ANSI compliant) @@ -562,11 +566,12 @@ cp COPYING.LIB COPYING AC_OUTPUT(Makefile src/Makefile include/Makefile docs/Makefile \ docs/examples/Makefile docs/devhelp/Makefile \ docs/examples/python/Makefile \ + lib/Makefile gl-tests/Makefile \ libvirt.pc libvirt.spec \ po/Makefile.in scripts/Makefile \ include/libvirt/Makefile include/libvirt/libvirt.h \ python/Makefile python/tests/Makefile \ - qemud/Makefile m4/Makefile \ + qemud/Makefile \ tests/Makefile proxy/Makefile \ tests/xml2sexprdata/Makefile \ tests/sexpr2xmldata/Makefile \ diff --git a/m4/Makefile.am b/m4/Makefile.am deleted file mode 100644 index 188b2fe..0000000 --- a/m4/Makefile.am +++ /dev/null @@ -1,3 +0,0 @@ - -EXTRA_DIST = compiler-flags.m4 - diff --git a/qemud/Makefile.am b/qemud/Makefile.am index 1737176..267b43e 100644 --- a/qemud/Makefile.am +++ b/qemud/Makefile.am @@ -27,6 +27,7 @@ libvirtd_SOURCES = \ #-D_XOPEN_SOURCE=600 -D_XOPEN_SOURCE_EXTENDED=1 -D_POSIX_C_SOURCE=199506L libvirtd_CFLAGS = \ + -I$(top_srcdir)/lib -I../lib \ -I$(top_srcdir)/include -I$(top_builddir)/include \ $(LIBXML_CFLAGS) $(GNUTLS_CFLAGS) \ $(WARN_CFLAGS) -DLOCAL_STATE_DIR="\"$(localstatedir)\"" \ @@ -37,7 +38,7 @@ libvirtd_CFLAGS = \ libvirtd_LDFLAGS = $(WARN_CFLAGS) $(LIBXML_LIBS) $(GNUTLS_LIBS) libvirtd_DEPENDENCIES = ../src/libvirt.la -libvirtd_LDADD = ../src/libvirt.la +libvirtd_LDADD = ../src/libvirt.la ../lib/libgnu.la if HAVE_AVAHI libvirtd_SOURCES += mdns.c mdns.h diff --git a/qemud/qemud.c b/qemud/qemud.c index f88ed42..caccd29 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -53,6 +53,7 @@ #include <libvirt/virterror.h> #include "internal.h" +#include "getaddrinfo.h" #include "../src/internal.h" #include "../src/remote_internal.h" #include "../src/conf.h" diff --git a/src/Makefile.am b/src/Makefile.am index 52ffbaf..efea233 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1,6 +1,8 @@ ## Process this file with automake to produce Makefile.in -INCLUDES = -I$(top_builddir)/include \ +INCLUDES = \ + -I$(top_srcdir)/lib -I../lib \ + -I../include \ -I@top_srcdir@/include \ -I@top_srcdir@/qemud \ $(LIBXML_CFLAGS) \ @@ -14,7 +16,7 @@ INCLUDES = -I$(top_builddir)/include \ $(WARN_CFLAGS) \ $(LIBVIRT_FEATURES) DEPS = libvirt.la -LDADDS = @STATIC_BINARIES@ $(WARN_CFLAGS) libvirt.la +LDADDS = @STATIC_BINARIES@ $(WARN_CFLAGS) libvirt.la ../lib/libgnu.la VIRSH_LIBS = @VIRSH_LIBS@ confdir = $(sysconfdir)/libvirt/ @@ -60,7 +62,7 @@ SERVER_SOURCES = \ libvirt_la_SOURCES = $(CLIENT_SOURCES) $(SERVER_SOURCES) libvirt_la_LIBADD = $(LIBXML_LIBS) $(GNUTLS_LIBS) $(LTLIBOBJS) \ - @CYGWIN_EXTRA_LIBADD@ + @CYGWIN_EXTRA_LIBADD@ ../lib/libgnu.la libvirt_la_LDFLAGS = -Wl,--version-script=$(srcdir)/libvirt_sym.version \ -version-info @LIBVIRT_VERSION_INFO@ \ $(COVERAGE_CFLAGS:-f%=-Wc,-f%) \ diff --git a/src/nodeinfo.c b/src/nodeinfo.c index a0a26eb..2ef49cb 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -29,14 +29,14 @@ #include <ctype.h> #include "nodeinfo.h" +#include "physmem.h" #ifdef __linux__ -#define MEMINFO_PATH "/proc/meminfo" #define CPUINFO_PATH "/proc/cpuinfo" /* NB, these are not static as we need to call them from testsuite */ -int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr nodeinfo); -int linuxNodeInfoMemPopulate(virConnectPtr conn, FILE *meminfo, virNodeInfoPtr nodeinfo); +int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, + virNodeInfoPtr nodeinfo); int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr nodeinfo) { char line[1024]; @@ -114,44 +114,11 @@ int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr n return 0; } - -int linuxNodeInfoMemPopulate(virConnectPtr conn, FILE *meminfo, - virNodeInfoPtr nodeinfo) { - char line[1024]; - - nodeinfo->memory = 0; - - while (fgets(line, sizeof(line), meminfo) != NULL) { - if (STREQLEN(line, "MemTotal:", 9)) { - char *p; - unsigned int ui; - if (xstrtol_ui(line + 10, &p, 10, &ui) == 0 - && (*p == '\0' || isspace(*p))) { - nodeinfo->memory = ui; - break; - } - } - } - if (!nodeinfo->memory) { - __virRaiseError(conn, NULL, NULL, 0, VIR_ERR_INTERNAL_ERROR, - VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, - "no memory found"); - return -1; - } - - return 0; -} - - #endif int virNodeInfoPopulate(virConnectPtr conn, virNodeInfoPtr nodeinfo) { struct utsname info; -#ifdef __linux__ - int ret; - FILE *cpuinfo, *meminfo; -#endif if (uname(&info) < 0) { __virRaiseError(conn, NULL, NULL, 0, VIR_ERR_INTERNAL_ERROR, @@ -164,7 +131,9 @@ int virNodeInfoPopulate(virConnectPtr conn, nodeinfo->model[sizeof(nodeinfo->model)-1] = '\0'; #ifdef __linux__ - cpuinfo = fopen(CPUINFO_PATH, "r"); + { + int ret; + FILE *cpuinfo = fopen(CPUINFO_PATH, "r"); if (!cpuinfo) { __virRaiseError(conn, NULL, NULL, 0, VIR_ERR_INTERNAL_ERROR, VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, @@ -176,17 +145,11 @@ int virNodeInfoPopulate(virConnectPtr conn, if (ret < 0) return -1; - meminfo = fopen(MEMINFO_PATH, "r"); - if (!meminfo) { - __virRaiseError(conn, NULL, NULL, 0, VIR_ERR_INTERNAL_ERROR, - VIR_ERR_ERROR, NULL, NULL, NULL, 0, 0, - "cannot open %s %s", MEMINFO_PATH, strerror(errno)); - return -1; - } - ret = linuxNodeInfoMemPopulate(conn, meminfo, nodeinfo); - fclose(meminfo); + /* Convert to KB. */ + nodeinfo->memory = physmem_total () / 1024; return ret; + } #else /* XXX Solaris will need an impl later if they port QEMU driver */ __virRaiseError(conn, NULL, NULL, 0, VIR_ERR_INTERNAL_ERROR, diff --git a/src/remote_internal.c b/src/remote_internal.c index a8227f3..275405a 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -52,6 +52,7 @@ #include "internal.h" #include "driver.h" +#include "getaddrinfo.h" #include "remote_internal.h" #include "remote_protocol.h" diff --git a/src/test.c b/src/test.c index 010ea15..bab280f 100644 --- a/src/test.c +++ b/src/test.c @@ -25,8 +25,6 @@ #ifdef WITH_TEST -#define _GNU_SOURCE /* for asprintf */ - #include <stdio.h> #include <string.h> #include <sys/time.h> diff --git a/tests/Makefile.am b/tests/Makefile.am index 512162b..4de0c45 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -11,6 +11,7 @@ SUBDIRS = virshdata confdata sexpr2xmldata xml2sexprdata xmconfigdata xencapsdat LIBVIRT = $(wildcard $(top_builddir)/src/.libs/libvirt_la-*.o) INCLUDES = \ + -I$(top_srcdir)/lib -I../lib \ -I$(top_builddir)/include \ -I$(top_builddir)/src \ -I$(top_srcdir)/include \ @@ -29,6 +30,7 @@ LDADDS = \ $(GNUTLS_LIBS) \ $(WARN_CFLAGS) \ $(LIBVIRT) \ + ../lib/libgnu.la \ $(COVERAGE_LDFLAGS) EXTRA_DIST = \ diff --git a/tests/nodeinfodata/linux-nodeinfo-1.txt b/tests/nodeinfodata/linux-nodeinfo-1.txt index 1a38ad1..e52e20a 100644 --- a/tests/nodeinfodata/linux-nodeinfo-1.txt +++ b/tests/nodeinfodata/linux-nodeinfo-1.txt @@ -1 +1 @@ -CPUs: 2, MHz: 2800, Nodes: 1, Sockets: 1, Cores: 2, Threads: 1, Memory: 2053960 +CPUs: 2, MHz: 2800, Nodes: 1, Sockets: 1, Cores: 2, Threads: 1 diff --git a/tests/nodeinfodata/linux-nodeinfo-2.txt b/tests/nodeinfodata/linux-nodeinfo-2.txt index 1c31a0c..12e819b 100644 --- a/tests/nodeinfodata/linux-nodeinfo-2.txt +++ b/tests/nodeinfodata/linux-nodeinfo-2.txt @@ -1 +1 @@ -CPUs: 2, MHz: 2211, Nodes: 1, Sockets: 1, Cores: 2, Threads: 1, Memory: 4059540 +CPUs: 2, MHz: 2211, Nodes: 1, Sockets: 1, Cores: 2, Threads: 1 diff --git a/tests/nodeinfodata/linux-nodeinfo-3.txt b/tests/nodeinfodata/linux-nodeinfo-3.txt index e2cc841..d285781 100644 --- a/tests/nodeinfodata/linux-nodeinfo-3.txt +++ b/tests/nodeinfodata/linux-nodeinfo-3.txt @@ -1 +1 @@ -CPUs: 4, MHz: 1595, Nodes: 1, Sockets: 2, Cores: 2, Threads: 1, Memory: 4059272 +CPUs: 4, MHz: 1595, Nodes: 1, Sockets: 2, Cores: 2, Threads: 1 diff --git a/tests/nodeinfodata/linux-nodeinfo-4.txt b/tests/nodeinfodata/linux-nodeinfo-4.txt index 2c75ea3..991d4f9 100644 --- a/tests/nodeinfodata/linux-nodeinfo-4.txt +++ b/tests/nodeinfodata/linux-nodeinfo-4.txt @@ -1 +1 @@ -CPUs: 4, MHz: 1000, Nodes: 1, Sockets: 1, Cores: 4, Threads: 1, Memory: 4059272 +CPUs: 4, MHz: 1000, Nodes: 1, Sockets: 1, Cores: 4, Threads: 1 diff --git a/tests/nodeinfodata/linux-nodeinfo-5.txt b/tests/nodeinfodata/linux-nodeinfo-5.txt index 01fee52..dce7ada 100644 --- a/tests/nodeinfodata/linux-nodeinfo-5.txt +++ b/tests/nodeinfodata/linux-nodeinfo-5.txt @@ -1 +1 @@ -CPUs: 4, MHz: 2814, Nodes: 1, Sockets: 2, Cores: 2, Threads: 1, Memory: 4059272 +CPUs: 4, MHz: 2814, Nodes: 1, Sockets: 2, Cores: 2, Threads: 1 diff --git a/tests/nodeinfodata/linux-nodeinfo-6.txt b/tests/nodeinfodata/linux-nodeinfo-6.txt index a7a2cfe..75cdaa9 100644 --- a/tests/nodeinfodata/linux-nodeinfo-6.txt +++ b/tests/nodeinfodata/linux-nodeinfo-6.txt @@ -1 +1 @@ -CPUs: 4, MHz: 1000, Nodes: 1, Sockets: 2, Cores: 2, Threads: 1, Memory: 4059272 +CPUs: 4, MHz: 1000, Nodes: 1, Sockets: 2, Cores: 2, Threads: 1 diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index 7275cc3..fb563b5 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -21,14 +21,13 @@ static char *abs_top_srcdir; #ifdef __linux__ extern int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr nodeinfo); -extern int linuxNodeInfoMemPopulate(virConnectPtr conn, FILE *meminfo, virNodeInfoPtr nodeinfo); -static int linuxTestCompareFiles(const char *cpuinfofile, const char *meminfofile, const char *outputfile) { +static int linuxTestCompareFiles(const char *cpuinfofile, const char *outputfile) { char actualData[MAX_FILE]; char expectData[MAX_FILE]; char *expect = &expectData[0]; virNodeInfo nodeinfo; - FILE *cpuinfo, *meminfo; + FILE *cpuinfo; if (virtTestLoadFile(outputfile, &expect, MAX_FILE) < 0) return -1; @@ -42,19 +41,10 @@ static int linuxTestCompareFiles(const char *cpuinfofile, const char *meminfofil } fclose(cpuinfo); - meminfo = fopen(meminfofile, "r"); - if (!meminfo) - return -1; - if (linuxNodeInfoMemPopulate(NULL, meminfo, &nodeinfo) < 0) { - fclose(meminfo); - return -1; - } - fclose(meminfo); - snprintf(actualData, MAX_FILE, - "CPUs: %u, MHz: %u, Nodes: %u, Sockets: %u, Cores: %u, Threads: %u, Memory: %lu\n", + "CPUs: %u, MHz: %u, Nodes: %u, Sockets: %u, Cores: %u, Threads: %u\n", nodeinfo.cpus, nodeinfo.mhz, nodeinfo.nodes, nodeinfo.sockets, - nodeinfo.cores, nodeinfo.threads, nodeinfo.memory); + nodeinfo.cores, nodeinfo.threads); if (STRNEQ(actualData, expectData)) { if (getenv("DEBUG_TESTS")) { @@ -70,15 +60,12 @@ static int linuxTestCompareFiles(const char *cpuinfofile, const char *meminfofil static int linuxTestNodeInfo(const void *data) { char cpuinfo[PATH_MAX]; - char meminfo[PATH_MAX]; char output[PATH_MAX]; snprintf(cpuinfo, PATH_MAX, "%s/tests/nodeinfodata/linux-%s.cpuinfo", abs_top_srcdir, (const char*)data); - snprintf(meminfo, PATH_MAX, "%s/tests/nodeinfodata/linux-%s.meminfo", - abs_top_srcdir, (const char*)data); snprintf(output, PATH_MAX, "%s/tests/nodeinfodata/linux-%s.txt", abs_top_srcdir, (const char*)data); - return linuxTestCompareFiles(cpuinfo, meminfo, output); + return linuxTestCompareFiles(cpuinfo, output); } #endif -- 1.5.3.6.950.g92b7b
On Wed, Dec 05, 2007 at 12:33:24AM +0100, Jim Meyering wrote:
The first portability problem was to determine the total physical memory available on the current system. Currently the code works only on Linux-like systems that have /proc/meminfo of an expected form. However, the gnulib physmem module handles 13 distinct types of systems and is well tested:
http://www.gnu.org/software/gnulib/MODULES.html#module=physmem
No argument there. I'm not fond of my current code code physical mem.
The second portability problem was to find a robust and LGPL-compatible getaddrinfo function to be used on systems lacking it. Here's the gnulib module:
http://www.gnu.org/software/gnulib/MODULES.html#module=getaddrinfo
Yep, need it for windows
Once the few gnulib hooks are in your configure.ac and Makefile.am files, there is very little extra work required to use the new functions. In the case of physmem, just use the function and include "physmem.h". For getaddrinfo, merely include "getaddrinfo.h" from the two files that use the function.
I've not looked closely, but since we're unconditiaonlly including it, I assume the getaddrinfo.h file is setup to not override the existing definition of getaddrinfo if the local OS has it.
This change brings in a lot of code, but many of the lib/.[ch] files are used only on systems that lack some required functionality. For example, the getaddrinfo.c file isn't even compiled when it's not needed.
In the patch below, I've included a new script called bootstrap. It is a wrapper around gnulib-tool that pulls into libvirt the files selected by the (currently two) modules in use. Those new files go in three places:
m4/*.m4 lib/*.[ch] and a few template .h.in files gl-tests/ for unit test C programs and Bourne shell scripts
I think the source code should go into gnulib/*.[ch] in case we ever want to have a lib/ dir for code shared by the daemon & library. There's no need to pollute the top level dir with gl-tests, when we can have tests/gnulib/, or gnulib/tests/. We've already got an m4/ directory, so we might as well use that (or a m4/gnulib, or gnulib/m4 subdir).
However, note that gettextize and libtoolize (run by autogen.sh) also deposit many *.m4 files in m4. I compared and found that 8 of the files that are already pulled in by the *ize programs are also pulled in (potentially newer versions) from gnulib. But currently, using gettext-0.16.1 or gettext-0.17, there is no difference in any of the overlapping files.
This sounds like the trickiest issue. We currently run both of those tools using --force, so they overwrite any existing files present in the local tree. IIRC libtoolize shouldn't touch the m4/ directory, but gettextize (or rather autopoint) definitely does.
Here's the patch that shows what existing parts of libvirt have to be modified to use these two new modules. To try it out, just apply the patch and then run this:
./autogen.sh && ./bootstrap && make && make check
Running bootstrap creates the new lib/ and gl-tests/ directories.
I don't want a dependancy on a script pulling in files from another random project that we have to grab from the internet.
Personally, I prefer not to add generated files to version control systems, because it can lead to problems with version skew if all developers don't use the same releases of the tools that do the generating. Perhaps more importantly, when there are massive diffs in the generated files, that can obscure real changes in non-generated parts of the code. That already happens to me whenever the .po files change.
Given a fresh CVS checkout of libvirt & fresh Fedora/RHEL/etc OS install one should be able to do a full build without requiring extra code checkouts from unrelated projects. Since gnulib is only distributed in source form, and not available from the OS package manager, the only viable approach is to commit the GNULib files to CVS. IMHO, this *reduces* the problem of version skew, because it doesn't rely on each developer getting the same copy of gnulib. We can update the in-CVS copy of the gnulib bits we care about as & when we need them and know everyone has the same bits. If gnulib really is as stable as its said to be, then we won't be seeing frequent massive diffs - we'll just have minor additions as we find more portability modules we need.
But if people prefer to add all of these imported files to CVS, just say the word and I'll prepare the patch. If so, do you guys want the gettextize- and libtoolize-added files to be version-controlled, now, too?
So the gettextize script copies its stuff into m4/ when you run the autogen script. We need to make sure the gnulib stuff in CVS takes priority over stuff added by gettextize. Does aclocal have any kind of ordering when you give it multiple include paths with -I arg. eg would 'aclocal -I m4/ -I gnulib/m4' ensure correct prioritization ? Another option if we go with the idea of having gnulib stuff in a subdir like gnulib/m4/*.m4, would be to change autogen.sh to do autopoint --force cp -f gnulib/m4/*.m4 m4/ ie, always use the top-level m4/ directory as a scratch area for stuff we don't commit to CVS, but that is used when creating configure.in. On the other hand, this means the released tar.gz would have 2 copies of the m4, so a 3rd option is to just remove the --force arg from autopoint and let the gnulib stuff naturally take priority by virtue of already existing on disk when autopoint is run. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Daniel P. Berrange wrote:
I think the source code should go into gnulib/*.[ch] in case we ever want to have a lib/ dir for code shared by the daemon & library. There's no need to pollute the top level dir with gl-tests, when we can have tests/gnulib/, or gnulib/tests/. We've already got an m4/ directory, so we might as well use that (or a m4/gnulib, or gnulib/m4 subdir).
Minor point here, but the way that Jim's bootstrap script is set up, it will create a gnulib/ temporary directory if it needs to git-clone gnulib.
I don't want a dependancy on a script pulling in files from another random project that we have to grab from the internet.
And this, I think, is the reason to check in the generated source files to CVS. It's not something I'd normally advocate, but there is surely a special exception to be made if (a) the file has to be downloaded from somewhere or (b) the file rarely changes and is hard to generate. The 'rpcgen'-generated files fall into (b), and the gnulib files fall into both categories.
Given a fresh CVS checkout of libvirt & fresh Fedora/RHEL/etc OS install one should be able to do a full build without requiring extra code checkouts from unrelated projects. Since gnulib is only distributed in source form, and not available from the OS package manager, the only viable approach is to commit the GNULib files to CVS.
Agreed. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
include "physmem.h". For getaddrinfo, merely include "getaddrinfo.h" from the two files that use the function.
Hi Dan,
I've not looked closely, but since we're unconditiaonlly including it, I assume the getaddrinfo.h file is setup to not override the existing definition of getaddrinfo if the local OS has it.
Yes, it works :-) getaddrinfo.h is provided by the gnulib module, so it's always present. As far as I know, no system provides /usr/include/getaddrinfo.h, so there's no risk of masking some existing definition there.
This change brings in a lot of code, but many of the lib/.[ch] files are used only on systems that lack some required functionality. For example, the getaddrinfo.c file isn't even compiled when it's not needed.
In the patch below, I've included a new script called bootstrap. It is a wrapper around gnulib-tool that pulls into libvirt the files selected by the (currently two) modules in use. Those new files go in three places:
m4/*.m4 lib/*.[ch] and a few template .h.in files gl-tests/ for unit test C programs and Bourne shell scripts
Actually, you indicated preference for the above directory names. I'm happy that you think differently, now :-)
I think the source code should go into gnulib/*.[ch] in case we ever want to have a lib/ dir for code shared by the daemon & library. There's no need to pollute the top level dir with gl-tests, when we can have tests/gnulib/, or gnulib/tests/. We've already got an m4/ directory, so we might as well use that (or a m4/gnulib, or gnulib/m4 subdir).
However, note that gettextize and libtoolize (run by autogen.sh) also deposit many *.m4 files in m4. I compared and found that 8 of the files that are already pulled in by the *ize programs are also pulled in (potentially newer versions) from gnulib. But currently, using gettext-0.16.1 or gettext-0.17, there is no difference in any of the overlapping files.
This sounds like the trickiest issue. We currently run both of those tools using --force, so they overwrite any existing files present in the local tree. IIRC libtoolize shouldn't touch the m4/ directory, but gettextize (or rather autopoint) definitely does.
Actually, libtoolize does. e.g., m4/libtool.m4 and m4/lt*.m4. ...
Running bootstrap creates the new lib/ and gl-tests/ directories.
I don't want a dependancy on a script pulling in files from another random project that we have to grab from the internet.
Personally, I prefer not to add generated files to version control systems, because it can lead to problems with version skew if all developers don't use the same releases of the tools that do the generating. Perhaps more importantly, when there are massive diffs in the generated files, that can obscure real changes in non-generated parts of the code. That already happens to me whenever the .po files change.
Given a fresh CVS checkout of libvirt & fresh Fedora/RHEL/etc OS install one should be able to do a full build without requiring extra code checkouts from unrelated projects. Since gnulib is only distributed in source form, and not available from the OS package manager, the only viable approach is to commit the GNULib files to CVS. ... By "viable" surely you mean: "viable for libvirt" :-)
As I said, I am happy to do it whichever way you guys prefer. My next patch will include cvs-adding all of the pulled-in files, but will leave the existing m4/ directory and not cvs-add all of the files added by gettextize and libtoolize.
IMHO, this *reduces* the problem of version skew, because it doesn't rely on each developer getting the same copy of gnulib. We can update
In my experience, getting the right copy of gnulib matters far less than using recent enough versions of autoconf, automake, libtool, gettext. *unless* you're the person who just reported that snprintf (or the function of the week) segfaults under unusual conditions when run on some old version of e.g., HP/UX.
the in-CVS copy of the gnulib bits we care about as & when we need them and know everyone has the same bits. If gnulib really is as stable as its said to be, then we won't be seeing frequent massive diffs - we'll just have minor additions as we find more portability modules we need.
Right.
But if people prefer to add all of these imported files to CVS, just say the word and I'll prepare the patch. If so, do you guys want the gettextize- and libtoolize-added files to be version-controlled, now, too?
So the gettextize script copies its stuff into m4/ when you run the autogen script. We need to make sure the gnulib stuff in CVS takes priority over stuff added by gettextize. Does aclocal have any kind of ordering when you give it multiple include paths with -I arg. eg would
'aclocal -I m4/ -I gnulib/m4'
ensure correct prioritization ?
Looks like it. I'll confirm.
Another option if we go with the idea of having gnulib stuff in a subdir like gnulib/m4/*.m4, would be to change autogen.sh to do
autopoint --force cp -f gnulib/m4/*.m4 m4/
ie, always use the top-level m4/ directory as a scratch area for stuff we don't commit to CVS, but that is used when creating configure.in.
On the other hand, this means the released tar.gz would have 2 copies of the m4, so a 3rd option is to just remove the --force arg from autopoint and let the gnulib stuff naturally take priority by virtue of already existing on disk when autopoint is run.
I like the idea of keeping the m4 directories separate. Will follow up with a proposal.
Jim Meyering wrote:
diff --git a/m4/Makefile.am b/m4/Makefile.am deleted file mode 100644 index 188b2fe..0000000 --- a/m4/Makefile.am +++ /dev/null @@ -1,3 +0,0 @@ - -EXTRA_DIST = compiler-flags.m4 -
Does this mean that compiler-flags.m4 is no longer going into the tarball? Shouldn't it?
-libvirtd_LDADD = ../src/libvirt.la +libvirtd_LDADD = ../src/libvirt.la ../lib/libgnu.la
Shouldn't we be using LIBOBJS (automake feature) here? I kind of thought this was how gnulib worked, and it was what I used for the previous getaddrinfo implementation.
+#include "getaddrinfo.h"
This include doesn't need to be conditional? I checked the rest of the patch, and it's fine. Unfortunately since I reinstalled Windows (for like, the 3rd time) yesterday I can't test it there yet, but will do later. I'd very much like to see something along these lines go in. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
Richard W.M. Jones wrote:
Jim Meyering wrote:
-libvirtd_LDADD = ../src/libvirt.la +libvirtd_LDADD = ../src/libvirt.la ../lib/libgnu.la
Shouldn't we be using LIBOBJS (automake feature) here? I kind of thought this was how gnulib worked, and it was what I used for the previous getaddrinfo implementation.
In fact, doesn't this mean there'll be another shared library built (and even worse, one just called "gnu")? Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
"Richard W.M. Jones" <rjones@redhat.com> wrote:
Richard W.M. Jones wrote:
Jim Meyering wrote:
-libvirtd_LDADD = ../src/libvirt.la +libvirtd_LDADD = ../src/libvirt.la ../lib/libgnu.la
Shouldn't we be using LIBOBJS (automake feature) here? I kind of thought this was how gnulib worked, and it was what I used for the previous getaddrinfo implementation.
In fact, doesn't this mean there'll be another shared library built (and even worse, one just called "gnu")?
It won't be installed. It's what automake documentation calls a convenience library.
"Richard W.M. Jones" <rjones@redhat.com> wrote: ...
-EXTRA_DIST = compiler-flags.m4
Does this mean that compiler-flags.m4 is no longer going into the tarball? Shouldn't it?
Hi Rich, It's fine to remove that, because all .m4 files that are used are included automatically. FYI, I did ensure that "make distcheck" passed before posting that patch :-)
-libvirtd_LDADD = ../src/libvirt.la +libvirtd_LDADD = ../src/libvirt.la ../lib/libgnu.la
Shouldn't we be using LIBOBJS (automake feature) here? I kind of thought this was how gnulib worked, and it was what I used for the previous getaddrinfo implementation.
You shouldn't modify LIBOBJS directly. Normally, you put source (e.g., *.[ch]) names in SOURCES or program_name_SOURCES and automake automatically derives $(LIBOBJS) based on that. For library names, you should use LDADD or library_name_LDADD.
+#include "getaddrinfo.h"
This include doesn't need to be conditional?
No. The getaddrinfo module includes both getaddrinfo.c and getaddrinfo.h.
I checked the rest of the patch, and it's fine. Unfortunately since I reinstalled Windows (for like, the 3rd time) yesterday I can't test it there yet, but will do later.
Thanks for the review.
On Wed, Dec 05, 2007 at 12:33:24AM +0100, Jim Meyering wrote:
Recently, I heard of two tricky portability problems in libvirt that are easy to solve with gnulib. Of course, gnulib provides a lot more, and is not exactly lightweight if you count "lines of code imported", but once the framework (this patch) is installed, adding an additional module is as easy as adding the module name to a list. Keep in mind that what matters with a portability library is that it stay out of your way -- and of course do its job well. To that end, gnulib has many per-module unit [...] The first portability problem was to determine the total physical memory available on the current system. Currently the code works only on Linux-like systems that have /proc/meminfo of an expected form. However, the gnulib physmem module handles 13 distinct types of systems and is well tested:
http://www.gnu.org/software/gnulib/MODULES.html#module=physmem
Right a portability solution for this is badly needed
The second portability problem was to find a robust and LGPL-compatible getaddrinfo function to be used on systems lacking it. Here's the gnulib module:
http://www.gnu.org/software/gnulib/MODULES.html#module=getaddrinfo
yes that's needed too for Windows now.
Once the few gnulib hooks are in your configure.ac and Makefile.am files, there is very little extra work required to use the new functions. In the case of physmem, just use the function and include "physmem.h". For getaddrinfo, merely include "getaddrinfo.h" from the two files that use the function.
This change brings in a lot of code, but many of the lib/.[ch] files are used only on systems that lack some required functionality. For example, the getaddrinfo.c file isn't even compiled when it's not needed.
My main concerns were over: - an 'invasion' of a lot of different code in libvirt own code base and the associated maintainance duties - Licencing problems, we are LGPLv2 and should be very careful to not import in the library code which would break it even inadvertently
In the patch below, I've included a new script called bootstrap. It is a wrapper around gnulib-tool that pulls into libvirt the
Like Dan I don't like this too much, we should have the files in CVS and not require dynamic fetching. Moreover this just fails for me here after applying the patch, the final patch should include everything needed to ./autogen.sh ; make ; make check both from a CVS checkout and from a tarball without assuming network connection. paphio:~/libvirt -> sh bootstrap bootstrap: getting gnulib files... bootstrap: line 50: git: command not found bootstrap: line 51: cleanup_gnulib: command not found bootstrap: line 59: gnulib/gnulib-tool: No such file or directory paphio:~/libvirt ->
files selected by the (currently two) modules in use. Those new files go in three places:
m4/*.m4 lib/*.[ch] and a few template .h.in files
Hum, if lib/ ends up containing only files coming from gnulib it's probably better to name it after it, that way it will be clear where this originates from
gl-tests/ for unit test C programs and Bourne shell scripts
should be in tests/gnulib/ or something similar, I concur with Dan on this,
However, note that gettextize and libtoolize (run by autogen.sh) also deposit many *.m4 files in m4. I compared and found that 8 of the files that are already pulled in by the *ize programs are also pulled in (potentially newer versions) from gnulib. But currently, using gettext-0.16.1 or gettext-0.17, there is no difference in any of the overlapping files.
We need to find a way, Dan suggested a mechanism,
Re Licenses: the two modules (and all of their dependent modules) are LGPL-compatible. This is enforced by running gnulib-tool with the --lgpl option. If you were to request a module with an incompatible license (say GPL or LGPLv3), it would fail.
Can we make qa static check at commit time ? We really need everything in CVS and checking individually all added files before commit sounds the right way to make sure there is no problem.
---------------------- Here's the patch that shows what existing parts of libvirt have to be modified to use these two new modules. To try it out, just apply the patch and then run this:
./autogen.sh && ./bootstrap && make && make check
Running bootstrap creates the new lib/ and gl-tests/ directories.
well it fails for me, but the patch applies fine.
Personally, I prefer not to add generated files to version control systems, because it can lead to problems with version skew if all developers don't use the same releases of the tools that do the generating. Perhaps more importantly, when there are massive diffs in the generated files, that can obscure real changes in non-generated parts of the code. That already happens to me whenever the .po files change.
But if people prefer to add all of these imported files to CVS, just say the word and I'll prepare the patch. If so, do you guys want the gettextize- and libtoolize-added files to be version-controlled, now, too?
Basically everything needed for autogen/build/check/install whould be in CVS. I will provide a finer grained review of the patch, hopefully it doesn't really change existing code, it's more glue at the autogen/configure.in and Makefiles level, trying to review the C files of gnulib may not be very productive (except for the Licence check which really need to be done individually). Thanks a lot for looking at this, I was initially rather dubious because of the size of the addition for the relatively few things being fixed a priori. It looks safe though (once all files are indvidually checked for licencing) and can probably help further as libvirt get ported to other things. One thing I really wonder though is suppot of that code when using Microsoft compilers on the various Windows platforms. Are those part of the target for gnulib. That's in my experience with libxml2 some of the place where the most tricky problems could be reported, so if this could help there theis would be a big argument for gnulib inclusion, thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard <veillard@redhat.com> wrote: ...
This change brings in a lot of code, but many of the lib/.[ch] files are used only on systems that lack some required functionality. For example, the getaddrinfo.c file isn't even compiled when it's not needed.
My main concerns were over: - an 'invasion' of a lot of different code in libvirt own code base and the associated maintainance duties - Licencing problems, we are LGPLv2 and should be very careful to not import in the library code which would break it even inadvertently
In the patch below, I've included a new script called bootstrap. It is a wrapper around gnulib-tool that pulls into libvirt the
Like Dan I don't like this too much, we should have the files in CVS and not require dynamic fetching. Moreover this just fails for me here after applying the patch, the final patch should include everything needed to ./autogen.sh ; make ; make check both from a CVS checkout and from a tarball without assuming network connection.
paphio:~/libvirt -> sh bootstrap bootstrap: getting gnulib files... bootstrap: line 50: git: command not found bootstrap: line 51: cleanup_gnulib: command not found bootstrap: line 59: gnulib/gnulib-tool: No such file or directory paphio:~/libvirt ->
Yes, the script requires that you have "git" installed. However, with you three against my one, we don't have to worry about this anymore :-) I'll continue to include the bootstrap script as the reference for how to import or update.
files selected by the (currently two) modules in use. Those new files go in three places:
m4/*.m4 lib/*.[ch] and a few template .h.in files
Hum, if lib/ ends up containing only files coming from gnulib it's probably better to name it after it, that way it will be clear where this originates from
gl-tests/ for unit test C programs and Bourne shell scripts
should be in tests/gnulib/ or something similar, I concur with Dan on this,
I'm easy on the names. I've adjusted to use these: gnulib/m4 gnulib/lib tests/gnulib
However, note that gettextize and libtoolize (run by autogen.sh) also deposit many *.m4 files in m4. I compared and found that 8 of the files that are already pulled in by the *ize programs are also pulled in (potentially newer versions) from gnulib. But currently, using gettext-0.16.1 or gettext-0.17, there is no difference in any of the overlapping files.
We need to find a way, Dan suggested a mechanism,
Re Licenses: the two modules (and all of their dependent modules) are LGPL-compatible. This is enforced by running gnulib-tool with the --lgpl option. If you were to request a module with an incompatible license (say GPL or LGPLv3), it would fail.
Can we make qa static check at commit time ? We really need everything in CVS and checking individually all added files before commit sounds the right way to make sure there is no problem.
---------------------- Here's the patch that shows what existing parts of libvirt have to be modified to use these two new modules. To try it out, just apply the patch and then run this:
./autogen.sh && ./bootstrap && make && make check
Running bootstrap creates the new lib/ and gl-tests/ directories.
well it fails for me, but the patch applies fine.
It should work if you install git.
Personally, I prefer not to add generated files to version control systems, because it can lead to problems with version skew if all developers don't use the same releases of the tools that do the generating. Perhaps more importantly, when there are massive diffs in the generated files, that can obscure real changes in non-generated parts of the code. That already happens to me whenever the .po files change.
But if people prefer to add all of these imported files to CVS, just say the word and I'll prepare the patch. If so, do you guys want the gettextize- and libtoolize-added files to be version-controlled, now, too?
Basically everything needed for autogen/build/check/install whould be in CVS.
Yep. I get the message :-) ...
One thing I really wonder though is suppot of that code when using Microsoft compilers on the various Windows platforms. Are those part of the target for gnulib. That's in my experience with libxml2 some of the place where the most tricky problems could be reported, so if this could help there theis would be a big argument for gnulib inclusion,
The compilers are less of a problem than the environment. We encourage people to use cygwin or mingw, since they're closer to being POSIX-compliant than the M$ environment. More and more people are discovering that gnulib works for Windows too, so there are more contributors. While the primary goal is to make things portable for Unix-based systems, we have never turned down high-quality patches.
More modules we could use (all for MinGW / Windows): - - - - vasprintf For asprintf which we use all over the place. - gettext MinGW lacks <libintl.h> and any gettext function. I thought that gettextize was supposed to supply all the code needed to implement these? Anyhow, it doesn't seem to. - strndup & strsep Non-standard functions, used all over. - fcntl [LGPLv3] The gnulib one fixes all the flags which are missing from Windows - sys_stat As above, but for S_* (Unix modes) macros - readlink [LGPLv3] This explicitly acts as a substitute for this system call on MinGW, according to the documentation anyway. - poll Only applicable if we want to build libvirtd on Windows, which isn't very useful at the moment. - - - The above would fix about 80-90% of the porting problems with MinGW, enabling us to have a natively compiled Windows binary and DLL. All of the modules mentioned above are 'LGPLv2' except where mentioned. I haven't checked the dependencies in detail however. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
"Richard W.M. Jones" <rjones@redhat.com> wrote:
More modules we could use (all for MinGW / Windows): ... - gettext
MinGW lacks <libintl.h> and any gettext function. I thought that gettextize was supposed to supply all the code needed to implement these? Anyhow, it doesn't seem to.
Hi Rich, That just means you'll have to install gettext there. That will provide things like the libintl library and the missing header files. Note that some projects used to include an intl/ directory, and that gave them libintl.a as well as libintl.h as a result of the usual ./configure && make process: i.e., no need for an external library. However, these days, it seems that most projects prefer not to include all of gettext's intl/ directory.
- strndup & strsep
stpcpy is another useful one (though currently not used in libvirt). Lets you do successive string concatenation cleanly and without the usual O(N^2) penalty that you get when doing it via strcat.
- fcntl [LGPLv3]
The gnulib one fixes all the flags which are missing from Windows
- sys_stat
As above, but for S_* (Unix modes) macros
- readlink [LGPLv3]
This explicitly acts as a substitute for this system call on MinGW, according to the documentation anyway.
This is such a thin wrapper that it should be easy to justify changing to LGPLv2.
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering -
Richard W.M. Jones