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 694883 - srcdir != builddir (out-of-tree) builds don't work
srcdir != builddir (out-of-tree) builds don't work
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: general
0.9.x
Other Linux
: Normal normal
: Unset
Assigned To: folks-maint
folks-maint
Depends on:
Blocks:
 
 
Reported: 2013-02-28 16:55 UTC by Simon McVittie
Modified: 2013-03-07 08:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add -I$(top_srcdir) throughout (7.10 KB, patch)
2013-02-28 16:56 UTC, Simon McVittie
accepted-commit_now Details | Review
Use absolute paths for --vapidir (5.22 KB, patch)
2013-02-28 16:57 UTC, Simon McVittie
accepted-commit_now Details | Review

Description Simon McVittie 2013-02-28 16:55:51 UTC
I do most builds out-of-tree, to make it easier to build multiple copies of a component with different configurations. This doesn't currently work for Folks.
Comment 1 Simon McVittie 2013-02-28 16:56:48 UTC
Created attachment 237628 [details] [review]
Add -I$(top_srcdir) throughout

Automake adds (the equivalent of) -I$(top_builddir) to our CPPFLAGS
automatically, so we can #include "config.h". It does not add
-I$(top_srcdir). Many things in folks want to #include <folks/folks.h>,
so we should always have -I$(top_srcdir) so we can pick that up.

Out-of-tree builds previously either didn't work, or relied on having
installed folks headers (I'm not sure which).
Comment 2 Simon McVittie 2013-02-28 16:57:07 UTC
Created attachment 237629 [details] [review]
Use absolute paths for --vapidir

In the rules currently generated by Automake, valac currently runs
cd'd into the ${srcdir}. It seems unwise to rely on that, and the
only thing that will work regardless of whether ${srcdir} is ".",
relative or absolute is its "absolutized" version. 

Similarly, look in both the ${srcdir} and the ${builddir} for Vala
bindings we generate: the ${srcdir} because that's where valac currently
puts them, and the ${builddir} because that's where they ought to go
in principle.
Comment 3 Philip Withnall 2013-03-05 23:22:52 UTC
Review of attachment 237628 [details] [review]:

Looks good to me. It would be nice to refactor these Makefiles to use a common include which specifies all these paths, but I guess that’s a separate bug. (Patches welcome!)
Comment 4 Philip Withnall 2013-03-05 23:24:53 UTC
Review of attachment 237629 [details] [review]:

This is ugly, but if you think it’s necessary…

Have you filed a bug upstream with Automake to improve their valac support?

Does this still pass `make distcheck`? If so, please commit to master. Thanks.
Comment 5 Simon McVittie 2013-03-06 11:24:48 UTC
(In reply to comment #4)
> Have you filed a bug upstream with Automake to improve their valac support?

No; what in their valac support do you want improved? The only thing I can think of that would avoid this double-path-entry would be documenting/guaranteeing that valac flags will always be interpreted as relative to the (srcdir|builddir), or that Vala bindings will always be output into the (srcdir|builddir).

I personally think valac should be treated as a build-time dependency, just like gcc or g-ir-scanner (and as a corollary, the built .c/.h should not be distributed, just like you don't distribute prebuilt .o files) - if you don't re-generate the C from the Vala, you're not really compiling from the preferred form for modification, and you're going to have difficulty making a security update or other minimal bugfix.

However, I realise that the maintainers of Vala and Automake disagree with that point of view, and the ability to build from "source" with only a C compiler has been a major selling point for Vala in the past.

> Does this still pass `make distcheck`?

It seems distcheck doesn't work unless you have enabled all the backends (I disabled the socialweb backend to reduce the length of the dependency stack), so "not necessarily, ask again when I've compiled more packages". :-(
Comment 6 Simon McVittie 2013-03-06 12:26:17 UTC
(In reply to comment #4)
> Does this still pass `make distcheck`? If so, please commit to master. Thanks.

Turns out the answer is "no, because test_dummy_libsocialweb() segfaults". Sorry, I don't have time to go debugging that right now...

The libsocialweb bits also need to be included in "Use absolute paths for --vapidir".

I'm going to have to context-switch away from this because while running tests for this, eds/add-contacts-stress-test got into what appears to be the pathological performance seen on Bug #689549, in only the second time I've seen this in 2 days' testing... so I'm going to try to debug what's going on there while I have the chance!
Comment 7 Simon McVittie 2013-03-06 18:54:25 UTC
(In reply to comment #6)
> (In reply to comment #4)
> > Does this still pass `make distcheck`? If so, please commit to master. Thanks.
> 
> Turns out the answer is "no, because test_dummy_libsocialweb() segfaults".

With that test stubbed out, it works... which isn't a regression, so I'm going to push the changes.

Committed as 36ed61b85c (amended to do the same thing to the tracker and libsocialweb backends) and a08b1b2f41f3.
Comment 8 Philip Withnall 2013-03-06 22:34:59 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > Have you filed a bug upstream with Automake to improve their valac support?
> 
> No; what in their valac support do you want improved? The only thing I can
> think of that would avoid this double-path-entry would be
> documenting/guaranteeing that valac flags will always be interpreted as
> relative to the (srcdir|builddir), or that Vala bindings will always be output
> into the (srcdir|builddir).

As you say, clarifying which directory valac will be run in, and outputting Vala bindings in $builddir rather than $srcdir.

> However, I realise that the maintainers of Vala and Automake disagree with that
> point of view, and the ability to build from "source" with only a C compiler
> has been a major selling point for Vala in the past.

Mostly due to the instability of Vala. I think that it should be reasonable to start requiring valac as a build-time dependency soon. However, that’s not really my decision to make.

(In reply to comment #6)
> (In reply to comment #4)
> > Does this still pass `make distcheck`? If so, please commit to master. Thanks.
> 
> Turns out the answer is "no, because test_dummy_libsocialweb() segfaults".
> Sorry, I don't have time to go debugging that right now...
> 
> The libsocialweb bits also need to be included in "Use absolute paths for
> --vapidir".

Right.

> I'm going to have to context-switch away from this

Thanks a lot for your work on this (and EDS)!

(In reply to comment #7)
> Committed as 36ed61b85c (amended to do the same thing to the tracker and
> libsocialweb backends) and a08b1b2f41f3.

Great!
Comment 9 Travis Reitter 2013-03-07 08:00:24 UTC
(In reply to comment #8)

> Mostly due to the instability of Vala. I think that it should be reasonable to
> start requiring valac as a build-time dependency soon. However, that’s not
> really my decision to make.

I tried to do this several months ago. The only reason I didn't was because Vala's autotools support flat-out doesn't let you *not* ship the generated files (and I couldn't be bothered to fix it because I have a few thousand other things higher on my list).