[PATCH 0/3] Build and test Ruby bindings

[PATCH 0/3] Build and test Ruby bindings

[Felipe Contreras]

This patch series adds simple tests for the Ruby bindings, which don't really
make sense to add unless we build the bindings by default, so I added support for that.

Unfortunately the pkg-config files for Ruby are specific to each major version,
so we have to check for Ruby 2.1, 2.0, 1.9, etc. For now I'm checking only for
Ruby 2.1, but more versions can be added later if demanded. Since this support
wasn't there before nothing will be broken if we don't build on say, Ruby 2.0.


Felipe Contreras (3):
  build: don't add sub Makefiles to the global deps
  build: add support to build Ruby bindings
  test: add tests for Ruby bindings

 Makefile                     |  2 +-
 bindings/ruby/Makefile.local | 21 ++++++++++
 bindings/ruby/extconf.rb     | 14 +------
 configure                    | 21 ++++++++++
 test/T540-ruby.sh            | 98 ++++++++++++++++++++++++++++++++++++++++++++
 test/test-lib.sh             |  1 +
 6 files changed, 144 insertions(+), 13 deletions(-)
 create mode 100644 bindings/ruby/Makefile.local
 create mode 100755 test/T540-ruby.sh

-- 
1.9.3+fc1~5~gfaddd51

[PATCH 1/3] build: don't add sub Makefiles to the global deps

[Felipe Contreras]

They don't affect the global build.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 4c0e8c6..2d1aee9 100644
--- a/Makefile
+++ b/Makefile
@@ -20,7 +20,7 @@ include Makefile.config
 
 # We make all targets depend on the Makefiles themselves.
 global_deps = Makefile Makefile.config Makefile.local \
-	$(subdirs:%=%/Makefile) $(subdirs:%=%/Makefile.local)
+	$(subdirs:%=%/Makefile.local)
 
 Makefile.config: $(srcdir)/configure
 ifeq ($(configure_options),)
-- 
1.9.3+fc1~5~gfaddd51

[PATCH 2/3] build: add support to build Ruby bindings

[Felipe Contreras]

So there's no need for the user to manually do that.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 bindings/ruby/Makefile.local | 21 +++++++++++++++++++++
 bindings/ruby/extconf.rb     | 14 ++------------
 configure                    | 21 +++++++++++++++++++++
 3 files changed, 44 insertions(+), 12 deletions(-)
 create mode 100644 bindings/ruby/Makefile.local

diff --git a/bindings/ruby/Makefile.local b/bindings/ruby/Makefile.local
new file mode 100644
index 0000000..8b1837c
--- /dev/null
+++ b/bindings/ruby/Makefile.local
@@ -0,0 +1,21 @@
+dir := bindings/ruby
+
+ifeq ($(WITH_RUBY),1)
+all: $(dir)/notmuch.so
+install: install-ruby
+clean: clean-ruby
+endif
+
+$(dir)/Makefile: $(dir)/extconf.rb
+	@ruby -C $(dir) extconf.rb --vendor
+
+$(dir)/notmuch.so: | $(dir)/Makefile lib/libnotmuch.so
+	@$(MAKE) -C $(dir) notmuch.so
+
+install-ruby: | $(dir)/Makefile
+	@$(MAKE) -C $(dir) install prefix=$(DESTDIR)/$(prefix)
+
+clean-ruby: | $(dir)/Makefile
+	$(MAKE) -C $(dir) clean
+
+.PHONY: install-ruby
diff --git a/bindings/ruby/extconf.rb b/bindings/ruby/extconf.rb
index 6160db2..abd67fc 100644
--- a/bindings/ruby/extconf.rb
+++ b/bindings/ruby/extconf.rb
@@ -13,18 +13,8 @@ $INCFLAGS = "-I#{dir} #{$INCFLAGS}"
 # make sure there are no undefined symbols
 $LDFLAGS += ' -Wl,--no-undefined'
 
-def have_local_library(lib, path, func, headers = nil)
-  checking_for checking_message(func, lib) do
-    lib = File.join(path, lib)
-    if try_func(func, lib, headers)
-      $LOCAL_LIBS += lib
-    end
-  end
-end
-
-if not have_local_library('libnotmuch.so', dir, 'notmuch_database_create', 'notmuch.h')
-  exit 1
-end
+# library
+$LOCAL_LIBS += "#{dir}/libnotmuch.so"
 
 # Create Makefile
 dir_config('notmuch')
diff --git a/configure b/configure
index 9bde2eb..3bdf6d7 100755
--- a/configure
+++ b/configure
@@ -21,6 +21,7 @@ srcdir=$(dirname "$0")
 
 subdirs="util compat lib parse-time-string completion doc emacs"
 subdirs="${subdirs} performance-test test test/test-databases"
+subdirs="${subdirs} bindings/ruby"
 
 # For a non-srcdir configure invocation (such as ../configure), create
 # the directory structure and copy Makefiles.
@@ -65,6 +66,7 @@ LIBDIR=
 WITH_EMACS=1
 WITH_BASH=1
 WITH_ZSH=1
+WITH_RUBY=1
 
 # Compatible GMime versions (with constraints).
 # If using GMime 2.6, we need to have a version >= 2.6.5 to avoid a
@@ -212,6 +214,14 @@ for option; do
 	 elif [ "${option#*=}" = '2.6' ]; then
 	     WITH_GMIME_VERSIONS=$GMIME_26_VERSION
 	fi
+    elif [ "${option%%=*}" = '--with-ruby' ]; then
+	if [ "${option#*=}" = 'no' ]; then
+	    WITH_RUBY=0
+	else
+	    WITH_RUBY=1
+	fi
+    elif [ "${option}" = '--without-ruby' ] ; then
+	WITH_RUBY=0
     elif [ "${option%%=*}" = '--build' ] ; then
 	true
     elif [ "${option%%=*}" = '--host' ] ; then
@@ -383,6 +393,14 @@ else
     WITH_BASH=0
 fi
 
+printf "Checking for ruby... "
+if pkg-config --exists ruby-2.1; then
+    printf "Yes.\n"
+else
+    printf "No (will not install Ruby bindings).\n"
+    WITH_RUBY=0
+fi
+
 if [ -z "${EMACSLISPDIR}" ]; then
     if pkg-config --exists emacs; then
 	EMACSLISPDIR=$(pkg-config emacs --variable sitepkglispdir)
@@ -906,6 +924,9 @@ WITH_BASH = ${WITH_BASH}
 # Support for zsh completion
 WITH_ZSH = ${WITH_ZSH}
 
+# Support for Ruby
+WITH_RUBY = ${WITH_RUBY}
+
 # Combined flags for compiling and linking against all of the above
 CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
 		   -DHAVE_CANONICALIZE_FILE_NAME=\$(HAVE_CANONICALIZE_FILE_NAME) \\
-- 
1.9.3+fc1~5~gfaddd51

[PATCH 3/3] test: add tests for Ruby bindings

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 test/T540-ruby.sh | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 test/test-lib.sh  |  1 +
 2 files changed, 99 insertions(+)
 create mode 100755 test/T540-ruby.sh

diff --git a/test/T540-ruby.sh b/test/T540-ruby.sh
new file mode 100755
index 0000000..a9f6ac5
--- /dev/null
+++ b/test/T540-ruby.sh
@@ -0,0 +1,98 @@
+#!/usr/bin/env bash
+#
+# Copyright (c) 2014 Felipe Contreras
+#
+
+test_description="Ruby bindings"
+
+. ./test-lib.sh
+
+export LD_LIBRARY_PATH=$TEST_DIRECTORY/../lib
+export RUBYLIB=$TEST_DIRECTORY/../bindings/ruby
+
+if ! test_have_prereq RUBY
+then
+	skip_all='skipping ruby tests'
+	test_done
+fi
+
+test_ruby () {
+	cat > test.rb <<-\EOF &&
+	#!/usr/bin/env ruby
+
+	require 'notmuch'
+
+	$MAIL_DIR = ENV['MAIL_DIR']
+	EOF
+	cat >> test.rb &&
+	ruby test.rb
+}
+
+test_expect_success 'setup' '
+	add_email_corpus &&
+	notmuch new --quiet &&
+	export MAIL_DIR
+'
+
+test_expect_success 'new and close' '
+	test_ruby <<-\EOF
+	db = Notmuch::Database.new($MAIL_DIR)
+	db.close
+	EOF
+'
+
+test_expect_success 'open' '
+	test_ruby <<-\EOF
+	Notmuch::Database.open($MAIL_DIR) do |db|
+	end
+	EOF
+'
+
+test_expect_success 'path' '
+	echo $MAIL_DIR > expected &&
+	test_ruby > actual <<-\EOF &&
+	Notmuch::Database.open($MAIL_DIR) do |db|
+		puts db.path
+	end
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'query' '
+	notmuch count "tag:inbox" > expected &&
+	test_ruby > actual <<-\EOF &&
+	Notmuch::Database.open($MAIL_DIR) do |db|
+		query = db.query("tag:inbox")
+		puts query.count_messages
+	end
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'query threads' '
+	notmuch search --output=threads "tag:inbox" > expected &&
+	test_ruby > actual <<-\EOF &&
+	Notmuch::Database.open($MAIL_DIR) do |db|
+		query = db.query("tag:inbox")
+		query.search_threads.each do |thread|
+			puts "thread:" + thread.thread_id
+		end
+	end
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'query messages' '
+	notmuch search --output=messages "tag:inbox" > expected &&
+	test_ruby > actual <<-\EOF &&
+	Notmuch::Database.open($MAIL_DIR) do |db|
+		query = db.query("tag:inbox")
+		query.search_messages.each do |message|
+			puts "id:" + message.message_id
+		end
+	end
+	EOF
+	test_cmp expected actual
+'
+
+test_done
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 8697d6a..1f9dd1f 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -1208,6 +1208,7 @@ test_init_ () {
 
 emacs_generate_script
 
+grep -q "WITH_RUBY = 1" ../Makefile.config && test_set_prereq RUBY
 
 # Use -P to resolve symlinks in our working directory so that the cwd
 # in subprocesses like git equals our $PWD (for pathname comparisons).
-- 
1.9.3+fc1~5~gfaddd51

Re: [PATCH 2/3] build: add support to build Ruby bindings

[Wael Nasreddine]

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

Felipe Contreras <felipe.contreras@gmail.com> writes:

> So there's no need for the user to manually do that.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  bindings/ruby/Makefile.local | 21 +++++++++++++++++++++
>  bindings/ruby/extconf.rb     | 14 ++------------
>  configure                    | 21 +++++++++++++++++++++
>  3 files changed, 44 insertions(+), 12 deletions(-)
>  create mode 100644 bindings/ruby/Makefile.local
>
> diff --git a/bindings/ruby/Makefile.local b/bindings/ruby/Makefile.local
> new file mode 100644
> index 0000000..8b1837c
> --- /dev/null
> +++ b/bindings/ruby/Makefile.local
> @@ -0,0 +1,21 @@
> +dir := bindings/ruby
> +
> +ifeq ($(WITH_RUBY),1)
> +all: $(dir)/notmuch.so
> +install: install-ruby
> +clean: clean-ruby
> +endif
> +
> +$(dir)/Makefile: $(dir)/extconf.rb
> +	@ruby -C $(dir) extconf.rb --vendor
> +
> +$(dir)/notmuch.so: | $(dir)/Makefile lib/libnotmuch.so
> +	@$(MAKE) -C $(dir) notmuch.so
> +
> +install-ruby: | $(dir)/Makefile
> +	@$(MAKE) -C $(dir) install prefix=$(DESTDIR)/$(prefix)
> +
> +clean-ruby: | $(dir)/Makefile
> +	$(MAKE) -C $(dir) clean
> +
> +.PHONY: install-ruby
> diff --git a/bindings/ruby/extconf.rb b/bindings/ruby/extconf.rb
> index 6160db2..abd67fc 100644
> --- a/bindings/ruby/extconf.rb
> +++ b/bindings/ruby/extconf.rb
> @@ -13,18 +13,8 @@ $INCFLAGS = "-I#{dir} #{$INCFLAGS}"
>  # make sure there are no undefined symbols
>  $LDFLAGS += ' -Wl,--no-undefined'
>  
> -def have_local_library(lib, path, func, headers = nil)
> -  checking_for checking_message(func, lib) do
> -    lib = File.join(path, lib)
> -    if try_func(func, lib, headers)
> -      $LOCAL_LIBS += lib
> -    end
> -  end
> -end
> -
> -if not have_local_library('libnotmuch.so', dir, 'notmuch_database_create', 'notmuch.h')
> -  exit 1
> -end
> +# library
> +$LOCAL_LIBS += "#{dir}/libnotmuch.so"
>  
>  # Create Makefile
>  dir_config('notmuch')
> diff --git a/configure b/configure
> index 9bde2eb..3bdf6d7 100755
> --- a/configure
> +++ b/configure
> @@ -21,6 +21,7 @@ srcdir=$(dirname "$0")
>  
>  subdirs="util compat lib parse-time-string completion doc emacs"
>  subdirs="${subdirs} performance-test test test/test-databases"
> +subdirs="${subdirs} bindings/ruby"
>  
>  # For a non-srcdir configure invocation (such as ../configure), create
>  # the directory structure and copy Makefiles.
> @@ -65,6 +66,7 @@ LIBDIR=
>  WITH_EMACS=1
>  WITH_BASH=1
>  WITH_ZSH=1
> +WITH_RUBY=1
>  
>  # Compatible GMime versions (with constraints).
>  # If using GMime 2.6, we need to have a version >= 2.6.5 to avoid a
> @@ -212,6 +214,14 @@ for option; do
>  	 elif [ "${option#*=}" = '2.6' ]; then
>  	     WITH_GMIME_VERSIONS=$GMIME_26_VERSION
>  	fi
> +    elif [ "${option%%=*}" = '--with-ruby' ]; then
> +	if [ "${option#*=}" = 'no' ]; then
> +	    WITH_RUBY=0
> +	else
> +	    WITH_RUBY=1
> +	fi
> +    elif [ "${option}" = '--without-ruby' ] ; then
> +	WITH_RUBY=0
>      elif [ "${option%%=*}" = '--build' ] ; then
>  	true
>      elif [ "${option%%=*}" = '--host' ] ; then
> @@ -383,6 +393,14 @@ else
>      WITH_BASH=0
>  fi
>  
> +printf "Checking for ruby... "
> +if pkg-config --exists ruby-2.1; then
Why don't you do an OR check for ruby 1.9, 2.0 and 2.1 allowing it to
build with any Ruby version?

> +    printf "Yes.\n"
> +else
> +    printf "No (will not install Ruby bindings).\n"
> +    WITH_RUBY=0
> +fi
> +
>  if [ -z "${EMACSLISPDIR}" ]; then
>      if pkg-config --exists emacs; then
>  	EMACSLISPDIR=$(pkg-config emacs --variable sitepkglispdir)
> @@ -906,6 +924,9 @@ WITH_BASH = ${WITH_BASH}
>  # Support for zsh completion
>  WITH_ZSH = ${WITH_ZSH}
>  
> +# Support for Ruby
> +WITH_RUBY = ${WITH_RUBY}
> +
>  # Combined flags for compiling and linking against all of the above
>  CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
>  		   -DHAVE_CANONICALIZE_FILE_NAME=\$(HAVE_CANONICALIZE_FILE_NAME) \\
> -- 
> 1.9.3+fc1~5~gfaddd51
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[-- Attachment #2.1: Type: text/plain, Size: 83 bytes --]

-- 
Wael Nasreddine | SRE at Google | wael.nasreddine@gmail.com | (650) 735-1773

[-- Attachment #2.2: Type: application/pgp-signature, Size: 180 bytes --]

Re: [PATCH 1/3] build: don't add sub Makefiles to the global deps

[Felipe Contreras]

Jani Nikula wrote:
> On Fri, 23 May 2014, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> > They don't affect the global build.
> 
> They do.

They don't.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/3] build: don't add sub Makefiles to the global deps

[Jani Nikula]

On Fri, 23 May 2014, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> They don't affect the global build.

They do.

BR,
Jani.

>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 4c0e8c6..2d1aee9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -20,7 +20,7 @@ include Makefile.config
>  
>  # We make all targets depend on the Makefiles themselves.
>  global_deps = Makefile Makefile.config Makefile.local \
> -	$(subdirs:%=%/Makefile) $(subdirs:%=%/Makefile.local)
> +	$(subdirs:%=%/Makefile.local)
>  
>  Makefile.config: $(srcdir)/configure
>  ifeq ($(configure_options),)
> -- 
> 1.9.3+fc1~5~gfaddd51
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/3] build: don't add sub Makefiles to the global deps

[Jani Nikula]

On Sun, 25 May 2014, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> Jani Nikula wrote:
>> On Fri, 23 May 2014, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> > They don't affect the global build.
>> 
>> They do.
>
> They don't.

Your patch, your commit message, please explain *why* you think they
wouldn't or shouldn't affect the global build. Just to mention one
thing, why someone modifying cflags in sub makefiles shouldn't cause
rebuild?


BR,
Jani.

Re: [PATCH 1/3] build: don't add sub Makefiles to the global deps

[Felipe Contreras]

Jani Nikula wrote:
> On Sun, 25 May 2014, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> > Jani Nikula wrote:
> >> On Fri, 23 May 2014, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> >> > They don't affect the global build.
> >> 
> >> They do.
> >
> > They don't.
> 
> Your patch, your commit message, please explain *why* you think they
> wouldn't or shouldn't affect the global build. Just to mention one
> thing, why someone modifying cflags in sub makefiles shouldn't cause
> rebuild?

Go to emacs/Makefile, it's basically a stub, why would any modifications
there affect the global build? It's not included anywhere.

emacs/Makefile.local is included in the global build, emacs/Makefile is
not.

-- 
Felipe Contreras

Re: [PATCH 1/3] build: don't add sub Makefiles to the global deps

[Jani Nikula]

On Sun, 25 May 2014, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> Jani Nikula wrote:
>> On Sun, 25 May 2014, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> > Jani Nikula wrote:
>> >> On Fri, 23 May 2014, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> >> > They don't affect the global build.
>> >> 
>> >> They do.
>> >
>> > They don't.
>> 
>> Your patch, your commit message, please explain *why* you think they
>> wouldn't or shouldn't affect the global build. Just to mention one
>> thing, why someone modifying cflags in sub makefiles shouldn't cause
>> rebuild?
>
> Go to emacs/Makefile, it's basically a stub, why would any modifications
> there affect the global build? It's not included anywhere.
>
> emacs/Makefile.local is included in the global build, emacs/Makefile is
> not.

Fair enough. And what is the benefit of not depending on them, since
they hardly ever change? I'd rather err on the depending side.

BR,
Jani.

Re: [PATCH 3/3] test: add tests for Ruby bindings

[David Bremner]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>  2 files changed, 99 insertions(+)
>  create mode 100755 test/T540-ruby.sh
> +# Copyright (c) 2014 Felipe Contreras

Thanks for writing these; lack of tests bindings has been a long time
irritant for me.  Can you put in a brief license statement as well as
the copyright line?  Something like "same as notmuch" or "GPL 3+" is ok
for me.

>  
> +grep -q "WITH_RUBY = 1" ../Makefile.config && test_set_prereq RUBY

This looks a bit fragile. Somewhere in the patch queue is something to
create a config.sh snippet which would help here.

d

Re: [PATCH 2/3] build: add support to build Ruby bindings

[David Bremner]

Felipe Contreras <felipe.contreras@gmail.com> writes:
> +
> +$(dir)/notmuch.so: | $(dir)/Makefile lib/libnotmuch.so
> +	@$(MAKE) -C $(dir) notmuch.so

- since this is the only place (afaik) in the notmuch codebase that
order only prereqs are used, their use probably deserves some comment.

- can you explain why you depend on $(dir)/Makefile here?

d