After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 701099 - folks looks for zeitgeist.h when configured with --disable-zeitgeist --enable-telepathy-backend and --disable-vala
folks looks for zeitgeist.h when configured with --disable-zeitgeist --enable...
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: Telepathy backend
git master
Other Linux
: Normal normal
: Unset
Assigned To: folks-maint
folks-maint
: 703221 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-05-27 20:34 UTC by Pacho Ramos
Modified: 2013-07-08 17:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tests: Fix whitespace in Makefile (1.39 KB, patch)
2013-06-20 16:34 UTC, Philip Withnall
committed Details | Review
build: Update git.mk from upstream (10.55 KB, patch)
2013-06-20 16:34 UTC, Philip Withnall
committed Details | Review
build: Modify git.mk to ignore generated C/H files and Vala .stamp files (842 bytes, patch)
2013-06-20 16:34 UTC, Philip Withnall
committed Details | Review
build: Remove redundant entries from GITIGNOREFILES (7.46 KB, patch)
2013-06-20 16:34 UTC, Philip Withnall
committed Details | Review
build: Require valac when building from a tarball (11.06 KB, patch)
2013-06-20 16:34 UTC, Philip Withnall
none Details | Review
build: Delete generated C/H/vapi/stamp files from tarballs (13.16 KB, patch)
2013-06-20 16:34 UTC, Philip Withnall
none Details | Review
build: Use target-specific VALAFLAGS rather than AM_VALAFLAGS (15.24 KB, patch)
2013-06-20 16:34 UTC, Philip Withnall
none Details | Review
build: Remove generated C/H/vapi/stamp files from MAINTAINERCLEANFILES (13.09 KB, patch)
2013-06-20 16:34 UTC, Philip Withnall
none Details | Review
build: Don’t build Vala headers in a subdirectory unnecessarily (2.92 KB, patch)
2013-06-20 16:34 UTC, Philip Withnall
none Details | Review
build: Reorder DIST_SUBDIRS to match SUBDIRS (772 bytes, patch)
2013-06-20 16:34 UTC, Philip Withnall
none Details | Review
build: Makefile whitespace fixes (1.22 KB, patch)
2013-06-20 16:35 UTC, Philip Withnall
none Details | Review
build: Fix automake dependency between tests/lib/telepathy and a subdir (1.60 KB, patch)
2013-06-20 16:35 UTC, Philip Withnall
none Details | Review
build.log (128.90 KB, text/plain)
2013-06-25 17:42 UTC, Pacho Ramos
  Details
build: Use target-specific VALAFLAGS rather than AM_VALAFLAGS (15.23 KB, patch)
2013-07-04 20:45 UTC, Philip Withnall
none Details | Review
build: Don’t build Vala headers in a subdirectory unnecessarily (4.76 KB, patch)
2013-07-04 20:46 UTC, Philip Withnall
none Details | Review
build: Reorder DIST_SUBDIRS to match SUBDIRS (772 bytes, patch)
2013-07-04 20:46 UTC, Philip Withnall
committed Details | Review
build: Makefile whitespace fixes (1.22 KB, patch)
2013-07-04 20:46 UTC, Philip Withnall
committed Details | Review
build: Use target-specific VALAFLAGS rather than AM_VALAFLAGS (2) (15.24 KB, patch)
2013-07-04 21:34 UTC, Travis Reitter
none Details | Review
build: Don’t build Vala headers in a subdirectory unnecessarily (2) (5.95 KB, patch)
2013-07-04 21:52 UTC, Travis Reitter
committed Details | Review
telepathy: Move Zeitgeist code entirely into Tpf.PersonaStore (6.72 KB, patch)
2013-07-05 14:30 UTC, Philip Withnall
committed Details | Review
telepathy: Fix build with --disable-zeitgeist (27.88 KB, patch)
2013-07-05 14:30 UTC, Philip Withnall
committed Details | Review
build: Use target-specific VALAFLAGS rather than AM_VALAFLAGS (3) (15.46 KB, patch)
2013-07-05 18:49 UTC, Travis Reitter
committed Details | Review
Don't always build tp-zeitgeist.vapi (1.49 KB, patch)
2013-07-08 16:16 UTC, Travis Reitter
committed Details | Review

Description Pacho Ramos 2013-05-27 20:34:25 UTC
But it compiles ok when vala support is enabled (!):
libtool: link: ( cd ".libs" && rm -f "libtp-lowlevel.la" && ln -s "../libtp-lowlevel.la" "libtp-lowlevel.la" )
/usr/bin/g-ir-scanner   --add-include-path=. --warn-all  --namespace=TpLowlevel --nsversion=lowlevel --libtool="/bin/sh ../../../libtool"  --include=GObject-2.0 --include=TelepathyGLib-0.12   --library=libtp-lowlevel.la --identifier-prefix=FolksTpLowlevel --cflags-begin -pthread -I/usr/include/telepathy-1.0 -I/usr/include/dbus-1.0 -I/usr/lib64/dbus-1.0/include -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include  -DTP_VERSION_MIN_REQUIRED=TP_VERSION_0_20 -DTP_VERSION_MAX_ALLOWED=TP_VERSION_0_20 --cflags-end  tp-lowlevel.c tp-lowlevel.h libtp-lowlevel.la --output tp-lowlevel.gir
tpf-persona-store.c:34:23: fatal error: zeitgeist.h: No such file or directory
compilation terminated.
make[4]: *** [libfolks_telepathy_la-tpf-persona-store.lo] Error 1
make[4]: *** Waiting for unfinished jobs....
tpf-persona.c:34:23: fatal error: zeitgeist.h: No such file or directory
compilation terminated.
make[4]: *** [libfolks_telepathy_la-tpf-persona.lo] Error 1
g-ir-scanner: compile: gcc -Wall -Wno-deprecated-declarations -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -march=native -pipe -O2 -ggdb -I/usr/include/gio-unix-2.0/ -I/usr/include/telepathy-1.0 -I/usr/include/dbus-1.0 -I/usr/lib64/dbus-1.0/include -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -c -o /var/tmp/portage/dev-libs/folks-0.9.1/work/folks-0.9.1/backends/telepathy/lib/tmp-introspectanKAdj/TpLowlevel-lowlevel.o /var/tmp/portage/dev-libs/folks-0.9.1/work/folks-0.9.1/backends/telepathy/lib/tmp-introspectanKAdj/TpLowlevel-lowlevel.c
g-ir-scanner: link: /bin/sh ../../../libtool --mode=link --tag=CC gcc -o /var/tmp/portage/dev-libs/folks-0.9.1/work/folks-0.9.1/backends/telepathy/lib/tmp-introspectanKAdj/TpLowlevel-lowlevel -export-dynamic -march=native -pipe -O2 -ggdb -Wl,-O1 -Wl,--as-needed -Wl,--hash-style=gnu /var/tmp/portage/dev-libs/folks-0.9.1/work/folks-0.9.1/backends/telepathy/lib/tmp-introspectanKAdj/TpLowlevel-lowlevel.o -L. libtp-lowlevel.la -lgio-2.0 -lgobject-2.0 -Wl,--export-dynamic -lgmodule-2.0 -pthread -lrt -lglib-2.0
libtool: link: gcc -o /var/tmp/portage/dev-libs/folks-0.9.1/work/folks-0.9.1/backends/telepathy/lib/tmp-introspectanKAdj/.libs/TpLowlevel-lowlevel -march=native -pipe -O2 -ggdb -Wl,-O1 -Wl,--hash-style=gnu /var/tmp/portage/dev-libs/folks-0.9.1/work/folks-0.9.1/backends/telepathy/lib/tmp-introspectanKAdj/TpLowlevel-lowlevel.o -Wl,--export-dynamic -pthread -Wl,--export-dynamic  -Wl,--as-needed -L. ./.libs/libtp-lowlevel.so -ltelepathy-glib -lgio-2.0 -lgobject-2.0 -lgmodule-2.0 -lrt -lglib-2.0 -pthread
make[4]: Leaving directory `/var/tmp/portage/dev-libs/folks-0.9.1/work/folks-0.9.1/backends/telepathy/lib'
make[3]: *** [all-recursive] Error 1
make[3]: Leaving directory `/var/tmp/portage/dev-libs/folks-0.9.1/work/folks-0.9.1/backends/telepathy'
make[2]: *** [all-recursive] Error 1
make[2]: Leaving directory `/var/tmp/portage/dev-libs/folks-0.9.1/work/folks-0.9.1/backends'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/var/tmp/portage/dev-libs/folks-0.9.1/work/folks-0.9.1'
make: *** [all] Error 2
Comment 1 Pacho Ramos 2013-06-09 10:57:08 UTC
Still the same with 0.9.2 :(
Comment 2 Philip Withnall 2013-06-16 21:47:22 UTC
This is because the C files used with --disable-vala are generated by a compilation run with --enable-zeitgeist, because the “#if HAVE_ZEITGEIST” blocks in tpf-persona-store.vala are processed by valac, not the C compiler.

As far as I can see, this is a fundamental limitation in the way we’ve made Zeitgeist support optional.

I’d be tempted to fix this by making the --[enable|disable]-vala option go away, and always requiring valac to be present when building folks, now that Vala is more mature and widely distributed. Travis, what do you think?
Comment 3 Travis Reitter 2013-06-18 18:56:52 UTC
I've been fine with valac being a hard build requirement for a while, so please do so. I'd like to make it a requirement for releases as well and not ship the .[ch] files but the Vala automake macros make it impossibly to /not/ ship them. That would also be nice to fix (but extremely low on my list).
Comment 4 Philip Withnall 2013-06-20 16:34:16 UTC
Created attachment 247344 [details] [review]
tests: Fix whitespace in Makefile
Comment 5 Philip Withnall 2013-06-20 16:34:20 UTC
Created attachment 247345 [details] [review]
build: Update git.mk from upstream

Upstream is now located at https://github.com/behdad/git.mk. The file hasn’t
been modified from upstream.
Comment 6 Philip Withnall 2013-06-20 16:34:24 UTC
Created attachment 247346 [details] [review]
build: Modify git.mk to ignore generated C/H files and Vala .stamp files

These modifications are being submitted upstream.
Comment 7 Philip Withnall 2013-06-20 16:34:27 UTC
Created attachment 247347 [details] [review]
build: Remove redundant entries from GITIGNOREFILES

Now that git.mk has been modified to add better Vala support, these
are unnecessary.
Comment 8 Philip Withnall 2013-06-20 16:34:40 UTC
Created attachment 247348 [details] [review]
build: Require valac when building from a tarball

Vala is a lot more widespread than when folks was originally released, and
generating all C/H files for the tarball means we can’t do conditional
compilation (since ifdefs are not passed through from Vala to C), causing
bugs such as bug #701099.

This removes the --[enable|disable]-vala configure option and propagates
changes to the Makefiles. Subsequent commits tidy things up and fix build
breakages.

Closes: https://bugzilla.gnome.org/show_bug.cgi?id=701099#c2
Comment 9 Philip Withnall 2013-06-20 16:34:43 UTC
Created attachment 247349 [details] [review]
build: Delete generated C/H/vapi/stamp files from tarballs

This forces Vala code to be recompiled when building from tarballs.

See: https://bugzilla.gnome.org/show_bug.cgi?id=701099#c2
Comment 10 Philip Withnall 2013-06-20 16:34:47 UTC
Created attachment 247350 [details] [review]
build: Use target-specific VALAFLAGS rather than AM_VALAFLAGS

This allows automake to parse the flags for each target and generate
appropriate rules. For example, by explicitly specifying “--vapi foo.vapi” in
foo_VALAFLAGS (not just specifying “--library foo”), dependency and clean-up
rules for foo.vapi will be generated by automake. Accordingly, this commit
also adds missing --vapi flags.

Helps:
Comment 11 Philip Withnall 2013-06-20 16:34:51 UTC
Created attachment 247351 [details] [review]
build: Remove generated C/H/vapi/stamp files from MAINTAINERCLEANFILES

As long as --vapi and -H/-h options are correctly passed to a target-specific
VALAFLAGS, automake will automatically add these generated files to the
maintainer-clean target.

Helps:
Comment 12 Philip Withnall 2013-06-20 16:34:54 UTC
Created attachment 247352 [details] [review]
build: Don’t build Vala headers in a subdirectory unnecessarily
Comment 13 Philip Withnall 2013-06-20 16:34:58 UTC
Created attachment 247353 [details] [review]
build: Reorder DIST_SUBDIRS to match SUBDIRS

So we don’t encounter problems during `make dist` due to recursing in
a different order from normal.
Comment 14 Philip Withnall 2013-06-20 16:35:02 UTC
Created attachment 247354 [details] [review]
build: Makefile whitespace fixes
Comment 15 Philip Withnall 2013-06-20 16:35:05 UTC
Created attachment 247355 [details] [review]
build: Fix automake dependency between tests/lib/telepathy and a subdir
Comment 16 Philip Withnall 2013-06-20 16:35:57 UTC
That was fun. Branch here: https://www.gitorious.org/folks/folks/commits/701099-kill-non-vala-build
Comment 17 Philip Withnall 2013-06-20 18:22:26 UTC
Bother. On further testing, it fails `make distcheck`. I was testing `make dist` + building from the tarball before, which works fine. I’ll come back to this in the next few days and see what I’ve done wrong.
Comment 18 Pacho Ramos 2013-06-25 17:42:04 UTC
Created attachment 247758 [details]
build.log

Not sure if related with this but, with folks-0.9.3, I get zeitgeist headers to be looked for even with I pass "--enable-vala" :S
Comment 19 Philip Withnall 2013-06-28 16:55:26 UTC
*** Bug 703221 has been marked as a duplicate of this bug. ***
Comment 20 Philip Withnall 2013-06-28 19:55:57 UTC
*** Bug 703221 has been marked as a duplicate of this bug. ***
Comment 21 Chris Vine 2013-06-29 00:05:32 UTC
#comment 4 of bug #703221

Then why not just put a suitable test for zeitgeist in configure.ac/aclocal which puts a define in config.h, and go from there?
Comment 22 Philip Withnall 2013-07-01 18:30:49 UTC
(In reply to comment #21)
> Then why not just put a suitable test for zeitgeist in configure.ac/aclocal
> which puts a define in config.h, and go from there?

Because we can’t get #ifdefs into the C code which is generated by Vala, so having #defines in config.h won’t help. The fix is to stop distributing generated C code, and require people to build from the Vala source themselves (for which the --disable-zeitgeist option works). That’s what my patch set will do, once I find time to finish/fix it.
Comment 23 Travis Reitter 2013-07-04 18:23:51 UTC
Review of attachment 247344 [details] [review]:

I think this has already been applied, but it looks good.
Comment 24 Travis Reitter 2013-07-04 18:24:45 UTC
Review of attachment 247345 [details] [review]:

Looks good.
Comment 25 Travis Reitter 2013-07-04 18:33:29 UTC
Review of attachment 247346 [details] [review]:

Looks good
Comment 26 Travis Reitter 2013-07-04 18:33:33 UTC
Review of attachment 247347 [details] [review]:

Looks good
Comment 27 Philip Withnall 2013-07-04 19:57:57 UTC
Comment on attachment 247344 [details] [review]
tests: Fix whitespace in Makefile

You’re right; I had already committed it. Whoops.
Comment 28 Philip Withnall 2013-07-04 19:58:56 UTC
Comment on attachment 247345 [details] [review]
build: Update git.mk from upstream

Pushed to master.

commit a5ca31d956d9461fd9d399f18921e39a1f3052ff
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Jun 20 16:08:54 2013 +0100

    build: Update git.mk from upstream
    
    Upstream is now located at https://github.com/behdad/git.mk. The file hasn’t
    been modified from upstream.

 git.mk | 175 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------
 1 file changed, 140 insertions(+), 35 deletions(-)
Comment 29 Philip Withnall 2013-07-04 19:59:14 UTC
Comment on attachment 247346 [details] [review]
build: Modify git.mk to ignore generated C/H files and Vala .stamp files

Committed to master.

commit e0052149c672f41a5d367d83bc1ef229a937abda
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Jun 20 16:09:36 2013 +0100

    build: Modify git.mk to ignore generated C/H files and Vala .stamp files
    
    These modifications are being submitted upstream.

 git.mk | 4 ++++
 1 file changed, 4 insertions(+)
Comment 30 Philip Withnall 2013-07-04 19:59:29 UTC
Comment on attachment 247347 [details] [review]
build: Remove redundant entries from GITIGNOREFILES

Committed to master.

commit 268ee7a387eb505f8a4f5c6c0ab6dc036e65e5e1
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Jun 20 16:35:00 2013 +0100

    build: Remove redundant entries from GITIGNOREFILES
    
    Now that git.mk has been modified to add better Vala support, these
    are unnecessary.

 backends/eds/Makefile.am           | 6 ------
 backends/key-file/Makefile.am      | 6 ------
 backends/libsocialweb/Makefile.am  | 6 ------
 backends/ofono/Makefile.am         | 5 -----
 backends/telepathy/Makefile.am     | 6 ------
 backends/tracker/Makefile.am       | 6 ------
 folks/Makefile.am                  | 9 +--------
 tests/lib/Makefile.am              | 5 +++++
 tests/lib/eds/Makefile.am          | 5 -----
 tests/lib/key-file/Makefile.am     | 5 -----
 tests/lib/libsocialweb/Makefile.am | 5 -----
 tests/lib/telepathy/Makefile.am    | 4 ----
 tests/lib/tracker/Makefile.am      | 5 -----
 tools/Makefile.am                  | 5 -----
 tools/inspect/Makefile.am          | 5 -----
 15 files changed, 6 insertions(+), 77 deletions(-)
Comment 31 Philip Withnall 2013-07-04 20:45:56 UTC
Created attachment 248411 [details] [review]
build: Use target-specific VALAFLAGS rather than AM_VALAFLAGS

This allows automake to parse the flags for each target and generate
appropriate rules. For example, by explicitly specifying “--vapi foo.vapi” in
foo_VALAFLAGS (not just specifying “--library foo”), dependency and clean-up
rules for foo.vapi will be generated by automake. Accordingly, this commit
also adds missing --vapi flags.

Helps:
Comment 32 Philip Withnall 2013-07-04 20:46:25 UTC
Created attachment 248412 [details] [review]
build: Don’t build Vala headers in a subdirectory unnecessarily
Comment 33 Philip Withnall 2013-07-04 20:46:29 UTC
Created attachment 248413 [details] [review]
build: Reorder DIST_SUBDIRS to match SUBDIRS

So we don’t encounter problems during `make dist` due to recursing in
a different order from normal.
Comment 34 Philip Withnall 2013-07-04 20:46:33 UTC
Created attachment 248414 [details] [review]
build: Makefile whitespace fixes
Comment 35 Philip Withnall 2013-07-04 20:53:09 UTC
It turns out that automake makes it far too hard for us to stop distributing generated C code (due to some interactions I can’t understand between distcheck and the relative paths for the .stamp files; distcheck ends up noticing the .stamp files in the git checkout, and assuming that means the C files in the source directory have been generated when they actually haven’t…then fails; I had no idea how to investigate further and have already burned enough time on this).

So, the four outstanding patches on this bug are orthogonal build system fixes which should be ready to commit. They don’t fix the --disable-zeitgeist problem. I have an idea for a conditionally-linked shim library which will fix it, but haven’t written the code yet. That’s a job for tomorrow.

Finally, I’ve submitted a bug report upstream against automake to ask for the possibility of automake-supported generation of tarballs which *don’t* contain generated C code. Let’s see how that progresses. I submitted it to a mysterious e-mail address, and sometime it might show up here: http://debbugs.gnu.org/cgi/pkgreport.cgi?package=automake
Comment 36 Travis Reitter 2013-07-04 21:34:40 UTC
Created attachment 248416 [details] [review]
build: Use target-specific VALAFLAGS rather than AM_VALAFLAGS (2)

This fixes a couple build errors I hit after building after a distclean. Diff the old version against mine to see the changes.

=================

make[4]: Entering directory `/home/treitter/collabora/folks/backends/libsocialweb/lib'
...
make[4]: *** No rule to make target `folks/folks-libsocialweb.h', needed by `FolksLibsocialweb-0.6.gir'.  Stop.
make[4]: *** Waiting for unfinished jobs....

================

make[4]: Entering directory `/home/treitter/collabora/folks/tests/lib'
  CC       libfolks_test_la-haze-remove-directory.lo
  CC       libfolks_test_la-test-case-helper.lo
  VALAC    libfolks_test_la_vala.stamp
haze-remove-directory.c:11:33: fatal error: folks-test-internal.h: No such file or directory
compilation terminated.
test-case-helper.c:23:33: fatal error: folks-test-internal.h: No such file or directory
compilation terminated.
Comment 37 Travis Reitter 2013-07-04 21:37:38 UTC
(In reply to comment #36)
> Diff the old version against mine to see the changes.

Apparently bugzilla removes the old patch (at least, I couldn't find it just now), so here's the meta-diff:


diff --git a/backends/libsocialweb/lib/Makefile.am b/backends/libsocialweb/lib/Makefile.am
index 9c3c586..f07cb60 100644
--- a/backends/libsocialweb/lib/Makefile.am
+++ b/backends/libsocialweb/lib/Makefile.am
@@ -50,7 +50,7 @@ libfolks_libsocialweb_la_VALAFLAGS = \
        --includedir folks \
        --library folks-libsocialweb \
        --vapi folks-libsocialweb.vapi \
-       -H folks-libsocialweb.h \
+       -H folks/folks-libsocialweb.h \
        $(NULL)
 
 libfolks_libsocialweb_la_CFLAGS = \
diff --git a/tests/lib/Makefile.am b/tests/lib/Makefile.am
index 77f7da0..def5939 100644
--- a/tests/lib/Makefile.am
+++ b/tests/lib/Makefile.am
@@ -90,6 +90,11 @@ libfolks_test_la_VALAFLAGS = \
        -g \
        $(NULL)
 
+BUILT_SOURCES = \
+       libfolks_test_la_vala.stamp \
+       $(NULL)
+
+
 MAINTAINERCLEANFILES = \
        $(patsubst %.vala,%.c,$(filter %.vala,$(libfolks_test_la_SOURCES))) \
        libfolks_test_la_vala.stamp \
Comment 38 Travis Reitter 2013-07-04 21:52:03 UTC
Created attachment 248418 [details] [review]
build: Don’t build Vala headers in a subdirectory unnecessarily (2)

My version fixes the MAINTAINERCLEANFILES definitions (which still included folks/) and moves the "-H folks-libsocialweb.h" correction from the last patch to this one (where it belongs).

Are you sure "make distcheck" still works after this? I thought that's why I made this change originally.

Unfortunately, I can't currently "make distcheck" myself because all the EDS tests fail for me with:

(/home/treitter/collabora/folks/tests/eds/.libs/lt-im-details:2683): GLib-GObject-CRITICAL **: Attempt to add property ESourceCamelImapx::filter-inbox after class was initialised

No amount of distcleaning or fixing EDS in my development PREFIX have helped.
Comment 39 Travis Reitter 2013-07-04 21:57:02 UTC
Review of attachment 248413 [details] [review]:

Looks good
Comment 40 Travis Reitter 2013-07-04 21:57:08 UTC
Review of attachment 248414 [details] [review]:

Looks good
Comment 41 Philip Withnall 2013-07-05 08:45:22 UTC
(In reply to comment #35)
> I submitted it to a mysterious
> e-mail address, and sometime it might show up here:
> http://debbugs.gnu.org/cgi/pkgreport.cgi?package=automake

It’s now appeared here: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=14796
Comment 42 Philip Withnall 2013-07-05 09:20:54 UTC
(In reply to comment #37)
> (In reply to comment #36)
> > Diff the old version against mine to see the changes.
> 
> Apparently bugzilla removes the old patch (at least, I couldn't find it just
> now), so here's the meta-diff:

These changes don’t look correct. Were you testing the patches independently or something? What automake version are you using? I’m on 1.11.3, but I’ve also tested with 1.12.2.

> diff --git a/backends/libsocialweb/lib/Makefile.am
> b/backends/libsocialweb/lib/Makefile.am
> index 9c3c586..f07cb60 100644
> --- a/backends/libsocialweb/lib/Makefile.am
> +++ b/backends/libsocialweb/lib/Makefile.am
> @@ -50,7 +50,7 @@ libfolks_libsocialweb_la_VALAFLAGS = \
>         --includedir folks \
>         --library folks-libsocialweb \
>         --vapi folks-libsocialweb.vapi \
> -       -H folks-libsocialweb.h \
> +       -H folks/folks-libsocialweb.h \
>         $(NULL)

Why should this one need changing back, but none of the other backends’?

>  libfolks_libsocialweb_la_CFLAGS = \
> diff --git a/tests/lib/Makefile.am b/tests/lib/Makefile.am
> index 77f7da0..def5939 100644
> --- a/tests/lib/Makefile.am
> +++ b/tests/lib/Makefile.am
> @@ -90,6 +90,11 @@ libfolks_test_la_VALAFLAGS = \
>         -g \
>         $(NULL)
> 
> +BUILT_SOURCES = \
> +       libfolks_test_la_vala.stamp \
> +       $(NULL)
> +
> +

That shouldn’t be necessary, as automake magically generates a dependency from haze-remove-directory.c to folks-test-internal.h, and has a recipe to generate the latter using valac.

Take a look at tests/lib/.deps/libfolks_test_la-haze-remove-directory.Plo and see if it contains more than “# dummy”. If not, delete it and force it to be re-created; that should fix the build problem (though I think distcleaning does this).
Comment 43 Philip Withnall 2013-07-05 14:30:39 UTC
Created attachment 248462 [details] [review]
telepathy: Move Zeitgeist code entirely into Tpf.PersonaStore

Ready to split it out into a shim library.

Helps:
Comment 44 Philip Withnall 2013-07-05 14:30:43 UTC
Created attachment 248463 [details] [review]
telepathy: Fix build with --disable-zeitgeist

Vala doesn’t pass #if conditionals through to generated C code, so when
building folks from a tarball, the --[enable|disable]-zeitgeist option was
having no effect, and Zeitgeist was always being pulled into the build.

Fix this by separating the Zeitgeist code out into a small noinst shim
library and always generating C code for two versions of it: one with
Zeitgeist enabled, and one with it disabled. The correct one’s C code is
then compiled and linked into libfolks-telepathy.la when building from the
tarball.

This is a horrible, ugly fix, but the only better fix is to disable
distribution of generated C code. This was attempted, but thwarted by
automake’s Vala support.

Closes:
Comment 45 Travis Reitter 2013-07-05 18:47:14 UTC
(In reply to comment #42)
> (In reply to comment #37)
> > (In reply to comment #36)
> > > Diff the old version against mine to see the changes.
> > 
> > Apparently bugzilla removes the old patch (at least, I couldn't find it just
> > now), so here's the meta-diff:
> 
> These changes don’t look correct. Were you testing the patches independently or
> something? What automake version are you using? I’m on 1.11.3, but I’ve also
> tested with 1.12.2.

Yeah, I was stepping through the patches one at a time. I guess that was a bad idea. (Yeah, I'm using automake 1.12.2-5.fc18)

> > diff --git a/backends/libsocialweb/lib/Makefile.am
> > b/backends/libsocialweb/lib/Makefile.am
> > index 9c3c586..f07cb60 100644
> > --- a/backends/libsocialweb/lib/Makefile.am
> > +++ b/backends/libsocialweb/lib/Makefile.am
> > @@ -50,7 +50,7 @@ libfolks_libsocialweb_la_VALAFLAGS = \
> >         --includedir folks \
> >         --library folks-libsocialweb \
> >         --vapi folks-libsocialweb.vapi \
> > -       -H folks-libsocialweb.h \
> > +       -H folks/folks-libsocialweb.h \
> >         $(NULL)
> 
> Why should this one need changing back, but none of the other backends’?

This is related to the patch-stepping. I hadn't gotten to the next patch yet and you slightly mis-rebased (where this change happened in the first patch but it happened in the other backends in the next patch).

> >  libfolks_libsocialweb_la_CFLAGS = \
> > diff --git a/tests/lib/Makefile.am b/tests/lib/Makefile.am
> > index 77f7da0..def5939 100644
> > --- a/tests/lib/Makefile.am
> > +++ b/tests/lib/Makefile.am
> > @@ -90,6 +90,11 @@ libfolks_test_la_VALAFLAGS = \
> >         -g \
> >         $(NULL)
> > 
> > +BUILT_SOURCES = \
> > +       libfolks_test_la_vala.stamp \
> > +       $(NULL)
> > +
> > +
> 
> That shouldn’t be necessary, as automake magically generates a dependency from
> haze-remove-directory.c to folks-test-internal.h, and has a recipe to generate
> the latter using valac.
> 
> Take a look at tests/lib/.deps/libfolks_test_la-haze-remove-directory.Plo and
> see if it contains more than “# dummy”. If not, delete it and force it to be
> re-created; that should fix the build problem (though I think distcleaning does
> this).

Fixed in my new revision.
Comment 46 Travis Reitter 2013-07-05 18:49:26 UTC
Created attachment 248478 [details] [review]
build: Use target-specific VALAFLAGS rather than AM_VALAFLAGS (3)

This slightly fixes (and justifies) the use of BUILT_SOURCES for the internal test library
Comment 47 Travis Reitter 2013-07-05 19:15:22 UTC
Review of attachment 248462 [details] [review]:

Looks good
Comment 48 Travis Reitter 2013-07-05 19:15:49 UTC
Review of attachment 248463 [details] [review]:

Sure, it's ugly, but it seems like the best option for now. So let's squash this bug.
Comment 49 Philip Withnall 2013-07-05 21:03:33 UTC
All committed.

commit 5867cda8e899a873e65796b9aa0492bfc32b95f0
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Fri Jul 5 15:26:51 2013 +0100

    telepathy: Fix build with --disable-zeitgeist
    
    Vala doesn’t pass #if conditionals through to generated C code, so when
    building folks from a tarball, the --[enable|disable]-zeitgeist option was
    having no effect, and Zeitgeist was always being pulled into the build.
    
    Fix this by separating the Zeitgeist code out into a small noinst shim
    library and always generating C code for two versions of it: one with
    Zeitgeist enabled, and one with it disabled. The correct one’s C code is
    then compiled and linked into libfolks-telepathy.la when building from the
    tarball.
    
    This is a horrible, ugly fix, but the only better fix is to disable
    distribution of generated C code. This was attempted, but thwarted by
    automake’s Vala support.
    
    Closes: https://bugzilla.gnome.org/show_bug.cgi?id=701099

 backends/telepathy/lib/Makefile.am             | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 backends/telepathy/lib/tp-zeitgeist-dummy.vala |  50 +++++++++++++++++++++++++++
 backends/telepathy/lib/tp-zeitgeist.vala       | 231 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 backends/telepathy/lib/tpf-persona-store.vala  | 166 +++++++++-------------------------------------------------------------------------------
 backends/telepathy/lib/tpf-persona.vala        |  18 +++-------
 configure.ac                                   |   1 +
 6 files changed, 437 insertions(+), 171 deletions(-)

commit 6afd8a4f4d2a8f799aee98ba72d3b5e4ea4d3bb4
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Fri Jul 5 11:09:51 2013 +0100

    telepathy: Move Zeitgeist code entirely into Tpf.PersonaStore
    
    Ready to split it out into a shim library.
    
    Helps: https://bugzilla.gnome.org/show_bug.cgi?id=701099

 backends/telepathy/lib/tpf-persona-store.vala | 20 +++++++++++++++++++-
 backends/telepathy/lib/tpf-persona.vala       | 66 +++++++++++++++++++++++++++++-------------------------------------
 2 files changed, 48 insertions(+), 38 deletions(-)

commit 6b47d66df3f3079880b4b51656886a7cb5fdbec8
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Jun 20 17:28:35 2013 +0100

    build: Makefile whitespace fixes

 backends/libsocialweb/lib/Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

commit 04537355224ae5615b2eb3c40c2236702e7473e1
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Jun 20 17:27:50 2013 +0100

    build: Reorder DIST_SUBDIRS to match SUBDIRS
    
    So we don’t encounter problems during `make dist` due to recursing in
    a different order from normal.

 Makefile.am | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

commit 4c06ecc26bfde7397357ef4ae7ad83a5925db43d
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Jun 20 17:27:24 2013 +0100

    build: Don’t build Vala headers in a subdirectory unnecessarily

 backends/eds/lib/Makefile.am          | 6 +++---
 backends/libsocialweb/lib/Makefile.am | 6 +++---
 backends/telepathy/lib/Makefile.am    | 6 +++---
 backends/tracker/lib/Makefile.am      | 6 +++---
 4 files changed, 12 insertions(+), 12 deletions(-)

commit 9423e5e214110e3d4d3b77482db87a0ec310599b
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Jun 20 17:26:02 2013 +0100

    build: Remove generated C/H/vapi/stamp files from MAINTAINERCLEANFILES
    
    As long as --vapi and -H/-h options are correctly passed to a target-specific
    VALAFLAGS, automake will automatically add these generated files to the
    maintainer-clean target.
    
    Helps: https://bugzilla.gnome.org/show_bug.cgi?id=701099

 backends/eds/lib/Makefile.am          | 14 ++------------
 backends/libsocialweb/lib/Makefile.am | 15 ++-------------
 backends/telepathy/lib/Makefile.am    | 15 ++-------------
 backends/tracker/lib/Makefile.am      | 15 ++-------------
 tests/eds/Makefile.am                 | 10 ++--------
 tests/folks/Makefile.am               | 23 -----------------------
 tests/key-file/Makefile.am            | 15 ---------------
 tests/lib/Makefile.am                 |  9 +--------
 tests/lib/eds/Makefile.am             |  5 -----
 tests/lib/key-file/Makefile.am        |  5 -----
 tests/lib/libsocialweb/Makefile.am    |  5 -----
 tests/lib/telepathy/Makefile.am       | 11 -----------
 tests/lib/tracker/Makefile.am         |  5 -----
 tests/libsocialweb/Makefile.am        | 16 ----------------
 tests/telepathy/Makefile.am           | 11 -----------
 tests/tracker/Makefile.am             | 69 +++------------------------------------------------------------------
 16 files changed, 14 insertions(+), 229 deletions(-)

commit 037caf3a4d1d1b8de5fb6c4d78887793f36700a8
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Thu Jun 20 17:21:41 2013 +0100

    build: Use target-specific VALAFLAGS rather than AM_VALAFLAGS
    
    This allows automake to parse the flags for each target and generate
    appropriate rules. For example, by explicitly specifying “--vapi foo.vapi” in
    foo_VALAFLAGS (not just specifying “--library foo”), dependency and clean-up
    rules for foo.vapi will be generated by automake. Accordingly, this commit
    also adds missing --vapi flags.
    
    Helps: https://bugzilla.gnome.org/show_bug.cgi?id=701099

 backends/eds/Makefile.am              | 32 +++++++++++++++-----------------
 backends/eds/lib/Makefile.am          |  6 ------
 backends/libsocialweb/Makefile.am     | 21 +++++++++------------
 backends/libsocialweb/lib/Makefile.am | 36 ++++++++++++++----------------------
 backends/telepathy/Makefile.am        | 21 +++++++++------------
 backends/telepathy/lib/Makefile.am    |  1 +
 backends/tracker/Makefile.am          | 27 ++++++++++++---------------
 backends/tracker/lib/Makefile.am      | 11 +----------
 folks/Makefile.am                     | 11 -----------
 tests/lib/Makefile.am                 |  7 ++++++-
 tests/lib/eds/Makefile.am             | 12 +++++++-----
 tests/lib/key-file/Makefile.am        | 12 +++++++-----
 tests/lib/libsocialweb/Makefile.am    | 12 +++++++-----
 tests/lib/telepathy/Makefile.am       | 16 +++++++++-------
 tests/lib/tracker/Makefile.am         | 16 +++++++---------
 tests/telepathy/Makefile.am           |  9 ---------
 16 files changed, 104 insertions(+), 146 deletions(-)
Comment 50 Travis Reitter 2013-07-08 16:12:50 UTC
Our fix didn't quite seem to get 100% of the way there. With --disable-zeitgeist, without it installed, we still get a build failure because tp-zeitgeist.vapi is still being built.

I've got a branch to fix this.
Comment 51 Travis Reitter 2013-07-08 16:16:33 UTC
Created attachment 248623 [details] [review]
Don't always build tp-zeitgeist.vapi

Patch from branch:

http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=bgo701099-zeitgeist-build
Comment 52 Travis Reitter 2013-07-08 17:14:55 UTC
(In reply to comment #51)
> Created an attachment (id=248623) [details] [review]
> Don't always build tp-zeitgeist.vapi
> 
> Patch from branch:
> 
> http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=bgo701099-zeitgeist-build

For the record, these changes still include both the tp-zeitgeist and tp-zeitgeist-dummy generated C and VAPI files in the dist tarball, which is a requirement.
Comment 53 Philip Withnall 2013-07-08 17:26:46 UTC
Review of attachment 248623 [details] [review]:

Go for it.
Comment 54 Travis Reitter 2013-07-08 17:30:57 UTC
commit 8a495d461ba4c61724218c7e5b5be87dace7197f
Author: Travis Reitter <travis.reitter@collabora.co.uk>
Date:   Mon Jul 8 09:14:09 2013 -0700

    build: don't depend on tp-zeitgeist.vapi for --disable-zeitgeist
    
    Bug: https://bugzilla.gnome.org/show_bug.cgi?id=701099

 backends/telepathy/lib/Makefile.am | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)