FreeBSD Support Patches

FreeBSD Support Patches

[Mike Kelly]

Hi,

These patches add some changes necessary for a clean build on FreeBSD,
and for most of the tests to pass.

Also mixed in are a few patches to disable emacs tests when you don't
have emacs, along with tests that depend upon emacs. I could split those
off onto a separate branch if required.

You can find the latest version of these patches on my github repo's
fbsd-support branch:

  https://github.com/pioto/notmuch/compare/master...fbsd-support 

[PATCH 01/10] Add support for FreeBSD.

[Mike Kelly]

---
 configure |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 71981b7..6c45880 100755
--- a/configure
+++ b/configure
@@ -356,6 +356,10 @@ elif [ $uname = "SunOS" ] ; then
     printf "Solaris.\n"
     platform=SOLARIS
     linker_resolves_library_dependencies=0
+elif [ $uname = "FreeBSD" ] ; then
+    printf "FreeBSD.\n"
+    platform=FREEBSD
+    linker_resolves_library_dependencies=0
 elif [ $uname = "Linux" ] || [ $uname = "GNU" ] ; then
     printf "$uname\n"
     platform="$uname"
@@ -645,7 +649,7 @@ HAVE_GETLINE = ${have_getline}
 # build its own version)
 HAVE_STRCASESTR = ${have_strcasestr}
 
-# Supported platforms (so far) are: LINUX, MACOSX, SOLARIS
+# Supported platforms (so far) are: LINUX, MACOSX, SOLARIS, FREEBSD
 PLATFORM = ${platform}
 
 # Whether the linker will automatically resolve the dependency of one
-- 
1.7.10.2

[PATCH 02/10] Fix compilation of symbol-test on FreeBSD

[Mike Kelly]

---
 test/Makefile.local |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/Makefile.local b/test/Makefile.local
index 4a6a4b1..c071aee 100644
--- a/test/Makefile.local
+++ b/test/Makefile.local
@@ -17,7 +17,7 @@ $(dir)/smtp-dummy: $(smtp_dummy_modules)
 	$(call quiet,CC) $^ -o $@
 
 $(dir)/symbol-test: $(dir)/symbol-test.o
-	$(call quiet,CXX) $^ -o $@ -Llib -lnotmuch -lxapian
+	$(call quiet,CXX) $^ -o $@ -Llib -lnotmuch $(XAPIAN_LDFLAGS)
 
 .PHONY: test check
 
-- 
1.7.10.2

[PATCH 03/10] Fix compilation of smtp-dummy on FreeBSD

[Mike Kelly]

---
 test/smtp-dummy.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/test/smtp-dummy.c b/test/smtp-dummy.c
index 3801a5e..aa82fa1 100644
--- a/test/smtp-dummy.c
+++ b/test/smtp-dummy.c
@@ -33,11 +33,14 @@
  * have been warned.
  */
 
+#define _GNU_SOURCE /* for getline */
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <errno.h>
-#include <netinet/ip.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
 #include <netdb.h>
 #include <unistd.h>
 
-- 
1.7.10.2

Re: [PATCH 03/10] Fix compilation of smtp-dummy on FreeBSD

[Jani Nikula]

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

On May 25, 2012 4:44 PM, "Mike Kelly" <pioto@pioto.org> wrote:
>
> ---
>  test/smtp-dummy.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/test/smtp-dummy.c b/test/smtp-dummy.c
> index 3801a5e..aa82fa1 100644
> --- a/test/smtp-dummy.c
> +++ b/test/smtp-dummy.c
> @@ -33,11 +33,14 @@
>  * have been warned.
>  */
>
> +#define _GNU_SOURCE /* for getline */

First of all, thanks for sharing your work.

Independent of this patch, I've been thinking about defining _GNU_SOURCE in
the makefile. I don't think it's very elegant to #define it everywhere. It
would also pave the way towards building with -pedantic (and possibly
-std=c99) in a portable way. For example, the compat test code #defines
_GNU_SOURCE while the actual code does not, which I think is an error.

I'll look into this after the weekend (unless you beat me to it).

BR,
Jani.

>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <errno.h>
> -#include <netinet/ip.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <netinet/in.h>
>  #include <netdb.h>
>  #include <unistd.h>
>
> --
> 1.7.10.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

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

[PATCH 04/10] Test against man pages in our source dir, not installed ones.

[Mike Kelly]

---
 test/help-test |    1 +
 1 file changed, 1 insertion(+)

diff --git a/test/help-test b/test/help-test
index 9f4b9c7..f309a24 100755
--- a/test/help-test
+++ b/test/help-test
@@ -3,6 +3,7 @@
 test_description="online help"
 . test-lib.sh
 
+export MANPATH="../../man"
 test_expect_success 'notmuch --help' 'notmuch --help'
 test_expect_success 'notmuch --help tag' 'notmuch --help tag'
 test_expect_success 'notmuch help' 'notmuch help'
-- 
1.7.10.2

[PATCH 05/10] Make test/basic more portable

[Mike Kelly]

---
 test/basic |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/basic b/test/basic
index d6aed24..4ce8967 100755
--- a/test/basic
+++ b/test/basic
@@ -53,7 +53,8 @@ test_expect_code 2 'failure to clean up causes the test to fail' '
 test_begin_subtest 'Ensure that all available tests will be run by notmuch-test'
 eval $(sed -n -e '/^TESTS="$/,/^"$/p' $TEST_DIRECTORY/notmuch-test)
 tests_in_suite=$(for i in $TESTS; do echo $i; done | sort)
-available=$(find "$TEST_DIRECTORY" -maxdepth 1 -type f -executable -printf '%f\n' | \
+available=$(find "$TEST_DIRECTORY" -maxdepth 1 -type f -perm +111 | \
+    sed -e "s,.*/,," | \
     sed -r -e "/^(aggregate-results.sh|notmuch-test|smtp-dummy|test-verbose|symbol-test|arg-test)$/d" | \
     sort)
 test_expect_equal "$tests_in_suite" "$available"
-- 
1.7.10.2

[PATCH 06/10] Fix test/count on FreeBSD

[Mike Kelly]

FreeBSD's `wc -l` includes some white space in front of the number, so
trim it off.
---
 test/count |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/test/count b/test/count
index 300b171..618dbb9 100755
--- a/test/count
+++ b/test/count
@@ -8,22 +8,22 @@ SEARCH="\"*\""
 
 test_begin_subtest "message count is the default for notmuch count"
 test_expect_equal \
-    "`notmuch search --output=messages ${SEARCH} | wc -l`" \
+    "`notmuch search --output=messages ${SEARCH} | wc -l |sed 's/^[[:space:]]*//'`" \
     "`notmuch count ${SEARCH}`"
 
 test_begin_subtest "message count with --output=messages"
 test_expect_equal \
-    "`notmuch search --output=messages ${SEARCH} | wc -l`" \
+    "`notmuch search --output=messages ${SEARCH} | wc -l |sed 's/^[[:space:]]*//'`" \
     "`notmuch count --output=messages ${SEARCH}`"
 
 test_begin_subtest "thread count with --output=threads"
 test_expect_equal \
-    "`notmuch search --output=threads ${SEARCH} | wc -l`" \
+    "`notmuch search --output=threads ${SEARCH} | wc -l |sed 's/^[[:space:]]*//'`" \
     "`notmuch count --output=threads ${SEARCH}`"
 
 test_begin_subtest "thread count is the default for notmuch search"
 test_expect_equal \
-    "`notmuch search ${SEARCH} | wc -l`" \
+    "`notmuch search ${SEARCH} | wc -l |sed 's/^[[:space:]]*//'`" \
     "`notmuch count --output=threads ${SEARCH}`"
 
 SEARCH="from:cworth and not from:cworth"
-- 
1.7.10.2

Re: [PATCH 06/10] Fix test/count on FreeBSD

[Peter Wang]

On Fri, 25 May 2012 09:43:27 -0400, Mike Kelly <pioto@pioto.org> wrote:
> FreeBSD's `wc -l` includes some white space in front of the number, so
> trim it off.
> ---
>  test/count |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/test/count b/test/count
> index 300b171..618dbb9 100755
> --- a/test/count
> +++ b/test/count
> @@ -8,22 +8,22 @@ SEARCH="\"*\""
>  
>  test_begin_subtest "message count is the default for notmuch count"
>  test_expect_equal \
> -    "`notmuch search --output=messages ${SEARCH} | wc -l`" \
> +    "`notmuch search --output=messages ${SEARCH} | wc -l |sed 's/^[[:space:]]*//'`" \
>      "`notmuch count ${SEARCH}`"

You could perform a proper integer comparison like:

test_expect_success "message count is the default for notmuch count" '
    test "`notmuch search --output=messages ${SEARCH} | wc -l`" \
     -eq "`notmuch count ${SEARCH}`"
'

Peter

[PATCH 07/10] emacs tests require emacs

[Mike Kelly]

Immediately skip all emacs tests when we don't have emacs.
---
 test/emacs                     |    2 ++
 test/emacs-address-cleaning    |    2 ++
 test/emacs-hello               |    2 ++
 test/emacs-large-search-buffer |    2 ++
 test/emacs-show                |    2 ++
 test/emacs-subject-to-filename |    2 ++
 test/emacs-test-functions      |    2 ++
 7 files changed, 14 insertions(+)

diff --git a/test/emacs b/test/emacs
index e9f954c..cc42b21 100755
--- a/test/emacs
+++ b/test/emacs
@@ -3,6 +3,8 @@
 test_description="emacs interface"
 . test-lib.sh
 
+test_require_external_prereq emacs
+
 EXPECTED=$TEST_DIRECTORY/emacs.expected-output
 
 add_email_corpus
diff --git a/test/emacs-address-cleaning b/test/emacs-address-cleaning
index 6ddde5c..9a189d5 100755
--- a/test/emacs-address-cleaning
+++ b/test/emacs-address-cleaning
@@ -3,6 +3,8 @@
 test_description="emacs address cleaning"
 . test-lib.sh
 
+test_require_external_prereq emacs
+
 test_begin_subtest "notmuch-test-address-clean part 1"
 test_emacs_expect_t '(notmuch-test-address-cleaning-1)'
 
diff --git a/test/emacs-hello b/test/emacs-hello
index a998dc4..b4ca831 100755
--- a/test/emacs-hello
+++ b/test/emacs-hello
@@ -3,6 +3,8 @@
 test_description="Testing emacs notmuch-hello view"
 . test-lib.sh
 
+test_require_external_prereq emacs
+
 EXPECTED=$TEST_DIRECTORY/emacs.expected-output
 
 add_email_corpus
diff --git a/test/emacs-large-search-buffer b/test/emacs-large-search-buffer
index 4351e33..f5c8927 100755
--- a/test/emacs-large-search-buffer
+++ b/test/emacs-large-search-buffer
@@ -2,6 +2,8 @@
 test_description="Emacs with large search results buffer"
 . test-lib.sh
 
+test_require_external_prereq emacs
+
 x=xxxxxxxxxx # 10
 x=$x$x$x$x$x$x$x$x$x$x # 100
 x=$x$x$x$x$x$x$x$x$x # 900
diff --git a/test/emacs-show b/test/emacs-show
index 5700d2e..b2a5cb2 100755
--- a/test/emacs-show
+++ b/test/emacs-show
@@ -3,6 +3,8 @@
 test_description="Testing emacs notmuch-show view"
 . test-lib.sh
 
+test_require_external_prereq emacs
+
 test_begin_subtest "Hiding Original Message region at beginning of a message"
 message_id='OriginalMessageHiding.1@notmuchmail.org'
 add_message \
diff --git a/test/emacs-subject-to-filename b/test/emacs-subject-to-filename
index 176e685..e8bc3ca 100755
--- a/test/emacs-subject-to-filename
+++ b/test/emacs-subject-to-filename
@@ -3,6 +3,8 @@
 test_description="emacs: mail subject to filename"
 . test-lib.sh
 
+test_require_external_prereq emacs
+
 # emacs server can't be started in a child process with $(test_emacs ...)
 test_emacs '(ignore)'
 
diff --git a/test/emacs-test-functions b/test/emacs-test-functions
index 0e1f9fc..dbbe7e4 100755
--- a/test/emacs-test-functions
+++ b/test/emacs-test-functions
@@ -3,6 +3,8 @@
 test_description="emacs test function sanity"
 . test-lib.sh
 
+test_require_external_prereq emacs
+
 test_begin_subtest "emacs test function sanity"
 test_emacs_expect_t 't'
 
-- 
1.7.10.2

[PATCH 08/10] test/atomicity: use a more portable 'cp' form

[Mike Kelly]

-a already implies -r/-R, so no need for both.

FreeBSD's cp complains:

  cp: the -R and -r options may not be specified together
---
 test/atomicity |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/atomicity b/test/atomicity
index 6df0a00..1c786fa 100755
--- a/test/atomicity
+++ b/test/atomicity
@@ -51,7 +51,7 @@ if test_require_external_prereq gdb; then
 
     # Prepare a snapshot of the updated maildir.  The gdb script will
     # update the database in this snapshot as it goes.
-    cp -ra $MAIL_DIR $MAIL_DIR.snap
+    cp -a $MAIL_DIR $MAIL_DIR.snap
     cp ${NOTMUCH_CONFIG} ${NOTMUCH_CONFIG}.snap
     NOTMUCH_CONFIG=${NOTMUCH_CONFIG}.snap notmuch config set database.path $MAIL_DIR.snap
 
-- 
1.7.10.2

[PATCH 09/10] test/crypto: current tests require emacs

[Mike Kelly]

These tests are based upon emacs_deliver_message, so without emacs they
will not work.
---
 test/crypto |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/test/crypto b/test/crypto
index be752b1..fcaf4be 100755
--- a/test/crypto
+++ b/test/crypto
@@ -7,6 +7,8 @@
 test_description='PGP/MIME signature verification and decryption'
 . ./test-lib.sh
 
+test_require_external_prereq emacs
+
 add_gnupg_home ()
 {
     local output
-- 
1.7.10.2

[PATCH 10/10] tests: bsd sed doesn't allow replacing '\n'

[Mike Kelly]

Various workarounds still don't act quite the same way. So, just use
perl, which seems to act the way these tests expect.
---
 test/maildir-sync |    2 +-
 test/multipart    |   22 +++++++++++-----------
 test/test-lib.sh  |    2 +-
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/test/maildir-sync b/test/maildir-sync
index d72ec07..311ea44 100755
--- a/test/maildir-sync
+++ b/test/maildir-sync
@@ -8,7 +8,7 @@ test_description="maildir synchronization"
 # --format=json" output includes some newlines. Also, need to avoid
 # including the local value of MAIL_DIR in the result.
 filter_show_json() {
-    sed -e 's/, /,\n/g'  | sed -e "s|${MAIL_DIR}/|MAIL_DIR/|"
+    perl -npe 's/, /,\n/g' | sed -e "s|${MAIL_DIR}/|MAIL_DIR/|"
     echo
 }
 
diff --git a/test/multipart b/test/multipart
index 72d3927..c868b7b 100755
--- a/test/multipart
+++ b/test/multipart
@@ -319,7 +319,7 @@ test_expect_success \
     "notmuch show --format=text --part=8 'id:87liy5ap00.fsf@yoom.home.cworth.org'"
 
 test_begin_subtest "--format=json --part=0, full message"
-notmuch show --format=json --part=0 'id:87liy5ap00.fsf@yoom.home.cworth.org' | sed 's|{"id":|\n{"id":|g' >OUTPUT
+notmuch show --format=json --part=0 'id:87liy5ap00.fsf@yoom.home.cworth.org' | perl -npe 's|{"id":|\n{"id":|g' >OUTPUT
 echo >>OUTPUT # expect *no* newline at end of output
 cat <<EOF >EXPECTED
 
@@ -337,7 +337,7 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "--format=json --part=1, message body"
-notmuch show --format=json --part=1 'id:87liy5ap00.fsf@yoom.home.cworth.org' | sed 's|{"id":|\n{"id":|g' >OUTPUT
+notmuch show --format=json --part=1 'id:87liy5ap00.fsf@yoom.home.cworth.org' | perl -npe 's|{"id":|\n{"id":|g' >OUTPUT
 echo >>OUTPUT # expect *no* newline at end of output
 cat <<EOF >EXPECTED
 
@@ -354,7 +354,7 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "--format=json --part=2, multipart/mixed"
-notmuch show --format=json --part=2 'id:87liy5ap00.fsf@yoom.home.cworth.org' | sed 's|{"id":|\n{"id":|g' >OUTPUT
+notmuch show --format=json --part=2 'id:87liy5ap00.fsf@yoom.home.cworth.org' | perl -npe 's|{"id":|\n{"id":|g' >OUTPUT
 echo >>OUTPUT # expect *no* newline at end of output
 cat <<EOF >EXPECTED
 
@@ -369,7 +369,7 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "--format=json --part=3, rfc822 part"
-notmuch show --format=json --part=3 'id:87liy5ap00.fsf@yoom.home.cworth.org' | sed 's|{"id":|\n{"id":|g' >OUTPUT
+notmuch show --format=json --part=3 'id:87liy5ap00.fsf@yoom.home.cworth.org' | perl -npe 's|{"id":|\n{"id":|g' >OUTPUT
 echo >>OUTPUT # expect *no* newline at end of output
 cat <<EOF >EXPECTED
 
@@ -381,7 +381,7 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "--format=json --part=4, rfc822's multipart/alternative"
-notmuch show --format=json --part=4 'id:87liy5ap00.fsf@yoom.home.cworth.org' | sed 's|{"id":|\n{"id":|g' >OUTPUT
+notmuch show --format=json --part=4 'id:87liy5ap00.fsf@yoom.home.cworth.org' | perl -npe 's|{"id":|\n{"id":|g' >OUTPUT
 echo >>OUTPUT # expect *no* newline at end of output
 cat <<EOF >EXPECTED
 
@@ -392,7 +392,7 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "--format=json --part=5, rfc822's html part"
-notmuch show --format=json --part=5 'id:87liy5ap00.fsf@yoom.home.cworth.org' | sed 's|{"id":|\n{"id":|g' >OUTPUT
+notmuch show --format=json --part=5 'id:87liy5ap00.fsf@yoom.home.cworth.org' | perl -npe 's|{"id":|\n{"id":|g' >OUTPUT
 echo >>OUTPUT # expect *no* newline at end of output
 cat <<EOF >EXPECTED
 
@@ -401,7 +401,7 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "--format=json --part=6, rfc822's text part"
-notmuch show --format=json --part=6 'id:87liy5ap00.fsf@yoom.home.cworth.org' | sed 's|{"id":|\n{"id":|g' >OUTPUT
+notmuch show --format=json --part=6 'id:87liy5ap00.fsf@yoom.home.cworth.org' | perl -npe 's|{"id":|\n{"id":|g' >OUTPUT
 echo >>OUTPUT # expect *no* newline at end of output
 cat <<EOF >EXPECTED
 
@@ -410,7 +410,7 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "--format=json --part=7, inline attachment"
-notmuch show --format=json --part=7 'id:87liy5ap00.fsf@yoom.home.cworth.org' | sed 's|{"id":|\n{"id":|g' >OUTPUT
+notmuch show --format=json --part=7 'id:87liy5ap00.fsf@yoom.home.cworth.org' | perl -npe 's|{"id":|\n{"id":|g' >OUTPUT
 echo >>OUTPUT # expect *no* newline at end of output
 cat <<EOF >EXPECTED
 
@@ -419,7 +419,7 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "--format=json --part=8, plain text part"
-notmuch show --format=json --part=8 'id:87liy5ap00.fsf@yoom.home.cworth.org' | sed 's|{"id":|\n{"id":|g' >OUTPUT
+notmuch show --format=json --part=8 'id:87liy5ap00.fsf@yoom.home.cworth.org' | perl -npe 's|{"id":|\n{"id":|g' >OUTPUT
 echo >>OUTPUT # expect *no* newline at end of output
 cat <<EOF >EXPECTED
 
@@ -428,7 +428,7 @@ EOF
 test_expect_equal_file OUTPUT EXPECTED
 
 test_begin_subtest "--format=json --part=9, pgp signature (unverified)"
-notmuch show --format=json --part=9 'id:87liy5ap00.fsf@yoom.home.cworth.org' | sed 's|{"id":|\n{"id":|g' >OUTPUT
+notmuch show --format=json --part=9 'id:87liy5ap00.fsf@yoom.home.cworth.org' | perl -npe 's|{"id":|\n{"id":|g' >OUTPUT
 echo >>OUTPUT # expect *no* newline at end of output
 cat <<EOF >EXPECTED
 
@@ -669,4 +669,4 @@ notmuch show --format=raw --part=3 id:base64-part-with-crlf > crlf.out
 echo -n -e "\xEF\x0D\x0A" > crlf.expected
 test_expect_equal_file crlf.out crlf.expected
 
-test_done
\ No newline at end of file
+test_done
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 06aaea2..4990ad6 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -565,7 +565,7 @@ notmuch_show_sanitize_all ()
 
 notmuch_json_show_sanitize ()
 {
-    sed -e 's|, |,\n |g' | \
+    perl -npe 's|, |,\n |g' | \
 	sed \
 	-e 's|"id": "[^"]*",|"id": "XXXXX",|' \
 	-e 's|"filename": "[^"]*",|"filename": "YYYYY",|'
-- 
1.7.10.2

Re: FreeBSD Support Patches

[Dmitry Kurochkin]

Hi Mike.

Mike Kelly <pioto@pioto.org> writes:

> Hi,
>
> These patches add some changes necessary for a clean build on FreeBSD,
> and for most of the tests to pass.
>
> Also mixed in are a few patches to disable emacs tests when you don't
> have emacs, along with tests that depend upon emacs. I could split those
> off onto a separate branch if required.
>

Why do we need to explicitly declare Emacs dependency for tests?  There
should be no need for it.  We have "implicit" dependencies which are
declared once (see test_declare_external_prereq calls at the end of
test-lib.sh) and are automatically handled when a test tries to use a
missing binary.  Explicit dependencies are hard to maintain (e.g. your
patch adds explicit emacs dependency for crypto test but misses gpg).
With rare exceptions we should not use explicit dependencies.

Regards,
  Dmitry

> You can find the latest version of these patches on my github repo's
> fbsd-support branch:
>
>   https://github.com/pioto/notmuch/compare/master...fbsd-support 
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: FreeBSD Support Patches

[Mike Kelly]

On Fri, May 25, 2012 at 10:15 AM, Dmitry Kurochkin
<dmitry.kurochkin@gmail.com> wrote:
> Why do we need to explicitly declare Emacs dependency for tests?  There
> should be no need for it.  We have "implicit" dependencies which are
> declared once (see test_declare_external_prereq calls at the end of
> test-lib.sh) and are automatically handled when a test tries to use a
> missing binary.  Explicit dependencies are hard to maintain (e.g. your
> patch adds explicit emacs dependency for crypto test but misses gpg).
> With rare exceptions we should not use explicit dependencies.

Because not every test actually has those implicit dependencies. For
example, some of the crypto tests depend upon emacs_deliver_message
working correctly for subsequents tests. Those emacs_deliver_message
tests are skipped, but not the ones after it that try to do something
with that injected message.

For the emacs-* test files, there are some tests that act the same
way. However, it is also a minor speed improvement to say that,
obviously, none of the emacs tests are going to work, so just don't
bother.

-- 
Mike Kelly

Re: FreeBSD Support Patches

[Dmitry Kurochkin]

Mike Kelly <pioto@pioto.org> writes:

> On Fri, May 25, 2012 at 10:15 AM, Dmitry Kurochkin
> <dmitry.kurochkin@gmail.com> wrote:
>> Why do we need to explicitly declare Emacs dependency for tests?  There
>> should be no need for it.  We have "implicit" dependencies which are
>> declared once (see test_declare_external_prereq calls at the end of
>> test-lib.sh) and are automatically handled when a test tries to use a
>> missing binary.  Explicit dependencies are hard to maintain (e.g. your
>> patch adds explicit emacs dependency for crypto test but misses gpg).
>> With rare exceptions we should not use explicit dependencies.
>
> Because not every test actually has those implicit dependencies. For
> example, some of the crypto tests depend upon emacs_deliver_message
> working correctly for subsequents tests. Those emacs_deliver_message
> tests are skipped, but not the ones after it that try to do something
> with that injected message.
>
> For the emacs-* test files, there are some tests that act the same
> way.

These subtests do not directly depend on Emacs.  They depend on other
subtests.  Currently, we do not support such dependencies.  But what you
propose is not the solution.  We have two options here: make all
subtests independent or introduce proper subtests dependencies.  The
former might require many changes to existing tests and may be hard to
enforce.  The latter is not trivial as well but is doable.  I planned to
implement subtests dependencies but never really got to it (and I do not
think I will anytime soon).

> However, it is also a minor speed improvement to say that,
> obviously, none of the emacs tests are going to work, so just don't
> bother.
>

I do not think maintaining an explicit list of dependencies worth a
minor speed improvement.

Given all above, I understand that your patches fix a common problem in
a simple way.  And it does not look like we would get proper solution
anytime soon.  So I am ok with these patches with two comments:

  * Provide a proper commit message to explain the issue in more detail.

  * Add an XXX comment for each explicit dependency, something like:

    // XXX: Workaround for subtests that depend on other subtests (and,
    // hence, indirectly depend on emacs).  Should be removed when we
    // have proper subtests dependencies.

Regards,
  Dmitry

> -- 
> Mike Kelly

Re: FreeBSD Support Patches

[Tomi Ollila]

On Fri, May 25 2012, Mike Kelly <pioto@pioto.org> wrote:

> Hi,
>
> These patches add some changes necessary for a clean build on FreeBSD,
> and for most of the tests to pass.
>
> Also mixed in are a few patches to disable emacs tests when you don't
> have emacs, along with tests that depend upon emacs. I could split those
> off onto a separate branch if required.

Yes...

Thank you for your efforts to make notmuch better; some general
comments on this patch series:


Commit messages are somewhat terse
check what http://notmuchmail.org/patchformatting/
has to say about this.

MANPATH setting has hit me too -- but that shold be set
in the same place where PATH is adjusted to find notmuch
command instead of hardcoding '../../man'.

s/executable/+111/ (and following sed (which could be combined
into next sed but that's another matter)) is good (that hit me
once too) -- This particular patch has the worst commit message,
though (Make test/basic more portable) ;/

in 6/10 there is missing space before sed in '...wc -l |sed...'.

-p overrides -n in perl, so perl -pe... (-npe confuses, at least
me...)

The patch series is supposed to add FreeBSD support; 
prereq additions to take care missing emacs should
be in separate series.

Some of the patches could be marked as 'trivial', putting those
to 'maybe ready' section in http://nmbug.tethera.net/status/
-- just if those had more comprehensive commit messages.

> You can find the latest version of these patches on my github repo's
> fbsd-support branch:
>
>   https://github.com/pioto/notmuch/compare/master...fbsd-support 

Tomi