[PATCH 0/5] Improving portability to Mac OS X

[PATCH 0/5] Improving portability to Mac OS X

[Charles Celerier]

The patches in this series include a number of minor changes intended to better
support the portability of notmuch to Mac OS X. The changes are relatively
non-intrusive. You may observe that these patches apply entirely to the notmuch
tests; it turns out, with the exception of the tests, notmuch ports very well
to Mac OS X. =]

Note that I have used GNU programs where possible (e.g. nm, sed, date, etc.) so
as to not introduce changes to the testing framework that are easily resolved
by installing standard GNU programs. For example, on Mac OS X the date program
is a BSD variant that does not include the '-d' option that allows specifying a
time to display. This appears to be necessary when testing on Mac OS X; I will
try to note this somewhere on the notmuch wiki soon.

Best,
Chuck

P.S. This is my first time submitting patches to a mailing list, so forgive me if I
have made any foolish mistakes in my attempt to submit changes.


Charles Celerier (5):
  test/Makefile.local: Added configured TALLOC_LDFLAGS.
  configure, test: Added variables for paths to true and false.
  atomicity.gdb: Allow breakpoint symbols to be resolved later.
  T360-symbol-hiding: Added code to support testing on Mac OS X.
  T360-symbol-hiding: Use nm instead of objdump.

 configure                  |  6 ++++++
 test/Makefile.local        | 10 +++++-----
 test/T360-symbol-hiding.sh | 12 ++++++++++--
 test/atomicity.gdb         |  3 +++
 4 files changed, 24 insertions(+), 7 deletions(-)

-- 
1.8.5.2 (Apple Git-48)

[PATCH 1/5] test/Makefile.local: Added configured TALLOC_LDFLAGS.

[Charles Celerier]

The linking to talloc is hard-coded in the testing Makefile. This patch
causes the linking to talloc to be done according to how TALLOC_LDFLAGS
was configured.

Signed-off-by: Charles Celerier <cceleri@cs.stanford.edu>
---
 test/Makefile.local | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/Makefile.local b/test/Makefile.local
index 987441f..d622eaf 100644
--- a/test/Makefile.local
+++ b/test/Makefile.local
@@ -16,7 +16,7 @@ $(dir)/arg-test: $(dir)/arg-test.o command-line-arguments.o util/libutil.a
 	$(call quiet,CC) $^ -o $@
 
 $(dir)/hex-xcode: $(dir)/hex-xcode.o command-line-arguments.o util/libutil.a
-	$(call quiet,CC) $^ -o $@ -ltalloc
+	$(call quiet,CC) $^ $(TALLOC_LDFLAGS) -o $@
 
 random_corpus_deps =  $(dir)/random-corpus.o  $(dir)/database-test.o \
 			notmuch-config.o command-line-arguments.o \
-- 
1.8.5.2 (Apple Git-48)

[PATCH 2/5] configure, test: Added variables for paths to true and false.

[Charles Celerier]

The path to true may not be the same on all platforms (e.g. on Mac OS X
it is /usr/bin/true), so the hard-coded path of /bin/true is not
portable. This is resolved by adding a step to the configure script to
locate the path of true and to use the TRUE variable wherever /bin/true
was needed. The same was done for false.

Signed-off-by: Charles Celerier <cceleri@cs.stanford.edu>
---
 configure           | 6 ++++++
 test/Makefile.local | 8 ++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index 9bde2eb..0bce0a3 100755
--- a/configure
+++ b/configure
@@ -50,6 +50,8 @@ CPPFLAGS=${CPPFLAGS:-}
 CXXFLAGS=${CXXFLAGS:-\$(CFLAGS)}
 LDFLAGS=${LDFLAGS:-}
 XAPIAN_CONFIG=${XAPIAN_CONFIG:-xapian-config}
+TRUE=$(which true)
+FALSE=$(which false)
 
 # We don't allow the EMACS or GZIP Makefile variables inherit values
 # from the environment as we do with CC and CXX above. The reason is
@@ -761,6 +763,10 @@ CXX = ${CXX}
 # Command to execute emacs from Makefiles
 EMACS = emacs --quick
 
+# Define the paths to true and false.
+TRUE = ${TRUE}
+FALSE = ${FALSE}
+
 # Default FLAGS for C compiler (can be overridden by user such as "make CFLAGS=-g")
 CFLAGS = ${CFLAGS}
 
diff --git a/test/Makefile.local b/test/Makefile.local
index d622eaf..e422c06 100644
--- a/test/Makefile.local
+++ b/test/Makefile.local
@@ -37,16 +37,16 @@ $(dir)/parse-time: $(dir)/parse-time.o parse-time-string/parse-time-string.o
 
 $(dir)/have-compact: Makefile.config
 ifeq ($(HAVE_XAPIAN_COMPACT),1)
-	ln -sf /bin/true $@
+	ln -sf $(TRUE) $@
 else
-	ln -sf /bin/false $@
+	ln -sf $(FALSE) $@
 endif
 
 $(dir)/have-man: Makefile.config
 ifeq ($(HAVE_SPHINX)$(HAVE_RST2MAN),00)
-	ln -sf /bin/false $@
+	ln -sf $(FALSE) $@
 else
-	ln -sf /bin/true $@
+	ln -sf $(TRUE) $@
 endif
 
 .PHONY: test check
-- 
1.8.5.2 (Apple Git-48)

[PATCH 3/5] atomicity.gdb: Allow breakpoint symbols to be resolved later.

[Charles Celerier]

On the Mac OS X platform, the rename() function symbol is not found
until the debugger begins running. The reason for this is unknown, but
allowing breakpoint symbols to be resolved later both solves the problem
and does not change the test.

Signed-off-by: Charles Celerier <cceleri@cs.stanford.edu>
---
 test/atomicity.gdb | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/test/atomicity.gdb b/test/atomicity.gdb
index fd67525..127dc8f 100644
--- a/test/atomicity.gdb
+++ b/test/atomicity.gdb
@@ -18,6 +18,9 @@ shell echo 0 > outcount
 
 shell touch inodes
 
+# for gdb on mac
+set breakpoint pending on
+
 break rename
 commands
 # As an optimization, only consider snapshots after a Xapian commit.
-- 
1.8.5.2 (Apple Git-48)

[PATCH 4/5] T360-symbol-hiding: Added code to support testing on Mac OS X.

[Charles Celerier]

The Mac OS X platform uses *.dylib object files instead of *.so object
files for linking. Adding the path to notmuch.dylib to the end of
DYLD_FALLBACK_LIBRARY_PATH has a similar effect to adding the path to
notmuch.so to LD_LIBRARY_PATH on most Linux-based platforms (see
dyld(1)).

Signed-off-by: Charles Celerier <cceleri@cs.stanford.edu>
---
 test/T360-symbol-hiding.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/test/T360-symbol-hiding.sh b/test/T360-symbol-hiding.sh
index 636ec91..97c734a 100755
--- a/test/T360-symbol-hiding.sh
+++ b/test/T360-symbol-hiding.sh
@@ -12,7 +12,14 @@ test_description='exception symbol hiding'
 . ./test-lib.sh
 
 run_test(){
-    result=$(LD_LIBRARY_PATH="$TEST_DIRECTORY/../lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" $TEST_DIRECTORY/symbol-test 2>&1)
+    case $(uname -s) in
+    Darwin)
+        result=$(DYLD_FALLBACK_LIBRARY_PATH="${DYLD_FALLBACK_LIBRARY_PATH:+$DYLD_FALLBACK_LIBRARY_PATH:}$TEST_DIRECTORY/../lib" $TEST_DIRECTORY/symbol-test 2>&1)
+        ;;
+    *)
+        result=$(LD_LIBRARY_PATH="$TEST_DIRECTORY/../lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" $TEST_DIRECTORY/symbol-test 2>&1)
+        ;;
+    esac
 }
 
 output="A Xapian exception occurred opening database: Couldn't stat 'fakedb/.notmuch/xapian'
-- 
1.8.5.2 (Apple Git-48)

[PATCH 5/5] T360-symbol-hiding: Use nm instead of objdump.

[Charles Celerier]

The output of `objdump -t` depends on the format of the object files
which are different across platforms (e.g. Mac OS X). Since we really
just want to filter the symbols in the object file, nm is a more
appropriate tool since it only lists symbols from object files (nm(1))
and has a consistent output format.

Signed-off-by: Charles Celerier <cceleri@cs.stanford.edu>
---
 test/T360-symbol-hiding.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/T360-symbol-hiding.sh b/test/T360-symbol-hiding.sh
index 97c734a..c5bbf27 100755
--- a/test/T360-symbol-hiding.sh
+++ b/test/T360-symbol-hiding.sh
@@ -33,7 +33,8 @@ test_begin_subtest 'checking output'
 test_expect_equal "$result" "$output"
 
 test_begin_subtest 'comparing existing to exported symbols'
-objdump -t $TEST_DIRECTORY/../lib/*.o | awk '$4 == ".text" && $6 ~ "^notmuch" {print $6}' | sort | uniq > ACTUAL
+
+nm -g $TEST_DIRECTORY/../lib/*.o | sed -n 's/.*\s\+T\s\+_\(notmuch_.*\)/\1/p' | sort | uniq > ACTUAL
 sed -n 's/[[:blank:]]*\(notmuch_[^;]*\);/\1/p' $TEST_DIRECTORY/../notmuch.sym | sort | uniq > EXPORTED
 test_expect_equal_file EXPORTED ACTUAL
 
-- 
1.8.5.2 (Apple Git-48)

Re: [PATCH 4/5] T360-symbol-hiding: Added code to support testing on Mac OS X.

[Tomi Ollila]

On Tue, May 06 2014, Charles Celerier <cceleri@cs.stanford.edu> wrote:

> The Mac OS X platform uses *.dylib object files instead of *.so object
> files for linking. Adding the path to notmuch.dylib to the end of
> DYLD_FALLBACK_LIBRARY_PATH has a similar effect to adding the path to
> notmuch.so to LD_LIBRARY_PATH on most Linux-based platforms (see
> dyld(1)).

This series LGTM. I don't understand this difference suffixing
DYLD_FALLBACK_LIBRARY_PATH with $TEST_DIRECTORY/../lib on Mac OS X
compared to prefixing LD_LIBRARY_PATH with the same on other
systems, so I take your word that it works :D

Tomi


>
> Signed-off-by: Charles Celerier <cceleri@cs.stanford.edu>
> ---
>  test/T360-symbol-hiding.sh | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/test/T360-symbol-hiding.sh b/test/T360-symbol-hiding.sh
> index 636ec91..97c734a 100755
> --- a/test/T360-symbol-hiding.sh
> +++ b/test/T360-symbol-hiding.sh
> @@ -12,7 +12,14 @@ test_description='exception symbol hiding'
>  . ./test-lib.sh
>  
>  run_test(){
> -    result=$(LD_LIBRARY_PATH="$TEST_DIRECTORY/../lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" $TEST_DIRECTORY/symbol-test 2>&1)
> +    case $(uname -s) in
> +    Darwin)
> +        result=$(DYLD_FALLBACK_LIBRARY_PATH="${DYLD_FALLBACK_LIBRARY_PATH:+$DYLD_FALLBACK_LIBRARY_PATH:}$TEST_DIRECTORY/../lib" $TEST_DIRECTORY/symbol-test 2>&1)
> +        ;;
> +    *)
> +        result=$(LD_LIBRARY_PATH="$TEST_DIRECTORY/../lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" $TEST_DIRECTORY/symbol-test 2>&1)
> +        ;;
> +    esac
>  }
>  
>  output="A Xapian exception occurred opening database: Couldn't stat 'fakedb/.notmuch/xapian'
> -- 
> 1.8.5.2 (Apple Git-48)
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/5] T360-symbol-hiding: Added code to support testing on Mac OS X.

[Charles Celerier]

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

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Tue, May 06 2014, Charles Celerier <cceleri@cs.stanford.edu> wrote:
>
>> The Mac OS X platform uses *.dylib object files instead of *.so object
>> files for linking. Adding the path to notmuch.dylib to the end of
>> DYLD_FALLBACK_LIBRARY_PATH has a similar effect to adding the path to
>> notmuch.so to LD_LIBRARY_PATH on most Linux-based platforms (see
>> dyld(1)).
>
> This series LGTM. I don't understand this difference suffixing
> DYLD_FALLBACK_LIBRARY_PATH with $TEST_DIRECTORY/../lib on Mac OS X
> compared to prefixing LD_LIBRARY_PATH with the same on other
> systems, so I take your word that it works :D

I just went back and read dyld(1) again. Prefixing would be fine, and I
agree it would look cleaner.

>>
>> Signed-off-by: Charles Celerier <cceleri@cs.stanford.edu>
>> ---
>>  test/T360-symbol-hiding.sh | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/test/T360-symbol-hiding.sh b/test/T360-symbol-hiding.sh
>> index 636ec91..97c734a 100755
>> --- a/test/T360-symbol-hiding.sh
>> +++ b/test/T360-symbol-hiding.sh
>> @@ -12,7 +12,14 @@ test_description='exception symbol hiding'
>>  . ./test-lib.sh
>>  
>>  run_test(){
>> -    result=$(LD_LIBRARY_PATH="$TEST_DIRECTORY/../lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" $TEST_DIRECTORY/symbol-test 2>&1)
>> +    case $(uname -s) in
>> +    Darwin)
>> +        result=$(DYLD_FALLBACK_LIBRARY_PATH="${DYLD_FALLBACK_LIBRARY_PATH:+$DYLD_FALLBACK_LIBRARY_PATH:}$TEST_DIRECTORY/../lib" $TEST_DIRECTORY/symbol-test 2>&1)
>> +        ;;
>> +    *)
>> +        result=$(LD_LIBRARY_PATH="$TEST_DIRECTORY/../lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" $TEST_DIRECTORY/symbol-test 2>&1)
>> +        ;;
>> +    esac
>>  }
>>  
>>  output="A Xapian exception occurred opening database: Couldn't stat 'fakedb/.notmuch/xapian'
>> -- 
>> 1.8.5.2 (Apple Git-48)
>>
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch

[-- Attachment #2: Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 4/5] T360-symbol-hiding: Added code to support testing on Mac OS X.

[Tomi Ollila]

On Tue, May 06 2014, Charles Celerier <cceleri@cs.stanford.edu> wrote:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>
>> On Tue, May 06 2014, Charles Celerier <cceleri@cs.stanford.edu> wrote:
>>
>>> The Mac OS X platform uses *.dylib object files instead of *.so object
>>> files for linking. Adding the path to notmuch.dylib to the end of
>>> DYLD_FALLBACK_LIBRARY_PATH has a similar effect to adding the path to
>>> notmuch.so to LD_LIBRARY_PATH on most Linux-based platforms (see
>>> dyld(1)).
>>
>> This series LGTM. I don't understand this difference suffixing
>> DYLD_FALLBACK_LIBRARY_PATH with $TEST_DIRECTORY/../lib on Mac OS X
>> compared to prefixing LD_LIBRARY_PATH with the same on other
>> systems, so I take your word that it works :D
>
> I just went back and read dyld(1) again. Prefixing would be fine, and I
> agree it would look cleaner.

That would also be less confusing. 

You could send a replacement patch 4/5 and use
id:1399395748-44920-5-git-send-email-cceleri@cs.stanford.edu
as reply-to: let's see how nmbug sorts those patches, then :D

Tomi


>
>>>
>>> Signed-off-by: Charles Celerier <cceleri@cs.stanford.edu>
>>> ---
>>>  test/T360-symbol-hiding.sh | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/test/T360-symbol-hiding.sh b/test/T360-symbol-hiding.sh
>>> index 636ec91..97c734a 100755
>>> --- a/test/T360-symbol-hiding.sh
>>> +++ b/test/T360-symbol-hiding.sh
>>> @@ -12,7 +12,14 @@ test_description='exception symbol hiding'
>>>  . ./test-lib.sh
>>>  
>>>  run_test(){
>>> -    result=$(LD_LIBRARY_PATH="$TEST_DIRECTORY/../lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" $TEST_DIRECTORY/symbol-test 2>&1)
>>> +    case $(uname -s) in
>>> +    Darwin)
>>> +        result=$(DYLD_FALLBACK_LIBRARY_PATH="${DYLD_FALLBACK_LIBRARY_PATH:+$DYLD_FALLBACK_LIBRARY_PATH:}$TEST_DIRECTORY/../lib" $TEST_DIRECTORY/symbol-test 2>&1)
>>> +        ;;
>>> +    *)
>>> +        result=$(LD_LIBRARY_PATH="$TEST_DIRECTORY/../lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" $TEST_DIRECTORY/symbol-test 2>&1)
>>> +        ;;
>>> +    esac
>>>  }
>>>  
>>>  output="A Xapian exception occurred opening database: Couldn't stat 'fakedb/.notmuch/xapian'
>>> -- 
>>> 1.8.5.2 (Apple Git-48)
>>>
>>> _______________________________________________
>>> notmuch mailing list
>>> notmuch@notmuchmail.org
>>> http://notmuchmail.org/mailman/listinfo/notmuch
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 3/5] atomicity.gdb: Allow breakpoint symbols to be resolved later.

[Tomi Ollila]

On Tue, May 06 2014, Charles Celerier <cceleri@cs.stanford.edu> wrote:

> On the Mac OS X platform, the rename() function symbol is not found
> until the debugger begins running. The reason for this is unknown, but
> allowing breakpoint symbols to be resolved later both solves the problem
> and does not change the test.
>
> Signed-off-by: Charles Celerier <cceleri@cs.stanford.edu>
> ---
>  test/atomicity.gdb | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/test/atomicity.gdb b/test/atomicity.gdb
> index fd67525..127dc8f 100644
> --- a/test/atomicity.gdb
> +++ b/test/atomicity.gdb
> @@ -18,6 +18,9 @@ shell echo 0 > outcount
>  
>  shell touch inodes
>  
> +# for gdb on mac
> +set breakpoint pending on
> +

actually this particular patch could be taken from David's series
as it has better comment:

id:1399381588-26271-2-git-send-email-david@tethera.net

>  break rename
>  commands
>  # As an optimization, only consider snapshots after a Xapian commit.
> -- 
> 1.8.5.2 (Apple Git-48)
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/5] configure, test: Added variables for paths to true and false.

[David Bremner]

Charles Celerier <cceleri@cs.stanford.edu> writes:

> The path to true may not be the same on all platforms (e.g. on Mac OS X
> it is /usr/bin/true), so the hard-coded path of /bin/true is not
> portable. This is resolved by adding a step to the configure script to
> locate the path of true and to use the TRUE variable wherever /bin/true
> was needed. The same was done for false.

Does this really need a configure script variable? It seems like
overkill.

d

Re: [PATCH 2/5] configure, test: Added variables for paths to true and false.

[Charles Celerier]

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

David Bremner <david@tethera.net> writes:

> Charles Celerier <cceleri@cs.stanford.edu> writes:
>
>> The path to true may not be the same on all platforms (e.g. on Mac OS X
>> it is /usr/bin/true), so the hard-coded path of /bin/true is not
>> portable. This is resolved by adding a step to the configure script to
>> locate the path of true and to use the TRUE variable wherever /bin/true
>> was needed. The same was done for false.
>
> Does this really need a configure script variable? It seems like
> overkill.

I will be the first too admit that I do not know much about configure
scripts, but adding a TRUE variable seemed straightforward. The problem
is that you want to make softlinks for test/have-man and
test/have-compact for how they are used in tests T010-help-test and
T020-compact; making softlinks requires full paths. You could use
`$(which true)` in test/Makefile.local, but I felt the code repetition
there was unecessary since you could just create a TRUE variable in the
configure script. An alternative is to change the tests so that the
location of true (and false) is no longer an issue.

chuck

[-- Attachment #2: Type: application/pgp-signature, Size: 494 bytes --]

Re: [PATCH 2/5] configure, test: Added variables for paths to true and false.

[David Bremner]

Charles Celerier <cceleri@cs.stanford.edu> writes:

> I will be the first too admit that I do not know much about configure
> scripts, but adding a TRUE variable seemed straightforward. 

configure is already big enough, I'd prefer not to add new things unless
they are needed.  This is not likely to be something the user wants to
change, and (as you note below) it's pretty easy for the the Makefile or
tests to guess.

>  You could use `$(which true)` in test/Makefile.local, but I felt the
> code repetition there was unecessary since you could just create a
> TRUE variable in the configure script.

well, it's not really code repetition because it is only used in that
one place.

> An alternative is to change the tests so that the location of true
> (and false) is no longer an issue.

Sure, that would be fine too.

d

Re: [PATCH 1/5] test/Makefile.local: Added configured TALLOC_LDFLAGS.

[David Bremner]

Charles Celerier <cceleri@cs.stanford.edu> writes:

> The linking to talloc is hard-coded in the testing Makefile. This patch
> causes the linking to talloc to be done according to how TALLOC_LDFLAGS
> was configured.

pushed to release and master.

d

[PATCH v2 4/5] T360-symbol-hiding: Added code to support testing on Mac OS X.

[Charles Celerier]

The Mac OS X platform uses *.dylib object files instead of *.so object
files for linking. Adding the path to notmuch.dylib to the end of
DYLD_FALLBACK_LIBRARY_PATH has a similar effect to adding the path to
notmuch.so to LD_LIBRARY_PATH on most Linux-based platforms (see
dyld(1)).

Signed-off-by: Charles Celerier <cceleri@cs.stanford.edu>
---
 test/T360-symbol-hiding.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/test/T360-symbol-hiding.sh b/test/T360-symbol-hiding.sh
index 636ec91..9239fc1 100755
--- a/test/T360-symbol-hiding.sh
+++ b/test/T360-symbol-hiding.sh
@@ -12,7 +12,14 @@ test_description='exception symbol hiding'
 . ./test-lib.sh
 
 run_test(){
-    result=$(LD_LIBRARY_PATH="$TEST_DIRECTORY/../lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" $TEST_DIRECTORY/symbol-test 2>&1)
+    case $(uname -s) in
+    Darwin)
+        result=$(DYLD_FALLBACK_LIBRARY_PATH="$TEST_DIRECTORY/../lib${DYLD_FALLBACK_LIBRARY_PATH:+:$DYLD_FALLBACK_LIBRARY_PATH}" $TEST_DIRECTORY/symbol-test 2>&1)
+        ;;
+    *)
+        result=$(LD_LIBRARY_PATH="$TEST_DIRECTORY/../lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" $TEST_DIRECTORY/symbol-test 2>&1)
+        ;;
+    esac
 }
 
 output="A Xapian exception occurred opening database: Couldn't stat 'fakedb/.notmuch/xapian'
-- 
1.8.5.2 (Apple Git-48)

[PATCH v2 5/5] T360-symbol-hiding: Use nm instead of objdump.

[Charles Celerier]

The output of `objdump -t` depends on the format of the object files
which are different across platforms (e.g. Mac OS X). Since we really
just want to filter the symbols in the object file, nm is a more
appropriate tool since it only lists symbols from object files (nm(1))
and has a consistent output format.

Signed-off-by: Charles Celerier <cceleri@cs.stanford.edu>
---
 test/T360-symbol-hiding.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/T360-symbol-hiding.sh b/test/T360-symbol-hiding.sh
index 9239fc1..21cabca 100755
--- a/test/T360-symbol-hiding.sh
+++ b/test/T360-symbol-hiding.sh
@@ -33,7 +33,7 @@ test_begin_subtest 'checking output'
 test_expect_equal "$result" "$output"
 
 test_begin_subtest 'comparing existing to exported symbols'
-objdump -t $TEST_DIRECTORY/../lib/*.o | awk '$4 == ".text" && $6 ~ "^notmuch" {print $6}' | sort | uniq > ACTUAL
+nm -g $TEST_DIRECTORY/../lib/*.o | sed -n 's/.*\s\+T\s\+_\(notmuch_.*\)/\1/p' | sort | uniq > ACTUAL
 sed -n 's/[[:blank:]]*\(notmuch_[^;]*\);/\1/p' $TEST_DIRECTORY/../notmuch.sym | sort | uniq > EXPORTED
 test_expect_equal_file EXPORTED ACTUAL
 
-- 
1.8.5.2 (Apple Git-48)

Re: [PATCH v2 5/5] T360-symbol-hiding: Use nm instead of objdump.

[David Bremner]

Charles Celerier <cceleri@cs.stanford.edu> writes:
>  test_begin_subtest 'comparing existing to exported symbols'
> -objdump -t $TEST_DIRECTORY/../lib/*.o | awk '$4 == ".text" && $6 ~ "^notmuch" {print $6}' | sort | uniq > ACTUAL
> +nm -g $TEST_DIRECTORY/../lib/*.o | sed -n 's/.*\s\+T\s\+_\(notmuch_.*\)/\1/p' | sort | uniq > ACTUAL
>  sed -n 's/[[:blank:]]*\(notmuch_[^;]*\);/\1/p' $TEST_DIRECTORY/../notmuch.sym | sort | uniq > EXPORTED

Hmm. It seems like the _ there is wrong. It grabs all of the symbols
starting with _notmuch, which are symbols we _don't_ want exported.
It makes me wonder what ends up in "notmuch.sym" on MacOS, if that test
passes for you.

d

Re: [PATCH v2 5/5] T360-symbol-hiding: Use nm instead of objdump.

[Charles Celerier]

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

David Bremner <david@tethera.net> writes:

> Charles Celerier <cceleri@cs.stanford.edu> writes:
>>  test_begin_subtest 'comparing existing to exported symbols'
>> -objdump -t $TEST_DIRECTORY/../lib/*.o | awk '$4 == ".text" && $6 ~ "^notmuch" {print $6}' | sort | uniq > ACTUAL
>> +nm -g $TEST_DIRECTORY/../lib/*.o | sed -n 's/.*\s\+T\s\+_\(notmuch_.*\)/\1/p' | sort | uniq > ACTUAL
>>  sed -n 's/[[:blank:]]*\(notmuch_[^;]*\);/\1/p' $TEST_DIRECTORY/../notmuch.sym | sort | uniq > EXPORTED
>
> Hmm. It seems like the _ there is wrong. It grabs all of the symbols
> starting with _notmuch, which are symbols we _don't_ want exported.
> It makes me wonder what ends up in "notmuch.sym" on MacOS, if that test
> passes for you.

Ok. Apologies in advance for the verbose content of this email. Here is my notmuch.sym:

    $ cat notmuch.sym
    {
    global:
	__ZTIN6Xapian10LogicErrorE;
	__ZTIN6Xapian12RuntimeErrorE;
	__ZTIN6Xapian16DocNotFoundErrorE;
	__ZTIN6Xapian20InvalidArgumentErrorE;
	__ZTIN6Xapian5ErrorE;
	__ZTSN6Xapian10LogicErrorE;
	__ZTSN6Xapian12RuntimeErrorE;
	__ZTSN6Xapian16DocNotFoundErrorE;
	__ZTSN6Xapian20InvalidArgumentErrorE;
	__ZTSN6Xapian5ErrorE;
     notmuch_status_to_string;
     notmuch_database_create;
     notmuch_database_open;
     notmuch_database_close;
     notmuch_database_compact;
     notmuch_database_destroy;
     notmuch_database_get_path;
     notmuch_database_get_version;
     notmuch_database_needs_upgrade;
     notmuch_database_upgrade;
     notmuch_database_begin_atomic;
     notmuch_database_end_atomic;
     notmuch_database_get_directory;
     notmuch_database_add_message;
     notmuch_database_remove_message;
     notmuch_database_find_message;
     notmuch_database_find_message_by_filename;
     notmuch_database_get_all_tags;
     notmuch_query_create;
     notmuch_query_get_query_string;
     notmuch_query_set_omit_excluded;
     notmuch_query_set_sort;
     notmuch_query_get_sort;
     notmuch_query_add_tag_exclude;
     notmuch_query_search_threads;
     notmuch_query_search_messages;
     notmuch_query_destroy;
     notmuch_threads_valid;
     notmuch_threads_get;
     notmuch_threads_move_to_next;
     notmuch_threads_destroy;
     notmuch_query_count_messages;
     notmuch_query_count_threads;
     notmuch_thread_get_thread_id;
     notmuch_thread_get_total_messages;
     notmuch_thread_get_toplevel_messages;
     notmuch_thread_get_messages;
     notmuch_thread_get_matched_messages;
     notmuch_thread_get_authors;
     notmuch_thread_get_subject;
     notmuch_thread_get_oldest_date;
     notmuch_thread_get_newest_date;
     notmuch_thread_get_tags;
     notmuch_thread_destroy;
     notmuch_messages_valid;
     notmuch_messages_get;
     notmuch_messages_move_to_next;
     notmuch_messages_destroy;
     notmuch_messages_collect_tags;
     notmuch_message_get_message_id;
     notmuch_message_get_thread_id;
     notmuch_message_get_replies;
     notmuch_message_get_filename;
     notmuch_message_get_filenames;
     notmuch_message_get_flag;
     notmuch_message_set_flag;
     notmuch_message_get_date;
     notmuch_message_get_header;
     notmuch_message_get_tags;
     notmuch_message_add_tag;
     notmuch_message_remove_tag;
     notmuch_message_remove_all_tags;
     notmuch_message_maildir_flags_to_tags;
     notmuch_message_tags_to_maildir_flags;
     notmuch_message_freeze;
     notmuch_message_thaw;
     notmuch_message_destroy;
     notmuch_tags_valid;
     notmuch_tags_get;
     notmuch_tags_move_to_next;
     notmuch_tags_destroy;
     notmuch_directory_set_mtime;
     notmuch_directory_get_mtime;
     notmuch_directory_get_child_files;
     notmuch_directory_get_child_directories;
     notmuch_directory_destroy;
     notmuch_filenames_valid;
     notmuch_filenames_get;
     notmuch_filenames_move_to_next;
     notmuch_filenames_destroy;
    local: *;
    };

The output of the test in question (T360-symbol-hiding) after
applying all of the patches in this series is

    T360-symbol-hiding: Testing exception symbol hiding
     PASS   running test
     PASS   checking output
     FAIL   comparing existing to exported symbols
	--- T360-symbol-hiding.3.EXPORTED   2014-05-08
     14:48:52.000000000 +0000
	+++ T360-symbol-hiding.3.ACTUAL 2014-05-08 14:48:52.000000000
     +0000
	@@ -26,7 +26,11 @@
	 notmuch_filenames_valid
	 notmuch_message_add_tag
	 notmuch_message_destroy
	+notmuch_message_file_close
	+notmuch_message_file_get_header
	+notmuch_message_file_open
	 notmuch_message_freeze
	+notmuch_message_get_author
	 notmuch_message_get_date
	 notmuch_message_get_filename
	 notmuch_message_get_filenames
	@@ -39,6 +43,7 @@
	 notmuch_message_maildir_flags_to_tags
	 notmuch_message_remove_all_tags
	 notmuch_message_remove_tag
	+notmuch_message_set_author
	 notmuch_message_set_flag
	 notmuch_message_tags_to_maildir_flags
	 notmuch_message_thaw
	@@ -58,6 +63,8 @@
	 notmuch_query_search_threads
	 notmuch_query_set_omit_excluded
	 notmuch_query_set_sort
	+notmuch_sha1_of_file
	+notmuch_sha1_of_string
	 notmuch_status_to_string
	 notmuch_tags_destroy
	 notmuch_tags_get

This output was a clear motivation for the patch in
id:1399402716-13714-1-git-send-email-cceleri@cs.stanford.edu.

Here is some of output of the matches made on the output of nm:

    $ nm -g test/../lib/*.o | sed -n '/.*\s\+T\s\+_\(notmuch_.*\)/p'
    00000000000028c0 T _notmuch_database_add_message
    0000000000002280 T _notmuch_database_begin_atomic
    0000000000001af0 T _notmuch_database_close
    0000000000001de0 T _notmuch_database_compact
    0000000000000300 T _notmuch_database_create
    0000000000001db0 T _notmuch_database_destroy
    0000000000002300 T _notmuch_database_end_atomic
    ...

chuck

[-- Attachment #2: Type: application/pgp-signature, Size: 494 bytes --]

Re: [PATCH v2 5/5] T360-symbol-hiding: Use nm instead of objdump.

[David Bremner]

Charles Celerier <cceleri@cs.stanford.edu> writes:

> David Bremner <david@tethera.net> writes:
>
>> Charles Celerier <cceleri@cs.stanford.edu> writes:
>>>  test_begin_subtest 'comparing existing to exported symbols'
>>> -objdump -t $TEST_DIRECTORY/../lib/*.o | awk '$4 == ".text" && $6 ~ "^notmuch" {print $6}' | sort | uniq > ACTUAL
>>> +nm -g $TEST_DIRECTORY/../lib/*.o | sed -n 's/.*\s\+T\s\+_\(notmuch_.*\)/\1/p' | sort | uniq > ACTUAL
>>>  sed -n 's/[[:blank:]]*\(notmuch_[^;]*\);/\1/p' $TEST_DIRECTORY/../notmuch.sym | sort | uniq > EXPORTED
>>
>> Hmm. It seems like the _ there is wrong. It grabs all of the symbols
>> starting with _notmuch, which are symbols we _don't_ want exported.
>> It makes me wonder what ends up in "notmuch.sym" on MacOS, if that test
>> passes for you.
>
> Ok. Apologies in advance for the verbose content of this email. Here is my notmuch.sym:

Actually, that's perfect. I think I finally understand what's going on,
if not completely how to fix it yet.

>
>     $ cat notmuch.sym
>     {
>     global:

this looks fine; only mangled C++ stuff is different.

> The output of the test in question (T360-symbol-hiding) after
> applying all of the patches in this series is
>
>     T360-symbol-hiding: Testing exception symbol hiding
>      PASS   running test
>      PASS   checking output
>      FAIL   comparing existing to exported symbols
> 	--- T360-symbol-hiding.3.EXPORTED   2014-05-08
>      14:48:52.000000000 +0000
> 	+++ T360-symbol-hiding.3.ACTUAL 2014-05-08 14:48:52.000000000
>      +0000
> 	@@ -26,7 +26,11 @@
> 	 notmuch_filenames_valid
> 	 notmuch_message_add_tag
> 	 notmuch_message_destroy
> 	+notmuch_message_file_close
> 	+notmuch_message_file_get_header
> 	+notmuch_message_file_open
>

OK, what's happening here is that your version of the test, I guess
using nm, is not skipping hidden symbols. This should probably be better
documented in the existing test, but in the case of hidden symbols,
$6 is ".hidden".  I don't know if nm understands symbol visibility at
all; I'd guess not.

So as a more portable workaround, I agree we could start being more
rigorous about not naming things "^notmuch_.*" unless they are intended
to be exported.

> This output was a clear motivation for the patch in
> id:1399402716-13714-1-git-send-email-cceleri@cs.stanford.edu.

I see that now. I think I'd prefer a more minimal patch that only adds _
to the front of symbols currently starting with notmuch.

> Here is some of output of the matches made on the output of nm:
>
>     $ nm -g test/../lib/*.o | sed -n '/.*\s\+T\s\+_\(notmuch_.*\)/p'
>     00000000000028c0 T _notmuch_database_add_message
>     0000000000002280 T _notmuch_database_begin_atomic
>     0000000000001af0 T _notmuch_database_close
>     0000000000001de0 T _notmuch_database_compact

With GNU nm, there is no leading _ in front of notmuch here, which is
what causes your version of the test to fail for me.

d

Re: [PATCH v2 5/5] T360-symbol-hiding: Use nm instead of objdump.

[Charles Celerier]

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

David Bremner <david@tethera.net> writes:

> Charles Celerier <cceleri@cs.stanford.edu> writes:
>
>> Here is some of output of the matches made on the output of nm:
>>
>>     $ nm -g test/../lib/*.o | sed -n '/.*\s\+T\s\+_\(notmuch_.*\)/p'
>>     00000000000028c0 T _notmuch_database_add_message
>>     0000000000002280 T _notmuch_database_begin_atomic
>>     0000000000001af0 T _notmuch_database_close
>>     0000000000001de0 T _notmuch_database_compact
>
> With GNU nm, there is no leading _ in front of notmuch here, which is
> what causes your version of the test to fail for me.

What version of GNU nm are you using?

    $ nm --version
    GNU nm (GNU Binutils) 2.24
    Copyright 2013 Free Software Foundation, Inc.
    This program is free software; you may redistribute it under the terms of
    the GNU General Public License version 3 or (at your option) any later version.
    This program has absolutely no warranty.

I'm not convinced the insertion of an underscore is nm's doing.

At this point, I'm not sure how to create a better version of this
patch. Are we renaming functions in notmuch-private.h? Should we stick
with objdump or switch to using nm?

chuck

[-- Attachment #2: Type: application/pgp-signature, Size: 494 bytes --]

Re: [PATCH v2 5/5] T360-symbol-hiding: Use nm instead of objdump.

[David Bremner]

Charles Celerier <cceleri@cs.stanford.edu> writes:

> David Bremner <david@tethera.net> writes:
>
>     $ nm --version
>     GNU nm (GNU Binutils) 2.24
>     Copyright 2013 Free Software Foundation, Inc.
>     This program is free software; you may redistribute it under the terms of
>     the GNU General Public License version 3 or (at your option) any later version.
>     This program has absolutely no warranty.
>
> I'm not convinced the insertion of an underscore is nm's doing.

OIC. right, nm is not the issue.

> At this point, I'm not sure how to create a better version of this
> patch. Are we renaming functions in notmuch-private.h? Should we stick
> with objdump or switch to using nm?

I'd go with renaming any functions that start with notmuch to start with
_notmuch.

Since it seems neither nm nor objdump provides uniform output between
OS/X and Linux, there isn't an obvious advantage to switching to nm. Any
idea if objdump can be made to work (in this test) on OS/X ?

d

Re: [PATCH v2 5/5] T360-symbol-hiding: Use nm instead of objdump.

[Charles Celerier]

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

David Bremner <david@tethera.net> writes:

> Charles Celerier <cceleri@cs.stanford.edu> writes:
>
>> At this point, I'm not sure how to create a better version of this
>> patch. Are we renaming functions in notmuch-private.h? Should we stick
>> with objdump or switch to using nm?
>
> I'd go with renaming any functions that start with notmuch to start with
> _notmuch.

Ok. Should I pull the patch from
id:1399402716-13714-1-git-send-email-cceleri@cs.stanford.edu into this
patch series?

> Since it seems neither nm nor objdump provides uniform output between
> OS/X and Linux, there isn't an obvious advantage to switching to nm. Any
> idea if objdump can be made to work (in this test) on OS/X ?

I can install GNU objdump on OS/X, but the problem seems to be that
objdump changes its output depending on the object file format (see -t
option documentation in objdump(1)). What does the output of nm look
like for you? Is really that different? Here is what my objdump output
looks like:

    $ objdump -t lib/*.o | sed -n '/\[\.text\] __\?notmuch/p' | tail
    00000000000009a0 g       0f SECT   01 0000 [.text] _notmuch_thread_get_authors
    0000000000000990 g       0f SECT   01 0000 [.text] _notmuch_thread_get_matched_messages
    0000000000000960 g       0f SECT   01 0000 [.text] _notmuch_thread_get_messages
    00000000000009d0 g       0f SECT   01 0000 [.text] _notmuch_thread_get_newest_date
    00000000000009c0 g       0f SECT   01 0000 [.text] _notmuch_thread_get_oldest_date
    00000000000009b0 g       0f SECT   01 0000 [.text] _notmuch_thread_get_subject
    00000000000009e0 g       0f SECT   01 0000 [.text] _notmuch_thread_get_tags
    0000000000000970 g       0f SECT   01 0000 [.text] _notmuch_thread_get_thread_id
    0000000000000950 g       0f SECT   01 0000 [.text] _notmuch_thread_get_toplevel_messages
    0000000000000980 g       0f SECT   01 0000 [.text] _notmuch_thread_get_total_messages

I suppose we could just account for the difference in output of objdump
on OS/X, but I was hoping that the output of nm would be more consistent
since its a simpler program.

chuck

[-- Attachment #2: Type: application/pgp-signature, Size: 494 bytes --]

Re: [PATCH v2 5/5] T360-symbol-hiding: Use nm instead of objdump.

[David Bremner]

Charles Celerier <cceleri@cs.stanford.edu> writes:

> David Bremner <david@tethera.net> writes:
>
>     $ objdump -t lib/*.o | sed -n '/\[\.text\] __\?notmuch/p' | tail
>     00000000000009a0 g       0f SECT   01 0000 [.text] _notmuch_thread_get_authors
>     0000000000000990 g       0f SECT   01 0000 [.text] _notmuch_thread_get_matched_messages
>     0000000000000960 g       0f SECT   01 0000 [.text] _notmuch_thread_get_messages
>     00000000000009d0 g       0f SECT   01 0000 [.text] _notmuch_thread_get_newest_date

Here is some equivalent output from Linux

$ objdump -t lib/*.o | sed -n '/[.]text.*notmuch/p' | tail -12

0000000000000150 g     F .text	000000000000095e .hidden _notmuch_thread_create
0000000000000ab0 g     F .text	0000000000000009 notmuch_thread_get_toplevel_messages
0000000000000ac0 g     F .text	0000000000000009 notmuch_thread_get_messages

...

Notice in particular the ".hidden" in the first line. Also, the lack of
extra _ on the front.  

It may be that the visibility information is not accessible on OS-X
using objdump; this is something different than whether a symbol is
extern. The symbols marked .hidden can be used across compilation unit
boundaries, but will not be exported from the shared library.

Re: [PATCH v2 4/5] T360-symbol-hiding: Added code to support testing on Mac OS X.

[David Bremner]

Charles Celerier <cceleri@cs.stanford.edu> writes:

> The Mac OS X platform uses *.dylib object files instead of *.so object
> files for linking. Adding the path to notmuch.dylib to the end of
> DYLD_FALLBACK_LIBRARY_PATH has a similar effect to adding the path to
> notmuch.so to LD_LIBRARY_PATH on most Linux-based platforms (see
> dyld(1)).

I was about to push this, but it occured to me that until we figure out
what to do about nm/objdump there wasn't much point.

d