Bug 41448

-Y´tests: add --enable-install-tests¡

Did you test what happens if $TEST_PYTHON is false (i.e. if python or twisted or similar are missing)?

+    [Enable installation of twisted tests]),

Only twisted ones?
The existing ones use a lower care first letter. I would keep the string just as ´install tests¡ or ´make tests installable¡.

+  AC_DEFINE(ENABLE_TESTS_INSTALL, [],

Why not ENABLE_INSTALL_TESTS like the option name?

+      [Installation of test files])

The documentation string is not very clear

+twistedcapstestsdir = $(GABBLE_TESTS_PATH)/twisted/caps

Are you sure there is no way to avoid having an entry per subdirectory? :(

+BUILT_SOURCES += $(service_files) $(conf_files) tools/exec-with-log.sh run-test.sh

And run-gabble.sh?

+EXTRA_DIST = \
+   $(TWISTED_TESTS) \
+   $(TWISTED_OTHER_FILES) \

How was it working before without this?

+script_fullname=`readlink -e "@abs_top_tests_installdir@/twisted/run-test.sh"`
+if [ `readlink -e "$0"` != "$script_fullname" ] ; then
+  echo "This script is meant to be installed"
+  exit 1
+fi

Hm?

+    /usr/bin/python @abs_top_tests_installdir@/twisted/$i

You should not rely on the python executable to be /usr/bin/python. I think that AM_PATH_PYTHON provides some magic to detect the python executable.

+Exec=@abs_top_tests_installdir@/twisted/tools/run-gabble.sh

I think it's a bit weird that you changed the purpose of the .service.in file in tools/ and moved the old one to tools/servicedir-uninstalled.
Why not keeping the old files as they were and having another dir for the installed ones? Or maybe having an uninstalled/ and an installed/ directories.

+export GABBLE_PLUGIN_DIR="usr/lib/telepathy/gabble-0"

It's missing a ´/¡ at the beginning, but anyway you should not rely on stuff being in /usr.

+  <!-- This is included last so local configuration can override what's 
+       in this standard file -->
+  
+
+  
+
+</busconfig>

Too many empty lines :P


´tests/twisted/Makefile.am: fix make dist¡
´fix confusion between both dbus config files¡

I think you should squash these patches with the previous one.

(In reply to comment #1)
> +  AC_DEFINE(ENABLE_TESTS_INSTALL, [],
> 
> Why not ENABLE_INSTALL_TESTS like the option name?

--enable-installed-tests, ENABLE_INSTALLED_TESTS would be better grammar, and match what I did in dbus.

> +twistedcapstestsdir = $(GABBLE_TESTS_PATH)/twisted/caps
> 
> Are you sure there is no way to avoid having an entry per subdirectory? :(

The thing to search for is "nobase" in Automake documentation. If nobase_dist_twisted_DATA contains "caps/foo.py" then it will be installed to $(twisteddir)/caps/foo.py.

> +EXTRA_DIST = \
> +   $(TWISTED_TESTS) \
> +   $(TWISTED_OTHER_FILES) \
> 
> How was it working before without this?

A guess: if you have things in a variable whose name has the dist_ prefix, they're always distributed, without needing to be in EXTRA_DIST. I personally prefer appropriate use of dist_ prefixes, which means EXTRA_DIST basically means "distributed but not installed".

> You should not rely on the python executable to be /usr/bin/python. I think
> that AM_PATH_PYTHON provides some magic to detect the python executable.

AM_PATH_PYTHON sets $(PYTHON). In Gabble only, $(TEST_PYTHON) is also relevant.

(In reply to comment #2)
> > You should not rely on the python executable to be /usr/bin/python. I think
> > that AM_PATH_PYTHON provides some magic to detect the python executable.
> 
> AM_PATH_PYTHON sets $(PYTHON). In Gabble only, $(TEST_PYTHON) is also relevant.

AM_PATH_PYTHON's killer feature is that if you know better than the build system (e.g. you're in Scratchbox, so /usr/bin/python looks like Python 2.5 but if you try running it it magically becomes Python 2.3), you can override it with "./configure PYTHON=/opt/really/python" or whatever.

(In reply to comment #1)
> tests: add --enable-install-tests¡
> 
> Did you test what happens if $TEST_PYTHON is false (i.e. if python or twisted
> or similar are missing)?

"make check" tests only the C tests, but the python tests are still installed when requested.

> +    [Enable installation of twisted tests]),
> 
> Only twisted ones?

C tests added.

> The existing ones use a lower care first letter. I would keep the string just
> as ´install tests¡ or ´make tests installable¡.

Done

> +  AC_DEFINE(ENABLE_TESTS_INSTALL, [],
> 
> Why not ENABLE_INSTALL_TESTS like the option name?

Changed to ENABLE_INSTALLED_TESTS

> +      [Installation of test files])
> 
> The documentation string is not very clear

Changed

> +twistedcapstestsdir = $(GABBLE_TESTS_PATH)/twisted/caps
> 
> Are you sure there is no way to avoid having an entry per subdirectory? :(

Changed to nobase

> +BUILT_SOURCES += $(service_files) $(conf_files) tools/exec-with-log.sh
> run-test.sh
> 
> And run-gabble.sh?

Added

> +EXTRA_DIST = \
> +   $(TWISTED_TESTS) \
> +   $(TWISTED_OTHER_FILES) \
> 
> How was it working before without this?

TWISTED_TESTS was already in EXTRA_DIST. TWISTED_OTHER_FILES was not but files
were listed individually in EXTRA_DIST.

> +script_fullname=`readlink -e "@abs_top_tests_installdir@/twisted/run-test.sh"`
> +if [ `readlink -e "$0"` != "$script_fullname" ] ; then
> +  echo "This script is meant to be installed"
> +  exit 1
> +fi
> 
> Hm?

If run-test.sh is run from the source tree, it exits with an error.

> +Exec=@abs_top_tests_installdir@/twisted/tools/run-gabble.sh
> 
> I think it's a bit weird that you changed the purpose of the .service.in file
> in tools/ and moved the old one to tools/servicedir-uninstalled.
> Why not keeping the old files as they were and having another dir for the
> installed ones? Or maybe having an uninstalled/ and an installed/ directories.

I think it worth having 2 subdirectories:
- servicedir/
- servicedir-uninstalled/
Updated.

> +export GABBLE_PLUGIN_DIR="usr/lib/telepathy/gabble-0"
> 
> It's missing a ´/¡ at the beginning, but anyway you should not rely on stuff
> being in /usr.

Fixed

> +  <!-- This is included last so local configuration can override what's 
> +       in this standard file -->
> +  
> +
> +  
> +
> +</busconfig>
> 
> Too many empty lines :P

Fixed

> ´tests/twisted/Makefile.am: fix make dist¡
> ´fix confusion between both dbus config files¡
> 
> I think you should squash these patches with the previous one.

Yes.

(In reply to comment #2)
> (In reply to comment #1)
> > +  AC_DEFINE(ENABLE_TESTS_INSTALL, [],
> > 
> > Why not ENABLE_INSTALL_TESTS like the option name?
> 
> --enable-installed-tests, ENABLE_INSTALLED_TESTS would be better grammar, and
> match what I did in dbus.

Done

> > +twistedcapstestsdir = $(GABBLE_TESTS_PATH)/twisted/caps
> > 
> > Are you sure there is no way to avoid having an entry per subdirectory? :(
> 
> The thing to search for is "nobase" in Automake documentation. If
> nobase_dist_twisted_DATA contains "caps/foo.py" then it will be installed to
> $(twisteddir)/caps/foo.py.

Done

> > You should not rely on the python executable to be /usr/bin/python. I think
> > that AM_PATH_PYTHON provides some magic to detect the python executable.
> 
> AM_PATH_PYTHON sets $(PYTHON). In Gabble only, $(TEST_PYTHON) is also relevant.

Really? A few scripts start with the shebang #!/usr/bin/python, e.g.
tools/glib-ginterface-gen.py

(In reply to comment #4)
> > AM_PATH_PYTHON sets $(PYTHON). In Gabble only, $(TEST_PYTHON) is also relevant.
> 
> Really? A few scripts start with the shebang #!/usr/bin/python, e.g.
> tools/glib-ginterface-gen.py

If they're run like "$(PYTHON) $(top_srcdir)/tools/glib-ginterface-gen.py" by the Makefile.am, then it doesn't matter what the shebang says, and they needn't be executable (although it's useful to have the shebang and +x as documentation that the file is intended to be run directly, and as a hint to syntax-highlighting text editors).

If they're run like "$(top_srcdir)/tools/glib-ginterface-gen.py" by the Makefile.am, then that's a bug.

+++ b/tests/twisted/tools/run-gabble.sh.in
@@ -2,7 +2,7 @@
 
 export G_DEBUG=fatal-warnings,fatal-criticals
 export GABBLE_TIMING=1
-export GABBLE_PLUGIN_DIR="@libdir@/telepathy/gabble-0"
+export GABBLE_PLUGIN_DIR="@abs_top_tests_installdir@/plugins:@libdir@/telepathy/gabble-0"

Are you using the same run-gabble.sh for both installed and uninstalled tests?
This is a bit weird, it could load the wrong plugin.
Also, I think it's @libexecdir@ and not @libdir@ (but I'm not 100% sure).

+non_installable_plugins = \
+	test.la
+

Another name could be better considering that sometimes you install it :)

 noinst_LTLIBRARIES = \
-	test.la
+	$(NULL)
+

Why leaving there an empty list? Just don't define noinst_LTLIBRARIES.

+export GABBLE_PLUGIN_DIR="@libdir@/telepathy/gabble-0"

@libexecdir@?

--- a/src/plugin-loader.c
+++ b/src/plugin-loader.c

Is it neded if you use a single run-gabble.sh?

(In reply to comment #6)
> +export GABBLE_PLUGIN_DIR="@libdir@/telepathy/gabble-0"
> 
> @libexecdir@?

No. plugins go in lib, the executable goes in libexec.

(In reply to comment #6)
> +++ b/tests/twisted/tools/run-gabble.sh.in
> @@ -2,7 +2,7 @@
> 
>  export G_DEBUG=fatal-warnings,fatal-criticals
>  export GABBLE_TIMING=1
> -export GABBLE_PLUGIN_DIR="@libdir@/telepathy/gabble-0"
> +export
> GABBLE_PLUGIN_DIR="@abs_top_tests_installdir@/plugins:@libdir@/telepathy/gabble-0"
> 
> Are you using the same run-gabble.sh for both installed and uninstalled tests?

No, run-gabble.sh is only used for the installed tests. The uninstalled tests still use exec-with-log.sh.

> This is a bit weird, it could load the wrong plugin.

GABBLE_PLUGIN_DIR for the installed tests needs 2 paths:
- @abs_top_tests_installdir@/plugins: for the "test" plugin (this plugin is not installed in regular installations)
- @libdir@/telepathy/gabble-0: for the "gateway" plugin

> Also, I think it's @libexecdir@ and not @libdir@ (but I'm not 100% sure).

What Jonny said :)

> +non_installable_plugins = \
> +    test.la
> +
> 
> Another name could be better considering that sometimes you install it :)

Right. "test_only_plugins"?

>  noinst_LTLIBRARIES = \
> -    test.la
> +    $(NULL)
> +
> 
> Why leaving there an empty list? Just don't define noinst_LTLIBRARIES.

If it is defined in one of the "if" branches, it must be defined in the other "if" branch. Otherwise, autoconf complains when something is added with the "+=" operator.

> +export GABBLE_PLUGIN_DIR="@libdir@/telepathy/gabble-0"
> 
> @libexecdir@?

What Jonny said :)

> --- a/src/plugin-loader.c
> +++ b/src/plugin-loader.c
> 
> Is it needed if you use a single run-gabble.sh?

Yes because I specified 2 directories when running the installed tests. run-gabble.sh is only used for the installed tests.

(In reply to comment #8)
> No, run-gabble.sh is only used for the installed tests. The uninstalled tests
> still use exec-with-log.sh.

Oh, I thought it replace exect-with-logs.sh. It's a bit confusing having 2 different scripts with completely different names for similar tasks in different cases (but I don't have any better suggestion now).

> > Another name could be better considering that sometimes you install it :)
> 
> Right. "test_only_plugins"?

Sounds good.


+  dir_vector = g_strsplit (directories_name, ":", 0);

Vector? It's not C++ :P


For the rest the patches are fine for me (if Simon doesn't have any other comment).

I pushed the last 2 fixes.

> +GABBLE_TESTS_PATH=${datarootdir}/telepathy-gabble-tests
...
> +if ENABLE_INSTALLED_TESTS
> +gabbletestsdir = $(GABBLE_TESTS_PATH)
> +gabbletests_PROGRAMS = $(tests_list)
> +gabbletests_DATA = gabble-C-tests.list

Machine-specific binaries in /usr/share/telepathy-gabble-tests? No thanks.

I'd personally just move all the tests to ${libdir}/telepathy-gabble-tests (or ${libdir}/telepathy/gabble-tests or something), but you could split the architecture-independent bits into /usr/share if you want to go that far.

If you rename GABBLE_TESTS_PATH to gabbletestsdir throughout, and change "gabbletestsdir = $(GABBLE_TESTS_PATH)" to "gabbletestsdir = @gabbletestsdir@" (or you might even be able to delete it entirely - automake might put that line in automatically), you'll have one less redundant variable, => one less source of confusion.

> tests: add --enable-install-tests

You renamed it, amend the commit message please.

> + test-debug.py \
> + test-fallback-socks5-proxy.py \
> + test-register.py \
> + test-location.py \
> + pubsub.py \
> + sidecar-own-caps.py \
> + sidecars.py \
> + mail-notification.py \
> + client-types.py \
> + gateways.py \
> + last-activity.py \
> + pep-support.py \
> + plugin-channel-managers.py \
> + power-save.py \
> + version.py \
> + dataforms.py \

If you're going to permute the order of things in a big list, please sort the new positions alphabetically while you're changing it anyway. That could be a separate, first, commit.

> -TWISTED_JINGLE_TESTS = \

You've removed the (documented!) ability to "make check TWISTED_JINGLE_TESTS=" when you know you haven't touched Jingle-specific code, so that it doesn't take 6 months to run the tests and then fail with an obscure race condition. (Or perhaps we've fixed the Jingle tests enough by now that only the first reason applies? Even so, it's a useful feature.)

> + $(AM_V_GEN)sed -e "s|[@]abs_top_tests_installdir[@]|$(GABBLE_TESTS_PATH)|g" \
> + -e "s|[@]python[@]|$(PYTHON)|g" \

abs_top_tests_installdir is a bit misleading: it looks like a standard autoconf substitution, but it isn't. I'd use @GABBLE_TESTS_PATH@, or even @gabbletestsdir@.

It's conventional to replace @Thing@ with $(Thing) (in any case combination), so the placeholder for Python should be @PYTHON@ instead of @python@.

> + @mkdir -p tools

Not portable. Use $(MKDIR_P) instead (and make sure we have AC_PROG_MKDIR_P in the configure.ac, but I think we do already).

> +if [ `readlink -e "$0"` != "$script_fullname" ] ; then
> + echo "This script is meant to be installed"

You could at least say "... installed at $script_fullname".

I'm not sure whether this use of readlink is portable away from Linux, but I'm not sure that I care either.

> +export PYTHONPATH=@abs_top_tests_installdir@/twisted

Not portable to all POSIX shells, use

    PYTHONPATH=...
    export PYTHONPATH

(and the same in all #!/bin/sh scripts).

> # because test.la is not installed, libtool will want to compile it as static
> # despite -shared (a convenience library), unless we also use -rpath
> -test_la_LDFLAGS = $(AM_LDFLAGS) -rpath $(plugindir)
> +test_la_LDFLAGS = $(AM_LDFLAGS) -rpath $(testplugindir)

This won't work in the not-installing-tests case, because testplugindir won't be defined.

I'd just move "test_la_LDFLAGS = $(AM_LDFLAGS) -rpath $(plugindir)" inside the "not installing tests" branch of the conditional. If test.la is going to be installed in $(testplugindir), it gets that as an rpath automatically, so you don't need special workarounds to force it to be a shared library.

(In reply to comment #11)
> I'd personally just move all the tests to ${libdir}/telepathy-gabble-tests (or
> ${libdir}/telepathy/gabble-tests or something), but you could split the
> architecture-independent bits into /usr/share if you want to go that far.

Note that under Red-Hat-style biarch and Debian-style multiarch, nothing that encodes an architecture-specific path (like ${libdir}, which will typically be something like /usr/lib64 on a biarch system or /usr/lib/x86_64-linux-gnu on a multiarch system) can go in /usr/share either.

Thanks for the reviews.

(In reply to comment #11)
> > +GABBLE_TESTS_PATH=${datarootdir}/telepathy-gabble-tests
> ...
> > +if ENABLE_INSTALLED_TESTS
> > +gabbletestsdir = $(GABBLE_TESTS_PATH)
> > +gabbletests_PROGRAMS = $(tests_list)
> > +gabbletests_DATA = gabble-C-tests.list
> 
> Machine-specific binaries in /usr/share/telepathy-gabble-tests? No thanks.
> 
> I'd personally just move all the tests to ${libdir}/telepathy-gabble-tests (or
> ${libdir}/telepathy/gabble-tests or something), but you could split the
> architecture-independent bits into /usr/share if you want to go that far.

Moved to ${libdir}/telepathy-gabble-tests.

> If you rename GABBLE_TESTS_PATH to gabbletestsdir throughout, and change
> "gabbletestsdir = $(GABBLE_TESTS_PATH)" to "gabbletestsdir = @gabbletestsdir@"
> (or you might even be able to delete it entirely - automake might put that line
> in automatically), you'll have one less redundant variable, => one less source
> of confusion.

Renamed to gabbletestsdir.

> > tests: add --enable-install-tests
> 
> You renamed it, amend the commit message please.

Done

> If you're going to permute the order of things in a big list, please sort the
> new positions alphabetically while you're changing it anyway. That could be a
> separate, first, commit.

Lists moved around in 2 separate commits.

> > -TWISTED_JINGLE_TESTS = \
> 
> You've removed the (documented!) ability to "make check TWISTED_JINGLE_TESTS="
> when you know you haven't touched Jingle-specific code, so that it doesn't take
> 6 months to run the tests and then fail with an obscure race condition. (Or
> perhaps we've fixed the Jingle tests enough by now that only the first reason
> applies? Even so, it's a useful feature.)

I reverted the 4 specialized lists.

> > + $(AM_V_GEN)sed -e "s|[@]abs_top_tests_installdir[@]|$(GABBLE_TESTS_PATH)|g" \
> > + -e "s|[@]python[@]|$(PYTHON)|g" \
> 
> abs_top_tests_installdir is a bit misleading: it looks like a standard autoconf
> substitution, but it isn't. I'd use @GABBLE_TESTS_PATH@, or even
> @gabbletestsdir@.

Done: it will be @gabbletestsdir@.

> It's conventional to replace @Thing@ with $(Thing) (in any case combination),
> so the placeholder for Python should be @PYTHON@ instead of @python@.

Done.

> > + @mkdir -p tools
> 
> Not portable. Use $(MKDIR_P) instead (and make sure we have AC_PROG_MKDIR_P in
> the configure.ac, but I think we do already).

Done. I added a first separate commit to add AC_PROG_MKDIR_P and to MKDIR_P-ify the Makefiles.

> > +if [ `readlink -e "$0"` != "$script_fullname" ] ; then
> > + echo "This script is meant to be installed"
> 
> You could at least say "... installed at $script_fullname".

Done.

> I'm not sure whether this use of readlink is portable away from Linux, but I'm
> not sure that I care either.

Ok, I keep readlink then.

> > +export PYTHONPATH=@abs_top_tests_installdir@/twisted
> 
> Not portable to all POSIX shells, use
> 
>     PYTHONPATH=...
>     export PYTHONPATH

Done

> (and the same in all #!/bin/sh scripts).

Done in a separate commit.

(In reply to comment #12)
> > # because test.la is not installed, libtool will want to compile it as static
> > # despite -shared (a convenience library), unless we also use -rpath
> > -test_la_LDFLAGS = $(AM_LDFLAGS) -rpath $(plugindir)
> > +test_la_LDFLAGS = $(AM_LDFLAGS) -rpath $(testplugindir)
> 
> This won't work in the not-installing-tests case, because testplugindir won't
> be defined.
> 
> I'd just move "test_la_LDFLAGS = $(AM_LDFLAGS) -rpath $(plugindir)" inside the
> "not installing tests" branch of the conditional. If test.la is going to be
> installed in $(testplugindir), it gets that as an rpath automatically, so you
> don't need special workarounds to force it to be a shared library.

Done. I keep a dummy test_la_LDFLAGS in the other branch of the if, otherwise autoconf complains.

It seems a bit unfortunate that a separate dbus-daemon and Gabble are spawned for every individual installed test one runs … but oh well!

I assume Gabble with this branch passes `make distcheck`?

Here are some nitpicks:

+        Test files installation.....:  ${installed_tests}

“Install unit tests”

+gabble-C-tests.list:
+	echo $(tests_list) > $@

If you use this:

  $(AM_V_GEN)echo $(tests_list) > $@

then it will show up as

  GEN     gabble-C-tests.list

in the output of make V=0.

-  const gchar *directory_name = g_getenv ("GABBLE_PLUGIN_DIR");
+  const gchar *directories_name = g_getenv ("GABBLE_PLUGIN_DIR");

'directory_names' would be better English.

(In reply to comment #15)
> It seems a bit unfortunate that a separate dbus-daemon and Gabble are spawned
> for every individual installed test one runs … but oh well!
> 
> I assume Gabble with this branch passes `make distcheck`?

Hum... It didn't but I fixed it now!

> Here are some nitpicks:
> 
> +        Test files installation.....:  ${installed_tests}
> 
> “Install unit tests”

fixed

> +gabble-C-tests.list:
> +    echo $(tests_list) > $@
> 
> If you use this:
> 
>   $(AM_V_GEN)echo $(tests_list) > $@
> 
> then it will show up as
> 
>   GEN     gabble-C-tests.list
> 
> in the output of make V=0.

fixed

> -  const gchar *directory_name = g_getenv ("GABBLE_PLUGIN_DIR");
> +  const gchar *directories_name = g_getenv ("GABBLE_PLUGIN_DIR");
> 
> 'directory_names' would be better English.

fixed

and merged to git master: d92500b..f9529b7