Created attachment 164235 [details] [review]
[gnome-shell.in] Rerun under 'jhbuild run' if started outside jhbuild

For a future patch, we should ensure we're consistently inside the
jhbuild environment if we're not already in it.  This only applies
when being run from the source tree.

Created attachment 164236 [details] [review]
[gnome-shell.in] Start dconf

This keeps the jhbuild-on-older-OSes working.

Review of attachment 164235 [details] [review]:

Ugh. if necessary we could jbuild run the DConf daemon, which would be 100x cleaner.

(In reply to comment #3)
> Review of attachment 164235 [details] [review]:
> 
> Ugh. if necessary we could jbuild run the DConf daemon, which would be 100x
> cleaner.

I'd still need to know to only do that in the uninstalled case.

Review of attachment 164236 [details] [review]:

"older-OSes" being every single released operating system...

::: src/gnome-shell.in
@@ +131,3 @@
 
+def start_dconf_await_service():
+    import dbus, gobject

There's no point in having lazy imports if the function using them is always called (if changing that, fix the other function that does a local lazy import of dbus)

@@ +133,3 @@
+    import dbus, gobject
+    from dbus.mainloop.glib import DBusGMainLoop
+    loop = gobject.MainLoop()

This variable would be clearer if it's down with the "wait to start" code

@@ +139,3 @@
+    bus_iface = dbus.Interface(bus_proxy, 'org.freedesktop.DBus')
+
+    # This is racy actually, but also really horrible to try to fix here...

Not worried about the race condition of dconf just about to exit - in production we are counting on D-Bus activation

@@ +142,3 @@
+    names = bus_iface.ListNames()
+    if 'ca.desrt.dconf' in names:
+        print "Dconf already running"

This is not an error, and will always occur on production systems, it must not print anything

@@ +146,3 @@
+
+    dconf_path = None
+    for dirname in os.environ['PATH'].split(':'):

I think I'd just use the logic:

 - If the service isn't already running
 - And $libexecdir/dconf-service exists
 - Run it

Looking in the path only make sense if we consider someone who has multiple non-system prefixes mised together, which is almost impossible to get right. We are already making 'installed to the same prefix' assumptions for other things like GSettings. (OK, maybe just for that... can't find other examples.)

@@ +156,3 @@
+    if not dconf_path:
+        raise SystemExit("Couldn't find dconf-service")
+    subprocess.Popen([dconf_path])

As mentioned in the bug, you should pass --keep-alive to dconf-service

@@ +162,3 @@
+        loop.quit()
+    def on_timeout():
+        raise SystemExit("Failed to start dconf-service; timed out")

timeouts should always return an explicit False, counting on None being False isn't good

@@ +165,3 @@
+    gobject.timeout_add_seconds(7, on_timeout)
+    bus_iface.connect_to_signal('NameOwnerChanged', on_name_owner_changed)
+    loop.run()

Disconnect from NameOwnerChanged here?

(In reply to comment #4)
> (In reply to comment #3)
> > Review of attachment 164235 [details] [review] [details]:
> > 
> > Ugh. if necessary we could jbuild run the DConf daemon, which would be 100x
> > cleaner.
> 
> I'd still need to know to only do that in the uninstalled case.

Why only in the uninstalled case? it's intentional that you can symlink your gnome-shell desktop file into ~/.config/autostart and have it work.

Basically, we discussed the use 'jhbuild run' option when Dan originally wrote the environment munging code, and the outcome was that (because of rpaths, etc), we only had to set a very limited set of variables and it was easier not to get jhbuild into the picture.

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Review of attachment 164235 [details] [review] [details] [details]:
> > > 
> > > Ugh. if necessary we could jbuild run the DConf daemon, which would be 100x
> > > cleaner.
> > 
> > I'd still need to know to only do that in the uninstalled case.
> 
> Why only in the uninstalled case? it's intentional that you can symlink your
> gnome-shell desktop file into ~/.config/autostart and have it work.

It's really crazy how many different ways we have to run the shell...(and all of them pessimize the normal case by leaving around a python process).

> Basically, we discussed the use 'jhbuild run' option when Dan originally wrote
> the environment munging code, and the outcome was that (because of rpaths,
> etc), we only had to set a very limited set of variables and it was easier not
> to get jhbuild into the picture.

So...hm, we don't need LD_LIBRARY_PATH set because mutter has an rpath?

(In reply to comment #7)
> > Basically, we discussed the use 'jhbuild run' option when Dan originally wrote
> > the environment munging code, and the outcome was that (because of rpaths,
> > etc), we only had to set a very limited set of variables and it was easier not
> > to get jhbuild into the picture.
> 
> So...hm, we don't need LD_LIBRARY_PATH set because mutter has an rpath?

That's the theory. Discussion in bug 572916.

Also:

+        args = ['jhbuild', 'run'] + sys.argv
+        os.execvp('jhbuild', args)

FWIW, that would fail for me; my default .jhbuildrc is the GNOME one, and I have an alias for "jhbuild -f ~/.jhbuildrc-gs" for gnome-shell.

(In reply to comment #4)

> > Basically, we discussed the use 'jhbuild run' option when Dan originally wrote
> > the environment munging code, and the outcome was that (because of rpaths,
> > etc), we only had to set a very limited set of variables and it was easier not
> > to get jhbuild into the picture.
> 
> So...hm, we don't need LD_LIBRARY_PATH set because mutter has an rpath?

I've verified that running:

 $libexecdir/dconf-service

outside the jhbuild environment works correctly and links to the jhbuild GLib.
 
I'll see if I can come up with a revised patch based on your patch and the simpler approach - I'd like to get the DConf switch in before I roll tarballs today so people using tarballs don't lose there settings once with the switching to GSettings and a second time with the switch to DConf.

Created attachment 164624 [details] [review]
Switch to using dconf

Here's a full patch that I think should work pretty well.

I switched the approach for D-Bus a bit - instead of looking
for the bus name before trying to start I try to ping the
service.

The reason for this is to handle the case where:

 - We have a system install of DConf
 - It's not running at the moment (exited from a timeout or whatever)

And only do manual activation if we really need to. There may
be problems in the future with JHBuild dconf module not working
with system DConf daemon, but if that becomes a problem, we will
need to actively kill the system DConf daemon. After 3.0 is out
we will presumably just rely on system DConf and not jhbuild it.

Assigning to walters for review, since the most tricky parts are likely the D-Bus parts.

Review of attachment 164624 [details] [review]:

Actually I had prepared the same patch, just waiting for the D-Bus parts. But that was not a lot of work, apart from debugging dconf. BTW, I'm running the Shell with dconf here and it works fine.

So, not reviewing the D-Bus parts, I have the joy to mark your patch as 'needs-work' ;-) :
- you need to remove GSETTINGS_BACKEND=gconf from gnome-shell-clock-preferences too.
- I think dconf requires libxml2 development files, which you didn't add to the build setup script (on Debian this is libxml2-dev)
- it might be good to find what packages we need for SuSE and Mandriva, before people come and complain this doesn't work ;-)
- valac in most distributions is too old for dconf (I tried and experienced GLib API issues), which needs 0.9.2. The best is likely to use that version instead of relying on git master, to avoid adding more bugs. So from the official GNOME modulesets:
  <tarball id="vala" version="0.9.2" autogenargs="--enable-vapigen">
    <source href="http://ftp.gnome.org/pub/GNOME/sources/vala/0.9/vala-0.9.2.tar.bz2"
            hash="sha256:22e1e224790663929f5df8b0611bd4928c065a8354ee7cdd2c97b6b37ed33c1d"
            md5sum="583f2c46da49f54e4f639eb706475abe" size="2358061"/>
    <dependencies>
      <dep package="glib"/>
    </dependencies>
  </tarball>

Created attachment 164630 [details] [review]
Switch to using dconf

Thanks for the comments; should be fixed here. I went ahead and
added vala and libgee to the moduleset, since it appears that
a newer vala is needed at least on Ubuntu, though the 0.8.1
on Fedora worked fine for me. I left the Mandriva and SuSE
parts of gnome-shell-build-setup.sh untouched, since they
are otherwise way out of date, and if they break, and
someone complains, it's a good opportunity to ask them to
fix the breakage.

Review of attachment 164630 [details] [review]:

Looks okayish to me, two really minor comments.

::: src/gnome-shell.in
@@ +152,3 @@
+        subprocess.Popen([dconf_path, '--keep-alive'])
+    except OSError, e:
+        print "\nFailed to start %s: %s" % (dconf_path, e)

Should probably >>sys.stderr,

@@ +161,3 @@
+
+    def on_name_owner_changed(name, prev_owner, new_owner):
+        if not (name == 'ca.desrt.dconf' and new_owner != ''):

Might make a local variable for ca.desrt.dconf

Review of attachment 164630 [details] [review]:

Looks okayish to me, two really minor comments.

::: src/gnome-shell.in
@@ +152,3 @@
+    except OSError, e:
+        print "\nFailed to start %s: %s" % (dconf_path, e)
+def start_dconf_await_service():

Should probably >>sys.stderr,

@@ +161,3 @@
+
+    def on_name_owner_changed(name, prev_owner, new_owner):
+        if not (name == 'ca.desrt.dconf' and new_owner != ''):

Might make a local variable for ca.desrt.dconf

Attachment 164630 [details] pushed as e8b72a2 - Switch to using dconf