[PATCH 00/10] Solaris support

[PATCH 00/10] Solaris support

[Blake Jones]

Hi all,

This patch series fixes several issues which are needed to allow notmuch
to build on Solaris 11.  I've been "testing" it for a month or so by
using Karel Zak's fork of mutt along with a copy of notmuch-0.13.2 that
I got to compile.  After a friend asked for a copy of my setup, I
decided to try to get my patches cleaned up enough to submit, so that he
wouldn't need to deal with them if he wanted to hack on it.

I don't have access to any machines running a modern version of Linux
(much less *BSD), so I haven't been able to check whether these changes
have broken any other platforms.  I've tried to follow the prevailing
idiom of compatibility checks and conditionally-set variables, to
minimize the impact on other platforms, but it's possible that I
overlooked something.

I'm really pleased with my experience using notmuch; using it along with
the fork of mutt has given me real control of my email for the first
time in many years.  So I'm happy to offer these changes up in the hopes
that they can be useful to others.

I'm not subscribed to the mailing list, so please CC me on any comments.
I'll also try to watch the web archives of the list just in case.

Blake

[PATCH 01/10] getpwuid: check for standards compliance (Solaris support)

[Blake Jones]

Add checks to "configure" to see whether _POSIX_PTHREAD_SEMANTICS needs
to be defined to get the right number of arguments in the prototypes for
getpwuid_r().  Solaris' default implementation conforms to POSIX.1c
Draft 6, rather than the final POSIX.1c spec.  The standards-compliant
version can be used by defining _POSIX_PTHREAD_SEMANTICS.

This change also adds the file "compat/check_getpwuid.c", which
configure uses to perform its check, and modifies compat/compat.h to
define _POSIX_PTHREAD_SEMANTICS if configure detected it was needed.
---
 compat/check_getpwuid.c |   10 ++++++++++
 compat/compat.h         |    4 ++++
 configure               |   23 +++++++++++++++++++++--
 3 files changed, 35 insertions(+), 2 deletions(-)
 create mode 100644 compat/check_getpwuid.c

diff --git a/compat/check_getpwuid.c b/compat/check_getpwuid.c
new file mode 100644
index 0000000..5da2590
--- /dev/null
+++ b/compat/check_getpwuid.c
@@ -0,0 +1,10 @@
+#include <pwd.h>
+
+int main()
+{
+    struct passwd passwd, *ignored;
+
+    (void) getpwuid_r (0, &passwd, NULL, 0, &ignored);
+
+    return (0);
+}
diff --git a/compat/compat.h b/compat/compat.h
index b2e2736..8374d2f 100644
--- a/compat/compat.h
+++ b/compat/compat.h
@@ -54,6 +54,10 @@ char* strcasestr(const char *haystack, const char *needle);
 #define IGNORE_RESULT(x) x
 #endif /* __GNUC__ */
 
+#if !STD_GETPWUID
+#define _POSIX_PTHREAD_SEMANTICS
+#endif
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/configure b/configure
index ea8a1ad..3c18a45 100755
--- a/configure
+++ b/configure
@@ -512,6 +512,17 @@ else
 fi
 rm -f compat/have_strcasestr
 
+printf "Checking for standard version of getpwuid_r... "
+if ${CC} -o compat/check_getpwuid "$srcdir"/compat/check_getpwuid.c > /dev/null 2>&1
+then
+    printf "Yes.\n"
+    std_getpwuid=1
+else
+    printf "No (will define _POSIX_PTHREAD_SEMANTICS to get it).\n"
+    std_getpwuid=0
+fi
+rm -f compat/check_getpwuid
+
 printf "int main(void){return 0;}\n" > minimal.c
 
 printf "Checking for rpath support... "
@@ -671,6 +682,11 @@ HAVE_GETLINE = ${have_getline}
 # build its own version)
 HAVE_STRCASESTR = ${have_strcasestr}
 
+# Whether the getpwuid_r function is standards-compliant
+# (if not, then notmuch will compile with -D_POSIX_PTHREAD_SEMANTICS
+# to enable the standards-compliant version -- needed for Solaris)
+STD_GETPWUID = ${std_getpwuid}
+
 # Supported platforms (so far) are: LINUX, MACOSX, SOLARIS, FREEBSD, OPENBSD
 PLATFORM = ${platform}
 
@@ -715,10 +731,13 @@ WITH_ZSH = ${WITH_ZSH}
 # Combined flags for compiling and linking against all of the above
 CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
 		   \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND)   \\
-		   \$(VALGRIND_CFLAGS) -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)
+		   \$(VALGRIND_CFLAGS)                                   \\
+		   -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)                 \\
+		   -DSTD_GETPWUID=\$(STD_GETPWUID)
 CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
 		     \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND) \\
 		     \$(VALGRIND_CFLAGS) \$(XAPIAN_CXXFLAGS)             \\
-                     -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)
+		     -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)               \\
+		     -DSTD_GETPWUID=\$(STD_GETPWUID)
 CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS)
 EOF
-- 
1.7.9.2

[PATCH 02/10] asctime: check for standards compliance (Solaris support)

[Blake Jones]

Add checks to "configure" to see whether _POSIX_PTHREAD_SEMANTICS needs
to be defined to get the right number of arguments in the prototypes for
asctime_r().  Solaris' default implementation conforms to POSIX.1c
Draft 6, rather than the final POSIX.1c spec.  The standards-compliant
version can be used by defining _POSIX_PTHREAD_SEMANTICS.

This change also adds the file "compat/check_asctime.c", which
configure uses to perform its check, and modifies compat/compat.h to
define _POSIX_PTHREAD_SEMANTICS if configure detected it was needed.
---
 compat/check_asctime.c |   17 +++++++++++++++++
 compat/compat.h        |    3 +++
 configure              |   22 ++++++++++++++++++++--
 3 files changed, 40 insertions(+), 2 deletions(-)
 create mode 100644 compat/check_asctime.c

diff --git a/compat/check_asctime.c b/compat/check_asctime.c
new file mode 100644
index 0000000..a595110
--- /dev/null
+++ b/compat/check_asctime.c
@@ -0,0 +1,17 @@
+/*
+ * This compatibility check actually succeeds (on Solaris) if
+ * _POSIX_PTHREAD_SEMANTICS is not defined.  But we need to define that to get
+ * the right version of getpwuid_r(), so we define it here to ensure that the
+ * compatibility check ends up doing the same thing as the rest of the code.
+ */
+#define	_POSIX_PTHREAD_SEMANTICS
+#include <time.h>
+
+int main()
+{
+    struct tm tm;
+
+    (void) asctime_r (&tm, NULL, 0);
+
+    return (0);
+}
diff --git a/compat/compat.h b/compat/compat.h
index 8374d2f..b750501 100644
--- a/compat/compat.h
+++ b/compat/compat.h
@@ -57,6 +57,9 @@ char* strcasestr(const char *haystack, const char *needle);
 #if !STD_GETPWUID
 #define _POSIX_PTHREAD_SEMANTICS
 #endif
+#if !STD_ASCTIME
+#define _POSIX_PTHREAD_SEMANTICS
+#endif
 
 #ifdef __cplusplus
 }
diff --git a/configure b/configure
index 3c18a45..047c011 100755
--- a/configure
+++ b/configure
@@ -523,6 +523,17 @@ else
 fi
 rm -f compat/check_getpwuid
 
+printf "Checking for standard version of asctime_r... "
+if ${CC} -o compat/check_asctime "$srcdir"/compat/check_asctime.c > /dev/null 2>&1
+then
+    printf "Yes.\n"
+    std_asctime=1
+else
+    printf "No (will define _POSIX_PTHREAD_SEMANTICS to get it).\n"
+    std_asctime=0
+fi
+rm -f compat/check_asctime
+
 printf "int main(void){return 0;}\n" > minimal.c
 
 printf "Checking for rpath support... "
@@ -687,6 +698,11 @@ HAVE_STRCASESTR = ${have_strcasestr}
 # to enable the standards-compliant version -- needed for Solaris)
 STD_GETPWUID = ${std_getpwuid}
 
+# Whether the asctime_r function is standards-compliant
+# (if not, then notmuch will compile with -D_POSIX_PTHREAD_SEMANTICS
+# to enable the standards-compliant version -- needed for Solaris)
+STD_ASCTIME = ${std_asctime}
+
 # Supported platforms (so far) are: LINUX, MACOSX, SOLARIS, FREEBSD, OPENBSD
 PLATFORM = ${platform}
 
@@ -733,11 +749,13 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
 		   \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND)   \\
 		   \$(VALGRIND_CFLAGS)                                   \\
 		   -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)                 \\
-		   -DSTD_GETPWUID=\$(STD_GETPWUID)
+		   -DSTD_GETPWUID=\$(STD_GETPWUID)                       \\
+		   -DSTD_ASCTIME=\$(STD_ASCTIME)
 CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
 		     \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND) \\
 		     \$(VALGRIND_CFLAGS) \$(XAPIAN_CXXFLAGS)             \\
 		     -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)               \\
-		     -DSTD_GETPWUID=\$(STD_GETPWUID)
+		     -DSTD_GETPWUID=\$(STD_GETPWUID)                     \\
+		     -DSTD_ASCTIME=\$(STD_ASCTIME)
 CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS)
 EOF
-- 
1.7.9.2

[PATCH 03/10] gethostbyname: check for libnsl (Solaris support)

[Blake Jones]

Add a check to "configure" to see whether -lnsl is needed for programs
that are using gethostbyname().  This change also adds the file
"compat/check_ghbn.c", which configure uses to perform its check.
---
 compat/check_ghbn.c |    8 ++++++++
 configure           |   17 ++++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)
 create mode 100644 compat/check_ghbn.c

diff --git a/compat/check_ghbn.c b/compat/check_ghbn.c
new file mode 100644
index 0000000..ec5f227
--- /dev/null
+++ b/compat/check_ghbn.c
@@ -0,0 +1,8 @@
+#include <netdb.h>
+
+int main()
+{
+    (void) gethostbyname(NULL);
+
+    return (0);
+}
diff --git a/configure b/configure
index 047c011..28d4110 100755
--- a/configure
+++ b/configure
@@ -534,6 +534,17 @@ else
 fi
 rm -f compat/check_asctime
 
+printf "Checking whether libnsl is needed for gethostbyname... "
+if ${CC} -o compat/check_ghbn "$srcdir"/compat/check_ghbn.c > /dev/null 2>&1
+then
+    printf "No.\n"
+    libnsl_ldflags=""
+else
+    printf "Yes.\n"
+    libnsl_ldflags="-lnsl"
+fi
+rm -f compat/check_ghbn
+
 printf "int main(void){return 0;}\n" > minimal.c
 
 printf "Checking for rpath support... "
@@ -723,6 +734,9 @@ GMIME_LDFLAGS = ${gmime_ldflags}
 TALLOC_CFLAGS = ${talloc_cflags}
 TALLOC_LDFLAGS = ${talloc_ldflags}
 
+# Flags needed to get gethostbyname() at link time
+LIBNSL_LDFLAGS = ${libnsl_ldflags}
+
 # Flags needed to have linker set rpath attribute
 RPATH_LDFLAGS = ${rpath_ldflags}
 
@@ -757,5 +771,6 @@ CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
 		     -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)               \\
 		     -DSTD_GETPWUID=\$(STD_GETPWUID)                     \\
 		     -DSTD_ASCTIME=\$(STD_ASCTIME)
-CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS)
+CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS) \\
+		     \$(LIBNSL_LDFLAGS)
 EOF
-- 
1.7.9.2

[PATCH 04/10] configure: check for -Wl,-rpath (Solaris support)

[Blake Jones]

Add a check to "configure" to see whether -Wl,-rpath can be used without
--enable-new-dtags.  Solaris needs the former and doesn't know about the
latter.
---
 configure |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configure b/configure
index 28d4110..5c5139f 100755
--- a/configure
+++ b/configure
@@ -552,6 +552,10 @@ if ${CC} -Wl,--enable-new-dtags -Wl,-rpath,/tmp/ -o minimal minimal.c >/dev/null
 then
     printf "Yes.\n"
     rpath_ldflags="-Wl,--enable-new-dtags -Wl,-rpath,\$(libdir)"
+elif ${CC} -Wl,-rpath,/tmp/ -o minimal minimal.c >/dev/null 2>&1
+then
+    printf "Yes.\n"
+    rpath_ldflags="-Wl,-rpath,\$(libdir)"
 else
     printf "No (nothing to worry about).\n"
     rpath_ldflags=""
-- 
1.7.9.2

[PATCH 05/10] install: check for non-SysV version (Solaris support)

[Blake Jones]

Solaris ships a program called "install" in /usr/sbin, which performs a
task that's fairly similar to the GNU and BSD "install" programs but
which uses very different command line arguments.  In particular, if it
is invoked without "-c", "-f", or "-n", it will search the target
directory for a file with the same name as the one being installed, and
it will only install the file if it finds a matching name.  More
excitingly, if it doesn't find a match, it will look in /bin, /usr/bin,
/etc, /lib, and /usr/lib and try to do the same there.

The standard workaround for this is to use GNU install.
It is available via the standard Solaris packaging system (in
"file/gnu-coreutils"), and installs itself as /usr/bin/ginstall.

This patch adds a check to "configure" to see if "install" behaves in a
way that's compatible with GNU and BSD install, and if not, it uses a
program called "ginstall" instead.  It also modifies "configure" to set
the $(INSTALL) variable, and changes various Makefiles to use it.
---
 Makefile.local            |    2 +-
 completion/Makefile.local |    4 ++--
 configure                 |   19 +++++++++++++++++++
 emacs/Makefile.local      |    6 +++---
 lib/Makefile.local        |    4 ++--
 man/Makefile.local        |    6 +++---
 vim/Makefile              |    6 ++----
 7 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/Makefile.local b/Makefile.local
index 2b91946..7ccb1cd 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -286,7 +286,7 @@ notmuch-shared: $(notmuch_client_modules) lib/$(LINKER_NAME)
 .PHONY: install
 install: all install-man
 	mkdir -p "$(DESTDIR)$(prefix)/bin/"
-	install notmuch-shared "$(DESTDIR)$(prefix)/bin/notmuch"
+	$(INSTALL) notmuch-shared "$(DESTDIR)$(prefix)/bin/notmuch"
 ifeq ($(MAKECMDGOALS), install)
 	@echo ""
 	@echo "Notmuch is now installed to $(DESTDIR)$(prefix)"
diff --git a/completion/Makefile.local b/completion/Makefile.local
index dfc1271..a648a78 100644
--- a/completion/Makefile.local
+++ b/completion/Makefile.local
@@ -14,9 +14,9 @@ install-$(dir):
 	@echo $@
 ifeq ($(WITH_BASH),1)
 	mkdir -p "$(DESTDIR)$(bash_completion_dir)"
-	install -m0644 $(bash_script) "$(DESTDIR)$(bash_completion_dir)/notmuch"
+	$(INSTALL) -m0644 $(bash_script) "$(DESTDIR)$(bash_completion_dir)/notmuch"
 endif
 ifeq ($(WITH_ZSH),1)
 	mkdir -p "$(DESTDIR)$(zsh_completion_dir)"
-	install -m0644 $(zsh_script) "$(DESTDIR)$(zsh_completion_dir)/_notmuch"
+	$(INSTALL) -m0644 $(zsh_script) "$(DESTDIR)$(zsh_completion_dir)/_notmuch"
 endif
diff --git a/configure b/configure
index 5c5139f..dae837e 100755
--- a/configure
+++ b/configure
@@ -591,6 +591,21 @@ for flag in -Wmissing-declarations; do
 done
 printf "\n\t${WARN_CFLAGS}\n"
 
+INSTALL="install"
+printf "Checking for working \"install\" program... "
+mkdir _tmp_
+cd _tmp_
+echo 1 > 1
+mkdir dest
+if install 1 dest > /dev/null 2>&1 ; then
+      printf "\"install\" works fine.\n"
+else
+      INSTALL="ginstall"
+      printf "using \"ginstall\".\n"
+fi
+cd ..
+rm -rf _tmp_
+
 rm -f minimal minimal.c
 
 cat <<EOF
@@ -777,4 +792,8 @@ CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
 		     -DSTD_ASCTIME=\$(STD_ASCTIME)
 CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS) \\
 		     \$(LIBNSL_LDFLAGS)
+
+# Which "install" program to use
+INSTALL = ${INSTALL}
+
 EOF
diff --git a/emacs/Makefile.local b/emacs/Makefile.local
index fb82247..ee778cb 100644
--- a/emacs/Makefile.local
+++ b/emacs/Makefile.local
@@ -36,11 +36,11 @@ endif
 .PHONY: install-emacs
 install-emacs:
 	mkdir -p "$(DESTDIR)$(emacslispdir)"
-	install -m0644 $(emacs_sources) "$(DESTDIR)$(emacslispdir)"
+	$(INSTALL) -m0644 $(emacs_sources) "$(DESTDIR)$(emacslispdir)"
 ifeq ($(HAVE_EMACS),1)
-	install -m0644 $(emacs_bytecode) "$(DESTDIR)$(emacslispdir)"
+	$(INSTALL) -m0644 $(emacs_bytecode) "$(DESTDIR)$(emacslispdir)"
 endif
 	mkdir -p "$(DESTDIR)$(emacsetcdir)"
-	install -m0644 $(emacs_images) "$(DESTDIR)$(emacsetcdir)"
+	$(INSTALL) -m0644 $(emacs_images) "$(DESTDIR)$(emacsetcdir)"
 
 CLEAN := $(CLEAN) $(emacs_bytecode)
diff --git a/lib/Makefile.local b/lib/Makefile.local
index 7785944..0c6b258 100644
--- a/lib/Makefile.local
+++ b/lib/Makefile.local
@@ -89,11 +89,11 @@ install: install-$(dir)
 
 install-$(dir): $(dir)/$(LIBNAME)
 	mkdir -p "$(DESTDIR)$(libdir)/"
-	install -m0644 "$(lib)/$(LIBNAME)" "$(DESTDIR)$(libdir)/"
+	$(INSTALL) -m0644 "$(lib)/$(LIBNAME)" "$(DESTDIR)$(libdir)/"
 	ln -sf $(LIBNAME) "$(DESTDIR)$(libdir)/$(SONAME)"
 	ln -sf $(LIBNAME) "$(DESTDIR)$(libdir)/$(LINKER_NAME)"
 	mkdir -p "$(DESTDIR)$(includedir)"
-	install -m0644 "$(srcdir)/$(lib)/notmuch.h" "$(DESTDIR)$(includedir)/"
+	$(INSTALL) -m0644 "$(srcdir)/$(lib)/notmuch.h" "$(DESTDIR)$(includedir)/"
 	$(LIBRARY_INSTALL_POST_COMMAND)
 
 SRCS  := $(SRCS) $(libnotmuch_c_srcs) $(libnotmuch_cxx_srcs)
diff --git a/man/Makefile.local b/man/Makefile.local
index 72e2a18..07dcf4c 100644
--- a/man/Makefile.local
+++ b/man/Makefile.local
@@ -38,9 +38,9 @@ install-man: $(COMPRESSED_MAN)
 	mkdir -p "$(DESTDIR)$(mandir)/man1"
 	mkdir -p "$(DESTDIR)$(mandir)/man5"
 	mkdir -p "$(DESTDIR)$(mandir)/man7"
-	install -m0644 $(MAN1_GZ) $(DESTDIR)/$(mandir)/man1
-	install -m0644 $(MAN5_GZ) $(DESTDIR)/$(mandir)/man5
-	install -m0644 $(MAN7_GZ) $(DESTDIR)/$(mandir)/man7
+	$(INSTALL) -m0644 $(MAN1_GZ) $(DESTDIR)/$(mandir)/man1
+	$(INSTALL) -m0644 $(MAN5_GZ) $(DESTDIR)/$(mandir)/man5
+	$(INSTALL) -m0644 $(MAN7_GZ) $(DESTDIR)/$(mandir)/man7
 	cd $(DESTDIR)/$(mandir)/man1 && ln -sf notmuch.1.gz notmuch-setup.1.gz
 
 update-man-versions: $(MAN_SOURCE)
diff --git a/vim/Makefile b/vim/Makefile
index f17bebf..7ceba7a 100644
--- a/vim/Makefile
+++ b/vim/Makefile
@@ -5,8 +5,6 @@ files = plugin/notmuch.vim \
 prefix = $(HOME)/.vim
 destdir = $(prefix)/plugin
 
-INSTALL = install -D -m644
-
 all: help
 
 help:
@@ -17,7 +15,7 @@ help:
 	@echo "    make symlink     - create symlinks in ~/.vim (useful for development)"
 
 install:
-	@for x in $(files); do $(INSTALL) $(CURDIR)/$$x $(prefix)/$$x; done
+	@for x in $(files); do $(INSTALL) -D -m644 $(CURDIR)/$$x $(prefix)/$$x; done
 
-link symlink: INSTALL = ln -fs
 link symlink: install
+	@for x in $(files); do ln -fs $(CURDIR)/$$x $(prefix)/$$x; done
-- 
1.7.9.2

Re: [PATCH 05/10] install: check for non-SysV version (Solaris support)

[Jani Nikula]

On Sun, 04 Nov 2012, Blake Jones <blakej@foo.net> wrote:
> Solaris ships a program called "install" in /usr/sbin, which performs a
> task that's fairly similar to the GNU and BSD "install" programs but
> which uses very different command line arguments.  In particular, if it
> is invoked without "-c", "-f", or "-n", it will search the target
> directory for a file with the same name as the one being installed, and
> it will only install the file if it finds a matching name.  More
> excitingly, if it doesn't find a match, it will look in /bin, /usr/bin,
> /etc, /lib, and /usr/lib and try to do the same there.
>
> The standard workaround for this is to use GNU install.
> It is available via the standard Solaris packaging system (in
> "file/gnu-coreutils"), and installs itself as /usr/bin/ginstall.
>
> This patch adds a check to "configure" to see if "install" behaves in a
> way that's compatible with GNU and BSD install, and if not, it uses a
> program called "ginstall" instead.  It also modifies "configure" to set
> the $(INSTALL) variable, and changes various Makefiles to use it.
> ---
>  Makefile.local            |    2 +-
>  completion/Makefile.local |    4 ++--
>  configure                 |   19 +++++++++++++++++++
>  emacs/Makefile.local      |    6 +++---
>  lib/Makefile.local        |    4 ++--
>  man/Makefile.local        |    6 +++---
>  vim/Makefile              |    6 ++----
>  7 files changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/Makefile.local b/Makefile.local
> index 2b91946..7ccb1cd 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -286,7 +286,7 @@ notmuch-shared: $(notmuch_client_modules) lib/$(LINKER_NAME)
>  .PHONY: install
>  install: all install-man
>  	mkdir -p "$(DESTDIR)$(prefix)/bin/"
> -	install notmuch-shared "$(DESTDIR)$(prefix)/bin/notmuch"
> +	$(INSTALL) notmuch-shared "$(DESTDIR)$(prefix)/bin/notmuch"
>  ifeq ($(MAKECMDGOALS), install)
>  	@echo ""
>  	@echo "Notmuch is now installed to $(DESTDIR)$(prefix)"
> diff --git a/completion/Makefile.local b/completion/Makefile.local
> index dfc1271..a648a78 100644
> --- a/completion/Makefile.local
> +++ b/completion/Makefile.local
> @@ -14,9 +14,9 @@ install-$(dir):
>  	@echo $@
>  ifeq ($(WITH_BASH),1)
>  	mkdir -p "$(DESTDIR)$(bash_completion_dir)"
> -	install -m0644 $(bash_script) "$(DESTDIR)$(bash_completion_dir)/notmuch"
> +	$(INSTALL) -m0644 $(bash_script) "$(DESTDIR)$(bash_completion_dir)/notmuch"
>  endif
>  ifeq ($(WITH_ZSH),1)
>  	mkdir -p "$(DESTDIR)$(zsh_completion_dir)"
> -	install -m0644 $(zsh_script) "$(DESTDIR)$(zsh_completion_dir)/_notmuch"
> +	$(INSTALL) -m0644 $(zsh_script) "$(DESTDIR)$(zsh_completion_dir)/_notmuch"
>  endif
> diff --git a/configure b/configure
> index 5c5139f..dae837e 100755
> --- a/configure
> +++ b/configure
> @@ -591,6 +591,21 @@ for flag in -Wmissing-declarations; do
>  done
>  printf "\n\t${WARN_CFLAGS}\n"
>  
> +INSTALL="install"
> +printf "Checking for working \"install\" program... "
> +mkdir _tmp_

This doesn't feel like a hot idea. Don't tell me you'd need to create a
compatibility script for using mktemp --tmpdir too...

Or how about just always using ginstall on Solaris?

BR,
Jani.

> +cd _tmp_
> +echo 1 > 1
> +mkdir dest
> +if install 1 dest > /dev/null 2>&1 ; then
> +      printf "\"install\" works fine.\n"
> +else
> +      INSTALL="ginstall"
> +      printf "using \"ginstall\".\n"
> +fi
> +cd ..
> +rm -rf _tmp_
> +
>  rm -f minimal minimal.c
>  
>  cat <<EOF
> @@ -777,4 +792,8 @@ CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
>  		     -DSTD_ASCTIME=\$(STD_ASCTIME)
>  CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS) \\
>  		     \$(LIBNSL_LDFLAGS)
> +
> +# Which "install" program to use
> +INSTALL = ${INSTALL}
> +
>  EOF
> diff --git a/emacs/Makefile.local b/emacs/Makefile.local
> index fb82247..ee778cb 100644
> --- a/emacs/Makefile.local
> +++ b/emacs/Makefile.local
> @@ -36,11 +36,11 @@ endif
>  .PHONY: install-emacs
>  install-emacs:
>  	mkdir -p "$(DESTDIR)$(emacslispdir)"
> -	install -m0644 $(emacs_sources) "$(DESTDIR)$(emacslispdir)"
> +	$(INSTALL) -m0644 $(emacs_sources) "$(DESTDIR)$(emacslispdir)"
>  ifeq ($(HAVE_EMACS),1)
> -	install -m0644 $(emacs_bytecode) "$(DESTDIR)$(emacslispdir)"
> +	$(INSTALL) -m0644 $(emacs_bytecode) "$(DESTDIR)$(emacslispdir)"
>  endif
>  	mkdir -p "$(DESTDIR)$(emacsetcdir)"
> -	install -m0644 $(emacs_images) "$(DESTDIR)$(emacsetcdir)"
> +	$(INSTALL) -m0644 $(emacs_images) "$(DESTDIR)$(emacsetcdir)"
>  
>  CLEAN := $(CLEAN) $(emacs_bytecode)
> diff --git a/lib/Makefile.local b/lib/Makefile.local
> index 7785944..0c6b258 100644
> --- a/lib/Makefile.local
> +++ b/lib/Makefile.local
> @@ -89,11 +89,11 @@ install: install-$(dir)
>  
>  install-$(dir): $(dir)/$(LIBNAME)
>  	mkdir -p "$(DESTDIR)$(libdir)/"
> -	install -m0644 "$(lib)/$(LIBNAME)" "$(DESTDIR)$(libdir)/"
> +	$(INSTALL) -m0644 "$(lib)/$(LIBNAME)" "$(DESTDIR)$(libdir)/"
>  	ln -sf $(LIBNAME) "$(DESTDIR)$(libdir)/$(SONAME)"
>  	ln -sf $(LIBNAME) "$(DESTDIR)$(libdir)/$(LINKER_NAME)"
>  	mkdir -p "$(DESTDIR)$(includedir)"
> -	install -m0644 "$(srcdir)/$(lib)/notmuch.h" "$(DESTDIR)$(includedir)/"
> +	$(INSTALL) -m0644 "$(srcdir)/$(lib)/notmuch.h" "$(DESTDIR)$(includedir)/"
>  	$(LIBRARY_INSTALL_POST_COMMAND)
>  
>  SRCS  := $(SRCS) $(libnotmuch_c_srcs) $(libnotmuch_cxx_srcs)
> diff --git a/man/Makefile.local b/man/Makefile.local
> index 72e2a18..07dcf4c 100644
> --- a/man/Makefile.local
> +++ b/man/Makefile.local
> @@ -38,9 +38,9 @@ install-man: $(COMPRESSED_MAN)
>  	mkdir -p "$(DESTDIR)$(mandir)/man1"
>  	mkdir -p "$(DESTDIR)$(mandir)/man5"
>  	mkdir -p "$(DESTDIR)$(mandir)/man7"
> -	install -m0644 $(MAN1_GZ) $(DESTDIR)/$(mandir)/man1
> -	install -m0644 $(MAN5_GZ) $(DESTDIR)/$(mandir)/man5
> -	install -m0644 $(MAN7_GZ) $(DESTDIR)/$(mandir)/man7
> +	$(INSTALL) -m0644 $(MAN1_GZ) $(DESTDIR)/$(mandir)/man1
> +	$(INSTALL) -m0644 $(MAN5_GZ) $(DESTDIR)/$(mandir)/man5
> +	$(INSTALL) -m0644 $(MAN7_GZ) $(DESTDIR)/$(mandir)/man7
>  	cd $(DESTDIR)/$(mandir)/man1 && ln -sf notmuch.1.gz notmuch-setup.1.gz
>  
>  update-man-versions: $(MAN_SOURCE)
> diff --git a/vim/Makefile b/vim/Makefile
> index f17bebf..7ceba7a 100644
> --- a/vim/Makefile
> +++ b/vim/Makefile
> @@ -5,8 +5,6 @@ files = plugin/notmuch.vim \
>  prefix = $(HOME)/.vim
>  destdir = $(prefix)/plugin
>  
> -INSTALL = install -D -m644
> -
>  all: help
>  
>  help:
> @@ -17,7 +15,7 @@ help:
>  	@echo "    make symlink     - create symlinks in ~/.vim (useful for development)"
>  
>  install:
> -	@for x in $(files); do $(INSTALL) $(CURDIR)/$$x $(prefix)/$$x; done
> +	@for x in $(files); do $(INSTALL) -D -m644 $(CURDIR)/$$x $(prefix)/$$x; done
>  
> -link symlink: INSTALL = ln -fs
>  link symlink: install
> +	@for x in $(files); do ln -fs $(CURDIR)/$$x $(prefix)/$$x; done
> -- 
> 1.7.9.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 05/10] install: check for non-SysV version (Solaris support)

[Blake Jones]

> > +INSTALL="install"
> > +printf "Checking for working \"install\" program... "
> > +mkdir _tmp_
> 
> This doesn't feel like a hot idea.

Out of curiosity, why not?  An "install" that behaves as expected is
one of the first things that an autoconf-generated "configure" looks
for.  Now, autoconf-configure implements that check using some
assumptions about where things are on different operating systems,
but that sort of check runs the risk of becoming stale (see below).

> Don't tell me you'd need to create a compatibility script for using
> mktemp --tmpdir too...

Yes I would, if it were used; not all the world's a GNU.  But in the
case of mktemp, the widely available "-p" and "-t" options seem to get
you most of the way there.  The SVR4 "install" in Solaris' /usr/sbin is
different enough from the BSD/GNU versions that I wouldn't want to try
to emulate it with a wrapper.

> Or how about just always using ginstall on Solaris?

I'd rather not do that.  With the old UCB tools having been EOL'ed [1],
/usr/ucb/install (which would have worked) will be going away.  There is
an open bug in the Sun bug tracking system about moving GNU install to
/usr/bin/install, specifically motivated by this change.  So while I
don't know if/when that bug will be fixed, I would guess [2] that a
future release of Solaris may have a BSD/GNU-compatible version of
"install" in the default $PATH.

Blake

[1] http://www.oracle.com/technetwork/systems/end-of-notices/eonsolaris11-392732.html
[2] Caveat: I work for Oracle in the Solaris kernel group, but I am not
    speaking for my employer.

Re: [PATCH 05/10] install: check for non-SysV version (Solaris support)

[Jani Nikula]

On Mon, 05 Nov 2012, Blake Jones <blakej@foo.net> wrote:
>> > +INSTALL="install"
>> > +printf "Checking for working \"install\" program... "
>> > +mkdir _tmp_
>> 
>> This doesn't feel like a hot idea.
>
> Out of curiosity, why not?

Note that I'm only referring to creating temp directories like this to
check for the install tool compatibility; otherwise I'm fine with the
general approach.

Jani.

Re: [PATCH 05/10] install: check for non-SysV version (Solaris support)

[Blake Jones]

> On Mon, 05 Nov 2012, Blake Jones <blakej@foo.net> wrote:
> >> > +INSTALL="install"
> >> > +printf "Checking for working \"install\" program... "
> >> > +mkdir _tmp_
> >> 
> >> This doesn't feel like a hot idea.
> >
> > Out of curiosity, why not?
> 
> Note that I'm only referring to creating temp directories like this to
> check for the install tool compatibility; otherwise I'm fine with the
> general approach.

Sure.  But what's wrong with creating a temp directory?  The configure
script creates plenty of temp files for testing compiler features and
library symbol availability.

Blake

[PATCH 06/10] strsep: check for availability (Solaris support)

[Blake Jones]

Solaris does not ship a version of the strsep() function.  This change
adds a check to "configure" to see whether notmuch needs to provide its
own implementation, and if so, it uses the new version in
"compat/strsep.c" (which was copied from Mutt, and apparently before
that from glibc).
---
 compat/Makefile.local |    4 +++
 compat/compat.h       |    4 +++
 compat/have_strsep.c  |   10 ++++++++
 compat/strsep.c       |   65 +++++++++++++++++++++++++++++++++++++++++++++++++
 configure             |   17 +++++++++++++
 5 files changed, 100 insertions(+)
 create mode 100644 compat/have_strsep.c
 create mode 100644 compat/strsep.c

diff --git a/compat/Makefile.local b/compat/Makefile.local
index 13f16cd..2c4f65f 100644
--- a/compat/Makefile.local
+++ b/compat/Makefile.local
@@ -13,4 +13,8 @@ ifneq ($(HAVE_STRCASESTR),1)
 notmuch_compat_srcs += $(dir)/strcasestr.c
 endif
 
+ifneq ($(HAVE_STRSEP),1)
+notmuch_compat_srcs += $(dir)/strsep.c
+endif
+
 SRCS := $(SRCS) $(notmuch_compat_srcs)
diff --git a/compat/compat.h b/compat/compat.h
index b750501..1fd2723 100644
--- a/compat/compat.h
+++ b/compat/compat.h
@@ -46,6 +46,10 @@ getdelim (char **lineptr, size_t *n, int delimiter, FILE *fp);
 char* strcasestr(const char *haystack, const char *needle);
 #endif /* !HAVE_STRCASESTR */
 
+#if !HAVE_STRSEP
+char *strsep(char **stringp, const char *delim);
+#endif /* !HAVE_STRSEP */
+
 /* Silence gcc warnings about unused results.  These warnings exist
  * for a reason; any use of this needs to be justified. */
 #ifdef __GNUC__
diff --git a/compat/have_strsep.c b/compat/have_strsep.c
new file mode 100644
index 0000000..5bd396c
--- /dev/null
+++ b/compat/have_strsep.c
@@ -0,0 +1,10 @@
+#define _GNU_SOURCE
+#include <string.h>
+
+int main()
+{
+    char *found;
+    char **stringp, const char *delim;
+
+    found = strsep(stringp, delim);
+}
diff --git a/compat/strsep.c b/compat/strsep.c
new file mode 100644
index 0000000..78ab9e7
--- /dev/null
+++ b/compat/strsep.c
@@ -0,0 +1,65 @@
+/* Copyright (C) 1992, 93, 96, 97, 98, 99, 2004 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, write to the Free
+   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+   02111-1307 USA.  */
+
+#include <string.h>
+
+/* Taken from glibc 2.6.1 */
+
+char *strsep (char **stringp, const char *delim)
+{
+  char *begin, *end;
+
+  begin = *stringp;
+  if (begin == NULL)
+    return NULL;
+
+  /* A frequent case is when the delimiter string contains only one
+     character.  Here we don't need to call the expensive `strpbrk'
+     function and instead work using `strchr'.  */
+  if (delim[0] == '\0' || delim[1] == '\0')
+    {
+      char ch = delim[0];
+
+      if (ch == '\0')
+	end = NULL;
+      else
+	{
+	  if (*begin == ch)
+	    end = begin;
+	  else if (*begin == '\0')
+	    end = NULL;
+	  else
+	    end = strchr (begin + 1, ch);
+	}
+    }
+  else
+    /* Find the end of the token.  */
+    end = strpbrk (begin, delim);
+
+  if (end)
+    {
+      /* Terminate the token and set *STRINGP past NUL character.  */
+      *end++ = '\0';
+      *stringp = end;
+    }
+  else
+    /* No more delimiters; this is the last token.  */
+    *stringp = NULL;
+
+  return begin;
+}
diff --git a/configure b/configure
index dae837e..e519abc 100755
--- a/configure
+++ b/configure
@@ -512,6 +512,17 @@ else
 fi
 rm -f compat/have_strcasestr
 
+printf "Checking for strsep... "
+if ${CC} -o compat/have_strsep "$srcdir"/compat/have_strsep.c > /dev/null 2>&1
+then
+    printf "Yes.\n"
+    have_strsep="1"
+else
+    printf "No (will use our own instead).\n"
+    have_strsep="0"
+fi
+rm -f compat/have_strsep
+
 printf "Checking for standard version of getpwuid_r... "
 if ${CC} -o compat/check_getpwuid "$srcdir"/compat/check_getpwuid.c > /dev/null 2>&1
 then
@@ -723,6 +734,10 @@ HAVE_GETLINE = ${have_getline}
 # build its own version)
 HAVE_STRCASESTR = ${have_strcasestr}
 
+# Whether the strsep function is available (if not, then notmuch will
+# build its own version)
+HAVE_STRSEP = ${have_strsep}
+
 # Whether the getpwuid_r function is standards-compliant
 # (if not, then notmuch will compile with -D_POSIX_PTHREAD_SEMANTICS
 # to enable the standards-compliant version -- needed for Solaris)
@@ -782,12 +797,14 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
 		   \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND)   \\
 		   \$(VALGRIND_CFLAGS)                                   \\
 		   -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)                 \\
+		   -DHAVE_STRSEP=\$(HAVE_STRSEP)                         \\
 		   -DSTD_GETPWUID=\$(STD_GETPWUID)                       \\
 		   -DSTD_ASCTIME=\$(STD_ASCTIME)
 CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
 		     \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND) \\
 		     \$(VALGRIND_CFLAGS) \$(XAPIAN_CXXFLAGS)             \\
 		     -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)               \\
+		     -DHAVE_STRSEP=\$(HAVE_STRSEP)                       \\
 		     -DSTD_GETPWUID=\$(STD_GETPWUID)                     \\
 		     -DSTD_ASCTIME=\$(STD_ASCTIME)
 CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS) \\
-- 
1.7.9.2

[PATCH 07/10] gen-version-script: parse Solaris "nm" output (Solaris support)

[Blake Jones]

The output of "nm" on Solaris is substantially different from that on
Linux, and the current version of gen-version-script is tied to the
Linux "nm" output.  This patch separates the parts of "nm" processing
which are dependent on the output format into a couple shell functions,
and makes another shell function to use the appropriate version of
"c++filt" to demangle symbols.  It also modifies lib/Makefile.local
to pass the generated symbol table correctly to the Solaris linker.
---
 lib/Makefile.local        |    4 ++++
 lib/gen-version-script.sh |   50 ++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/lib/Makefile.local b/lib/Makefile.local
index 0c6b258..2068e4a 100644
--- a/lib/Makefile.local
+++ b/lib/Makefile.local
@@ -30,7 +30,11 @@ LIBRARY_SUFFIX = so
 LINKER_NAME = libnotmuch.$(LIBRARY_SUFFIX)
 SONAME = $(LINKER_NAME).$(LIBNOTMUCH_VERSION_MAJOR)
 LIBNAME = $(SONAME).$(LIBNOTMUCH_VERSION_MINOR).$(LIBNOTMUCH_VERSION_RELEASE)
+ifeq ($(PLATFORM),SOLARIS)
+LIBRARY_LINK_FLAG = -shared -Wl,-M notmuch.sym -Wl,-soname=$(SONAME) -Wl,--no-undefined -lc
+else
 LIBRARY_LINK_FLAG = -shared -Wl,--version-script=notmuch.sym,-soname=$(SONAME) -Wl,--no-undefined
+endif
 ifeq ($(PLATFORM),OPENBSD)
 LIBRARY_LINK_FLAG += -lc
 endif
diff --git a/lib/gen-version-script.sh b/lib/gen-version-script.sh
index 76670d5..d7d96da 100644
--- a/lib/gen-version-script.sh
+++ b/lib/gen-version-script.sh
@@ -1,3 +1,4 @@
+#!/bin/sh
 
 # we go through a bit of work to get the unmangled names of the
 # typeinfo symbols because of
@@ -11,10 +12,44 @@ fi
 HEADER=$1
 shift
 
+if [ `uname -s` == SunOS ] ; then
+    #
+    # Using Solaris "nm", a defined symbol looks like this:
+    #
+    # [Index]    Value    Size Type  Bind  Other Shndx   Name
+    # [15]    |    128|     16|FUNC |GLOB |0    |1      |notmuch_tags_get
+    #
+    # and an undefined symbol looks like this:
+    #
+    # [Index]    Value    Size Type  Bind  Other Shndx   Name
+    # [35]    |      0|      0|NOTY |GLOB |0    |UNDEF  |notmuch_tags_get
+    #
+    find_xapian_error() {
+	nawk -F'\|' '$7 !~ "UNDEF" && $8 ~ "Xapian.*Error" { print $8 }'
+    }
+    find_compat_syms() {
+	nawk -F'\|' '$7 !~ "UNDEF" && $8 ~ "get(line|delim)" { print $8 ";" }'
+    }
+    demangle() {
+	gc++filt "$@"
+    }
+else
+    find_xapian_error() {
+	awk '$1 ~ "^[0-9a-fA-F][0-9a-fA-F]*$" && $3 ~ "Xapian.*Error" {print $3}'
+    }
+    find_compat_syms() {
+	awk '$1 ~ "^[0-9a-fA-F][0-9a-fA-F]*$" && $2 == "T" && $3 ~ "^get(line|delim)$" {print $3 ";"}'
+    }
+    demangle() {
+	c++filt "$@"
+    }
+fi
+
 printf '{\nglobal:\n'
-nm  $* | awk '$1 ~ "^[0-9a-fA-F][0-9a-fA-F]*$" && $3 ~ "Xapian.*Error" {print $3}' | sort | uniq | \
-while read sym; do
-    demangled=$(c++filt $sym)
+
+# Find the typeinfo for "Xapian::*Error"s.
+nm $* | find_xapian_error | sort | uniq | while read sym; do
+    demangled=$(demangle $sym)
     case $demangled in
 	typeinfo*) 
 	    printf "\t$sym;\n"
@@ -23,6 +58,11 @@ while read sym; do
 	    ;;
     esac
 done
-nm $* | awk '$1 ~ "^[0-9a-fA-F][0-9a-fA-F]*$" && $2 == "T" && $3 ~ "^get(line|delim)$" {print $3 ";"}'
-sed  -n 's/^[[:space:]]*\(notmuch_[a-z_]*\)[[:space:]]*(.*/ \1;/p' $HEADER
+
+# Find the "compat" syms that we need to export.
+nm $* | find_compat_syms
+
+# Finally, get the real notmuch symbols.
+sed -n 's/^[ 	]*\(notmuch_[a-z_]*\)[ 	]*(.*/ \1;/p' $HEADER
+
 printf "local: *;\n};\n"
-- 
1.7.9.2

[PATCH 08/10] notmuch-config: header for index() prototype (Solaris support)

[Blake Jones]

Linux, FreeBSD, and Solaris all expect to find the prototype for
"index()" in <strings.h>.  On some operating systems, including
<string.h> is sufficient to get the prototype, but that's not the case
on Solaris.  This patch just modifies notmuch-config.c to include
<strings.h> to get the prototype.
---
 notmuch-config.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/notmuch-config.c b/notmuch-config.c
index 3e37a2d..2537a65 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -23,6 +23,7 @@
 #include <pwd.h>
 #include <netdb.h>
 #include <assert.h>
+#include <strings.h>
 
 static const char toplevel_config_comment[] =
     " .notmuch-config - Configuration file for the notmuch mail system\n"
-- 
1.7.9.2

Re: [PATCH 08/10] notmuch-config: header for index() prototype (Solaris support)

[Jani Nikula]

On Sun, 04 Nov 2012, Blake Jones <blakej@foo.net> wrote:
> Linux, FreeBSD, and Solaris all expect to find the prototype for
> "index()" in <strings.h>.  On some operating systems, including
> <string.h> is sufficient to get the prototype, but that's not the case
> on Solaris.  This patch just modifies notmuch-config.c to include
> <strings.h> to get the prototype.

We should probably just nuke index() and use strchr() instead.

Jani.


diff --git a/notmuch-config.c b/notmuch-config.c
index 3e37a2d..47eb743 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -688,7 +688,7 @@ _item_split (char *item, char **group, char **key)
 
     *group = item;
 
-    period = index (item, '.');
+    period = strchr (item, '.');
     if (period == NULL || *(period+1) == '\0') {
 	fprintf (stderr,
 		 "Invalid configuration name: %s\n"

Re: [PATCH 08/10] notmuch-config: header for index() prototype (Solaris support)

[Tomi Ollila]

On Sun, Nov 04 2012, Jani Nikula wrote:

> On Sun, 04 Nov 2012, Blake Jones <blakej@foo.net> wrote:
>> Linux, FreeBSD, and Solaris all expect to find the prototype for
>> "index()" in <strings.h>.  On some operating systems, including
>> <string.h> is sufficient to get the prototype, but that's not the case
>> on Solaris.  This patch just modifies notmuch-config.c to include
>> <strings.h> to get the prototype.
>
> We should probably just nuke index() and use strchr() instead.

indeed!

>
> Jani.
>

Tomi


>
> diff --git a/notmuch-config.c b/notmuch-config.c
> index 3e37a2d..47eb743 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -688,7 +688,7 @@ _item_split (char *item, char **group, char **key)
>  
>      *group = item;
>  
> -    period = index (item, '.');
> +    period = strchr (item, '.');
>      if (period == NULL || *(period+1) == '\0') {
>  	fprintf (stderr,
>  		 "Invalid configuration name: %s\n"
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 08/10] notmuch-config: header for index() prototype (Solaris support)

[Blake Jones]

>> On Sun, 04 Nov 2012, Blake Jones <blakej@foo.net> wrote:
>>> Linux, FreeBSD, and Solaris all expect to find the prototype for
>>> "index()" in <strings.h>.  On some operating systems, including
>>> <string.h> is sufficient to get the prototype, but that's not the case
>>> on Solaris.  This patch just modifies notmuch-config.c to include
>>> <strings.h> to get the prototype.
>>
>> We should probably just nuke index() and use strchr() instead.
> 
> indeed!

That was my initial preference, but I didn't know if there was anyone
who was committed to the BSD name.  Given that two more people think
it's a good idea, I'll do that instead.

Blake

[PATCH 09/10] debugger.c: correct return type from getppid() (Solaris support)

[Blake Jones]

Cast the return value of getppid() to "int" from "pid_t" in debugger.c,
since it is being passed to sprintf("%d"), which wants an "int"
argument.  On Solaris, "pid_t" is a "long" for 32-bit programs.
---
 debugger.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/debugger.c b/debugger.c
index e8b9378..8ff13d6 100644
--- a/debugger.c
+++ b/debugger.c
@@ -36,7 +36,7 @@ debugger_is_active (void)
     if (RUNNING_ON_VALGRIND)
 	return TRUE;
 
-    sprintf (buf, "/proc/%d/exe", getppid ());
+    sprintf (buf, "/proc/%d/exe", (int) getppid ());
     if (readlink (buf, buf, sizeof (buf)) != -1 &&
 	strncmp (basename (buf), "gdb", 3) == 0)
     {
-- 
1.7.9.2

[PATCH 10/10] timegm: add portable implementation (Solaris support)

[Blake Jones]

The timegm(3) function is a non-standard extension to libc which is
available in GNU libc and on some BSDs.  Although SunOS had this
function in its libc, Solaris (unfortunately) removed it.  This patch
implements a very simple version of timegm() which is good enough for
parse-time-string.c.

Although notmuch's idiom for portability is to test for native
availability and put alternate versions in compat/, that approach led to
a compilation problem in this case.  libnotmuch.a includes a call to
parse_time_string() from parse-time-vrp.o, and parse_time_string() in
libparse-time-string.a needs to call timegm().  An attempt to create
compat/timegm.c caused the link to fail, because libparse-time-string.a
acquired a dependency on the new timegm.o in libnotmuch.a, and the
linker only does a single pass on each ".a" looking for dependencies.
This seems to be the case both for the GNU linker and the Solaris
linker.  A different possible workaround would have been to include
libnotmuch.a multiple times on the link line, but that seemed like a
brittle way to track this dependency.
---
 parse-time-string/parse-time-string.c |   37 ++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/parse-time-string/parse-time-string.c b/parse-time-string/parse-time-string.c
index 584067d..28901af 100644
--- a/parse-time-string/parse-time-string.c
+++ b/parse-time-string/parse-time-string.c
@@ -1315,6 +1315,41 @@ fixup_ampm (struct state *state)
     return 0;
 }
 
+static int
+leapyear (int year)
+{
+    return ((year % 4) == 0 && ((year % 100) != 0 || (year % 400) == 0));
+}
+
+/*
+ * This is a simple implementation of timegm() which does what is needed
+ * by create_output() -- just turns the "struct tm" into a GMT time_t.
+ * It does not normalize any of the fields of the "struct tm", nor does
+ * it set tm_wday or tm_yday.
+ */
+static time_t
+local_timegm (struct tm *tm)
+{
+    int	monthlen[2][12] = {
+	{ 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 },
+	{ 31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 },
+    };
+    int	year, month, days;
+
+    days = 365 * (tm->tm_year - 70);
+    for (year = 70; year < tm->tm_year; year++) {
+	if (leapyear(1900 + year)) {
+	    days++;
+	}
+    }
+    for (month = 0; month < tm->tm_mon; month++) {
+	days += monthlen[leapyear(1900 + year)][month];
+    }
+    days += tm->tm_mday - 1;
+
+    return ((((days * 24) + tm->tm_hour) * 60 + tm->tm_min) * 60 + tm->tm_sec);
+}
+
 /* Combine absolute and relative fields, and round. */
 static int
 create_output (struct state *state, time_t *t_out, const time_t *ref,
@@ -1465,7 +1500,7 @@ create_output (struct state *state, time_t *t_out, const time_t *ref,
     if (is_field_set (state, TM_TZ)) {
 	/* tm is in specified TZ, convert to UTC for timegm(3). */
 	tm.tm_min -= get_field (state, TM_TZ);
-	t = timegm (&tm);
+	t = local_timegm (&tm);
     } else {
 	/* tm is in local time. */
 	t = mktime (&tm);
-- 
1.7.9.2

Re: [PATCH 10/10] timegm: add portable implementation (Solaris support)

[Jani Nikula]

[-- Attachment #1: Type: text/plain, Size: 3659 bytes --]

On Nov 4, 2012 11:30 AM, "Blake Jones" <blakej@foo.net> wrote:
>
> The timegm(3) function is a non-standard extension to libc which is
> available in GNU libc and on some BSDs.  Although SunOS had this
> function in its libc, Solaris (unfortunately) removed it.  This patch
> implements a very simple version of timegm() which is good enough for
> parse-time-string.c.
>
> Although notmuch's idiom for portability is to test for native
> availability and put alternate versions in compat/, that approach led to
> a compilation problem in this case.  libnotmuch.a includes a call to
> parse_time_string() from parse-time-vrp.o, and parse_time_string() in
> libparse-time-string.a needs to call timegm().  An attempt to create
> compat/timegm.c caused the link to fail, because libparse-time-string.a
> acquired a dependency on the new timegm.o in libnotmuch.a, and the
> linker only does a single pass on each ".a" looking for dependencies.
> This seems to be the case both for the GNU linker and the Solaris
> linker.  A different possible workaround would have been to include
> libnotmuch.a multiple times on the link line, but that seemed like a
> brittle way to track this dependency.

I'd prefer to use timegm() where available, and the suggested alternative
[1] elsewhere. I'll look into the compat build issues when I have a moment.

Jani.

[1] http://www.kernel.org/doc/man-pages/online/pages/man3/timegm.3.html

> ---
>  parse-time-string/parse-time-string.c |   37
++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/parse-time-string/parse-time-string.c
b/parse-time-string/parse-time-string.c
> index 584067d..28901af 100644
> --- a/parse-time-string/parse-time-string.c
> +++ b/parse-time-string/parse-time-string.c
> @@ -1315,6 +1315,41 @@ fixup_ampm (struct state *state)
>      return 0;
>  }
>
> +static int
> +leapyear (int year)
> +{
> +    return ((year % 4) == 0 && ((year % 100) != 0 || (year % 400) == 0));
> +}
> +
> +/*
> + * This is a simple implementation of timegm() which does what is needed
> + * by create_output() -- just turns the "struct tm" into a GMT time_t.
> + * It does not normalize any of the fields of the "struct tm", nor does
> + * it set tm_wday or tm_yday.
> + */
> +static time_t
> +local_timegm (struct tm *tm)
> +{
> +    int        monthlen[2][12] = {
> +       { 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 },
> +       { 31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 },
> +    };
> +    int        year, month, days;
> +
> +    days = 365 * (tm->tm_year - 70);
> +    for (year = 70; year < tm->tm_year; year++) {
> +       if (leapyear(1900 + year)) {
> +           days++;
> +       }
> +    }
> +    for (month = 0; month < tm->tm_mon; month++) {
> +       days += monthlen[leapyear(1900 + year)][month];
> +    }
> +    days += tm->tm_mday - 1;
> +
> +    return ((((days * 24) + tm->tm_hour) * 60 + tm->tm_min) * 60 +
tm->tm_sec);
> +}
> +
>  /* Combine absolute and relative fields, and round. */
>  static int
>  create_output (struct state *state, time_t *t_out, const time_t *ref,
> @@ -1465,7 +1500,7 @@ create_output (struct state *state, time_t *t_out,
const time_t *ref,
>      if (is_field_set (state, TM_TZ)) {
>         /* tm is in specified TZ, convert to UTC for timegm(3). */
>         tm.tm_min -= get_field (state, TM_TZ);
> -       t = timegm (&tm);
> +       t = local_timegm (&tm);
>      } else {
>         /* tm is in local time. */
>         t = mktime (&tm);
> --
> 1.7.9.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[-- Attachment #2: Type: text/html, Size: 4809 bytes --]

Re: [PATCH 10/10] timegm: add portable implementation (Solaris support)

[Blake Jones]

Hi Jani,

> I'd prefer to use timegm() where available, and the suggested
> alternative [1] elsewhere.
> 
> [1] http://www.kernel.org/doc/man-pages/online/pages/man3/timegm.3.html

I considered this alternative, but decided against it because it's
completely MT-unsafe.  I don't know whether libnotmuch itself is
MT-safe, but a process which called this routine in one thread would
temporarily throw off any timezone-related work that any other threads
were doing, even if they weren't using libnotmuch.

> I'll look into the compat build issues when I have a moment.

If you do, here's a boiled-down version of the problem that I came up
with while investigating it:

    $ echo 'int main() { extern int foo1(); return foo1(); }' > main.c
    $ echo 'int foo1() { extern int bar();  return bar();  }' > foo1.c
    $ echo 'int bar()  { extern int foo2(); return foo2(); }' > bar.c
    $ echo 'int foo2() { return 0;                         }' > foo2.c
    $ gcc -c main.c foo1.c bar.c foo2.c
    $ ar rcs libfoo.a foo1.o foo2.o
    $ ar rcs libbar.a bar.o
    $ gcc -o main main.o libfoo.a libbar.a
      [fails]
    $ gcc -o main main.o libbar.a libfoo.a
      [fails]

Another alternative would be to just include parse-time-string.o in
libnotmuch.a directly; I think that would solve the problem.

Blake

Re: [PATCH 10/10] timegm: add portable implementation (Solaris support)

[Jani Nikula]

On Sun, 04 Nov 2012, Blake Jones <blakej@foo.net> wrote:
> Hi Jani,
>
>> I'd prefer to use timegm() where available, and the suggested
>> alternative [1] elsewhere.
>> 
>> [1] http://www.kernel.org/doc/man-pages/online/pages/man3/timegm.3.html
>
> I considered this alternative, but decided against it because it's
> completely MT-unsafe.  I don't know whether libnotmuch itself is
> MT-safe, but a process which called this routine in one thread would
> temporarily throw off any timezone-related work that any other threads
> were doing, even if they weren't using libnotmuch.

That is a valid point. Yet it doesn't change the fact that I'd prefer to
use timegm() where available. Internally, glibc uses the same code to
implement both timegm() and mktime(), and I'd hate it if the results
were subtly different depending on whether the time zone was specified
in the input or not. That said, I'm not opposed to using your simple
timegm() alternative in the compat code if you think it's good enough to
get you going on Solaris.

As to solving the compat linking problem, I think the patch at the end
of this message should fix it. Please try that with the regular notmuch
approach to portability. The general idea is to keep parse-time-string
as independent as possible from the rest of notmuch (possibly turning it
into a dynamic library and a package of its own eventually), but I think
including compat.h is an acceptable exception to make.

HTH,
Jani.


diff --git a/parse-time-string/Makefile.local b/parse-time-string/Makefile.local
index 53534f3..b3e5385 100644
--- a/parse-time-string/Makefile.local
+++ b/parse-time-string/Makefile.local
@@ -1,7 +1,9 @@
 dir := parse-time-string
 extra_cflags += -I$(srcdir)/$(dir)
 
-libparse-time-string_c_srcs := $(dir)/parse-time-string.c
+libparse-time-string_c_srcs =		\
+	$(notmuch_compat_srcs)		\
+	$(dir)/parse-time-string.c
 
 libparse-time-string_modules := $(libparse-time-string_c_srcs:.c=.o)
 
diff --git a/parse-time-string/parse-time-string.c b/parse-time-string/parse-time-string.c
index 584067d3..ccad422 100644
--- a/parse-time-string/parse-time-string.c
+++ b/parse-time-string/parse-time-string.c
@@ -32,6 +32,7 @@
 #include <sys/time.h>
 #include <sys/types.h>
 
+#include "compat.h"
 #include "parse-time-string.h"
 
 /*

Re: [PATCH 10/10] timegm: add portable implementation (Solaris support)

[Blake Jones]

> That is a valid point. Yet it doesn't change the fact that I'd prefer
> to use timegm() where available. Internally, glibc uses the same code
> to implement both timegm() and mktime(), and I'd hate it if the
> results were subtly different depending on whether the time zone was
> specified in the input or not.

That's fine with me.

> That said, I'm not opposed to using your simple timegm() alternative
> in the compat code if you think it's good enough to get you going on
> Solaris.

I think it is, assuming you don't plan to use tm_wday or tm_yday in your
parse-time-string code, and that you don't plan to depend on the side
effect of timegm() canonicalizing the passed-in "struct tm".

> As to solving the compat linking problem, I think the patch at the end
> of this message should fix it. Please try that with the regular
> notmuch approach to portability. The general idea is to keep
> parse-time-string as independent as possible from the rest of notmuch
> (possibly turning it into a dynamic library and a package of its own
> eventually), but I think including compat.h is an acceptable exception
> to make.

Yeah, this seems to work.  I'll update my patch set accordingly.

Blake

Re: [PATCH 10/10] timegm: add portable implementation (Solaris support)

[Tomi Ollila]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

On Sun, Nov 04 2012, Blake Jones wrote:

> Hi Jani,
>
>> I'd prefer to use timegm() where available, and the suggested
>> alternative [1] elsewhere.
>> 
>> [1] http://www.kernel.org/doc/man-pages/online/pages/man3/timegm.3.html
>
> I considered this alternative, but decided against it because it's
> completely MT-unsafe.  I don't know whether libnotmuch itself is
> MT-safe, but a process which called this routine in one thread would
> temporarily throw off any timezone-related work that any other threads
> were doing, even if they weren't using libnotmuch.

Yet another idea for an alternative. Compile by entering 'sh xtimegm.c'
and then run ./xtimegm

Simple cases seems to work. Dst change may (or then may not) give one
hour difference to the expected. The test "coverage" could be easily
expanded to that ;)

Hmm, I also found this:
http://lists.gnu.org/archive/html/bug-gnulib/2003-09/msg00004.html
which does 2 mktime's and one gmtime_r (without allocating tg!)...
Pick any of these 3 -- or something different (e.g. less NIH if there is)

> Blake


Tomi


[-- Attachment #2: yet another attempt for timegm() implementation --]
[-- Type: text/plain, Size: 2225 bytes --]

#if 0 /* -*- mode: c; c-file-style: "stroustrup"; tab-width: 8; -*-
 set -eu; trg=`basename "$0" .c`; rm -f "$trg"
 WARN="-Wall -Wno-long-long -Wstrict-prototypes -pedantic"
 WARN="$WARN -Wcast-align -Wpointer-arith " # -Wfloat-equal #-Werror
 WARN="$WARN -W -Wwrite-strings -Wcast-qual -Wshadow" # -Wconversion
 case ${1-} in '') set x -O2; shift; esac
 #case ${1-} in '') set x -ggdb; shift; esac
 set -x; exec ${CC:-gcc} -std=c99 $WARN "$@" -o "$trg" "$0"
 exit $?
 */
#endif
/*
 * $ xtimegm.c $
 *
 * Author: Tomi Ollila -- too ät iki piste fi
 *
 * Created: Mon 05 Nov 2012 07:54:35 EET too
 * Last modified: Mon 05 Nov 2012 08:27:52 EET too
 *
 * Public domain.
 */

#define _POSIX_SOURCE

#include <time.h>

time_t xtimegm(struct tm * atm)
{
    struct tm ltm;
    time_t t = mktime(atm);
    int sd;
    int dd;

    (void)gmtime_r(&t, &ltm);

    sd = (atm->tm_hour - ltm.tm_hour) * 3600 + (atm->tm_min - ltm.tm_min) * 60
	+ (atm->tm_sec - ltm.tm_sec);

    dd = (atm->tm_mday - ltm.tm_mday);

    if (dd == 0)
	;
    else if (dd == -1) sd -= 86400;  /* e.g. 3, 2 */
    else if (dd == 1)  sd += 86400;  /* e.g. 2, 3 */
    else if (dd > 1)   sd -= 86400;  /* e.g. 1, 31 */
    else /* dd < 1 */  sd += 86400;  /* e.g. 31, 1 */

    printf("sd: %d\n", sd); // test line.

    return t + sd;
}

#include <stdio.h>
void test(int hour, int mday)
{
    struct tm mtm;
    time_t t;
    struct tm * otm;

    mtm.tm_sec = 0;
    mtm.tm_min = 0;
    mtm.tm_hour = hour;

    mtm.tm_mday = mday;
    mtm.tm_mon = 11;
    mtm.tm_year = 100;
    mtm.tm_isdst = -1;

    t = xtimegm(&mtm);
    otm = gmtime(&t);

    printf("%2d %02d:%02d:%02d\n", otm->tm_mday,
	   otm->tm_hour, otm->tm_min, otm->tm_sec);
}

void tests(const char * tz)
{
    printf("TZ: %s\n", tz);
    setenv("TZ", tz, 1);
    tzset();
    test(23, 30);
    test(23, 31);
    test(23, 1);
    test(23, 2);
    printf("--\n");
    test(1, 30);
    test(1, 31);
    test(1, 1);
    test(1, 2);

}

int main(int argc, char * argv[])
{
    // ls /usr/share/zoneinfo...
    tests("EET");
    tests("MST");
    tests("HST");
    tests("NZ");

    return 0;
}

Re: [PATCH 10/10] timegm: add portable implementation (Solaris support)

[Blake Jones]

[-- Attachment #1: Type: text/plain, Size: 1268 bytes --]

> Yet another idea for an alternative. Compile by entering 'sh xtimegm.c'
> and then run ./xtimegm
> 
> Simple cases seems to work. Dst change may (or then may not) give one
> hour difference to the expected. The test "coverage" could be easily
> expanded to that ;)
> 
> Hmm, I also found this:
> http://lists.gnu.org/archive/html/bug-gnulib/2003-09/msg00004.html
> which does 2 mktime's and one gmtime_r (without allocating tg!)...
> Pick any of these 3 -- or something different (e.g. less NIH if there is)

Of these two, I would probably lean slightly toward the latter, in that
it relies more on libc for the time zone handling logic.

But in general, this seems to me like a case where an explicit
implementation like mine is less prone to failure, because it doesn't
need to worry about time zones at all.  The other approaches rely on
letting libc do all the hard work of time zone manipulation, and then
reading the tea leaves to find a way to undo it.  I would guess that if
some change in the time standards is going to break one of these
implementations, it's going to be some new time zone specification, not
a change in the number of days in a month. :)

For what it's worth, I used the attached program to test my
implementation, and it passed.

Blake


[-- Attachment #2: Type: text/plain, Size: 2553 bytes --]

#include <time.h>

static int
leapyear(int year)
{
	return ((year % 4) == 0 && ((year % 100) != 0 || (year % 400) == 0));
}

/*
 * This is a simple implementation of timegm() which does what is needed
 * by parse-time-string.c -- just turns the "struct tm" into a GMT time_t.
 * It does not normalize any of the fields of the "struct tm", nor does
 * it set tm_wday or tm_yday.
 */
static time_t
timegm(struct tm *tm)
{
	int	monthlen[2][12] = {
		{ 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 },
		{ 31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 },
	};
	int	year, month, days;

	days = 365 * (tm->tm_year - 70);
	for (year = 70; year < tm->tm_year; year++) {
		if (leapyear(1900 + year)) {
			days++;
		}
	}
	for (month = 0; month < tm->tm_mon; month++) {
		days += monthlen[leapyear(1900 + year)][month];
	}
	days += tm->tm_mday - 1;

	return ((((days * 24) + tm->tm_hour) * 60 + tm->tm_min) * 60 +
	    tm->tm_sec);
}

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

void
tm_test(int niter)
{
	const int	monthlen[2][12] = {
		{ 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 },
		{ 31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 },
	};
	int		i;
	struct tm	tm, *tmp;
	time_t		t;

	for (i = 0; i < niter; i++) {
		tm.tm_year = (lrand48() % 67) + 70;
		tm.tm_mon = lrand48() % 12;
		do {
			t = (lrand48() % 31) + 1;
		} while (t > monthlen[leapyear(1900 + tm.tm_year)][tm.tm_mon]);
		tm.tm_mday = t;
		tm.tm_hour = lrand48() % 24;
		tm.tm_min = lrand48() % 60;
		tm.tm_sec = lrand48() % 60;

		t = timegm(&tm);
		tmp = gmtime(&t);

		if (tmp->tm_sec != tm.tm_sec ||
		    tmp->tm_min != tm.tm_min ||
		    tmp->tm_hour != tm.tm_hour ||
		    tmp->tm_mday != tm.tm_mday ||
		    tmp->tm_mon != tm.tm_mon ||
		    tmp->tm_year != tm.tm_year) {
			printf("%4d-%02d-%02d %02d:%02d:%02d -> "
			    "%4d-%02d-%02d %02d:%02d:%02d\n",
			    tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
			    tm.tm_hour, tm.tm_min, tm.tm_sec,
			    tmp->tm_year + 1900, tmp->tm_mon + 1, tmp->tm_mday,
			    tmp->tm_hour, tmp->tm_min, tmp->tm_sec);

		}
	}
}

void
time_test(int niter)
{
	int		i;
	time_t		st, et;
	struct tm	*tmp;

	for (i = 0; i < niter; i++) {
		st = (time_t)lrand48();

		tmp = gmtime(&st);
		et = timegm(tmp);

		if (st != et) {
			printf("%d -> %d (%4d-%02d-%02d %02d:%02d:%02d)\n",
			    st, et,
			    tmp->tm_year + 1900, tmp->tm_mon + 1, tmp->tm_mday,
			    tmp->tm_hour, tmp->tm_min, tmp->tm_sec);
		}
	}
}

int
main(void)
{
	const int	niter = 10000000;

	srand48(getpid());

	tm_test(niter);
	time_test(niter);

	return (0);
}

Re: [PATCH 10/10] timegm: add portable implementation (Solaris support)

[Tomi Ollila]

On Mon, Nov 05 2012, Blake Jones wrote:

>> Yet another idea for an alternative. Compile by entering 'sh xtimegm.c'
>> and then run ./xtimegm
>> 
>> Simple cases seems to work. Dst change may (or then may not) give one
>> hour difference to the expected. The test "coverage" could be easily
>> expanded to that ;)
>> 
>> Hmm, I also found this:
>> http://lists.gnu.org/archive/html/bug-gnulib/2003-09/msg00004.html
>> which does 2 mktime's and one gmtime_r (without allocating tg!)...
>> Pick any of these 3 -- or something different (e.g. less NIH if there is)
>
> Of these two, I would probably lean slightly toward the latter, in that
> it relies more on libc for the time zone handling logic.
>
> But in general, this seems to me like a case where an explicit
> implementation like mine is less prone to failure, because it doesn't
> need to worry about time zones at all. 

Good point.

> The other approaches rely on
> letting libc do all the hard work of time zone manipulation, and then
> reading the tea leaves to find a way to undo it.

Did you look at the gnu libc version -- I bet it is pretty hairy...

>  I would guess that if
> some change in the time standards is going to break one of these
> implementations, it's going to be some new time zone specification, not
> a change in the number of days in a month. :)
>
> For what it's worth, I used the attached program to test my
> implementation, and it passed.

Thanks, It's nice to see your simple implementation passes these tests...

Just for curiosity: What do you think lacks in your timegm() that it could
not be promoted as 'complete timegm() solution'.

In any way, including this timegm() function in compat suits me fine.

>
> Blake
>

Tomi

Re: [PATCH 10/10] timegm: add portable implementation (Solaris support)

[Blake Jones]

>> The other approaches rely on letting libc do all the hard work of
>> time zone manipulation, and then reading the tea leaves to find a way
>> to undo it.
> 
> Did you look at the gnu libc version -- I bet it is pretty hairy...

I didn't look at either the GNU or the Solaris libc version.  But the
file that implements the timezone handling (and localtime(), mktime(),
etc.) in Solaris' libc is nearly 3000 lines of code, so I suspect
there's an awful lot of stuff going on.

>> For what it's worth, I used the attached program to test my
>> implementation, and it passed.
> 
> Thanks, It's nice to see your simple implementation passes these
> tests...
> 
> Just for curiosity: What do you think lacks in your timegm() that it
> could not be promoted as 'complete timegm() solution'.

Well, since there isn't a standard for timegm(), I'm comparing it to
what glibc and BSD do.  The glibc mktime() man page, for example,
mentions that mktime() modifies the fields of the tm structure as
follows:

    - tm_wday and tm_yday are set to values determined from the contents
      of the other fields

    - if structure members are outside their valid interval, they will
      be normalized (so that, for example, 40 October is changed into 9
      November)

    - tm_isdst is set (regardless of its initial value) to a positive
      value or to 0, respectively, to indicate whether DST is or is not
      in effect at the specified time.

    - Calling mktime() also sets the external variable tzname with
      information about the current timezone. 

The corresponding timegm() man page for glibc doesn't say whether
timegm() does the same thing, but I would assume it does.  The FreeBSD
timegm() man page says that its version does update the fields of the
"tm" structure, like its mktime() implementation.

My implementation of timegm() does none of these things.  It treats the
passed-in "struct tm" as constant, and just returns a valid time_t.  If
you really wanted to make this mktime() be as capable as the ones in the
GNU and BSD libc's, you could have it turn around and call gmtime_r() on
the generated time_t, and pass the original "struct tm" to gmtime_r().
Personally, I think this is unnecessary overloading of a function that
does this one thing just fine, and in practice Jani's code doesn't seem
to need it, so I didn't bother.

> In any way, including this timegm() function in compat suits me fine.

Great.

Blake

Re: [PATCH 00/10] Solaris support

[Jani Nikula]

On Sun, 04 Nov 2012, Blake Jones <blakej@foo.net> wrote:
> Hi all,
>
> This patch series fixes several issues which are needed to allow notmuch
> to build on Solaris 11.  I've been "testing" it for a month or so by
> using Karel Zak's fork of mutt along with a copy of notmuch-0.13.2 that
> I got to compile.  After a friend asked for a copy of my setup, I
> decided to try to get my patches cleaned up enough to submit, so that he
> wouldn't need to deal with them if he wanted to hack on it.

Thanks for doing this. I had a *very* brief look at the patches, and
commented on what I thought stood out. Otherwise I didn't spot anything
out of the ordinary, but I'd like it if someone else had a glance too.

BR,
Jani.