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 641724 - gnome-shell's launcher script shouldn't be in Python
gnome-shell's launcher script shouldn't be in Python
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2011-02-07 14:00 UTC by Guillaume Desmottes
Modified: 2011-03-14 15:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
initial libmutter work (1.87 KB, patch)
2011-03-03 17:24 UTC, Dan Winship
needs-work Details | Review
the mutter side of step 2 (8.71 KB, patch)
2011-03-03 17:41 UTC, Dan Winship
needs-work Details | Review
the non-working gnome-shell side of the step 2 mutter patch. (6.27 KB, patch)
2011-03-03 17:42 UTC, Dan Winship
needs-work Details | Review
Use libmutter-wm, and build a real gnome-shell binary (50.81 KB, patch)
2011-03-05 16:08 UTC, Dan Winship
needs-work Details | Review
src: update for mutter include reorganization (8.41 KB, patch)
2011-03-05 16:08 UTC, Dan Winship
accepted-commit_now Details | Review
Use libmutter-wm, and build a real gnome-shell binary (52.87 KB, patch)
2011-03-06 18:07 UTC, Dan Winship
committed Details | Review
src: update for mutter include reorganization (8.41 KB, patch)
2011-03-06 18:08 UTC, Dan Winship
committed Details | Review
Simplify and bashify gnome-shell-extension-tool (5.33 KB, patch)
2011-03-14 15:13 UTC, Quentin "Sardem FF7" Glidic
none Details | Review

Description Guillaume Desmottes 2011-02-07 14:00:02 UTC
The src/gnome-shell executable is currently a Python script. I do love Python very much, but having to launch a VM to start an app which using another VM is pretty silly.
Comment 1 Dan Winship 2011-02-07 14:25:58 UTC
The majority of the script is just to deal with running GNOME Shell in a GNOME 2.x environment (or running it from the source tree). But yeah, I'd always been assuming that it would eventually turn into something close to:

    #!/bin/sh

    exec mutter --mutter-plugins=libgnome-shell "$@"

The --xephyr stuff is going to go away. The --perf and --create-extension stuff could go into separate scripts. It wouldn't be hard to do the rest as a shell script.
Comment 2 Giovanni Campagna 2011-02-07 18:05:38 UTC
(In reply to comment #1)
> The majority of the script is just to deal with running GNOME Shell in a GNOME
> 2.x environment (or running it from the source tree). But yeah, I'd always been
> assuming that it would eventually turn into something close to:
> 
>     #!/bin/sh
> 
>     exec mutter --mutter-plugins=libgnome-shell "$@"
> 
> The --xephyr stuff is going to go away. The --perf and --create-extension stuff
> could go into separate scripts. It wouldn't be hard to do the rest as a shell
> script.

Even better, the --create-extension can be removed, now that gnome-shell-extensions ships the same example.
Comment 3 Lennart Poettering 2011-02-08 20:03:32 UTC
the gnome-shell python script shows up very unfavourably in bootchart (for example, here: http://www.harald-hoyer.de/personal/blog/2011-02-03-fedora-rawhide-bootcharts -- but that's not even a slow machine...), especially on slower hw and on rotating media. Pulling in Python is a recipe for making the boot slow. Also, invoking glxinfo from the script makes it even worse. It would be great if the python script could go away.

On fedora we have been trying hard to speed things up by removing shells from the boot process. Due to that we'd much appreciate if the python script would not be replaced by a shell script, but would simply go away entirely, and g-s/mutter could just be invoked like any other C program.
Comment 4 Dan Winship 2011-02-14 15:40:27 UTC
(In reply to comment #3)
> On fedora we have been trying hard to speed things up by removing shells from
> the boot process. Due to that we'd much appreciate if the python script would
> not be replaced by a shell script, but would simply go away entirely, and
> g-s/mutter could just be invoked like any other C program.

Hm... I was going to say that we can't easily do that (unless we had a C program that was just the equivalent of the above shell script), but I guess it would be pretty trivial to just build a "libmutter" with -Dmain=mutter_main, and then have gnome-shell be a binary that links to libmutter, does its own initialization, adds "--mutter-plugins=libgnome-shell" to argv and then calls mutter_main(). And then we could make it progressively saner from there. (Having the "plugin" be in process rather than dlopened, splitting mutter's main into separate mutter_init() and mutter_run() sections so we could move the initialization stuff from gnome-shell-plugin.c into main instead, etc.)
Comment 5 Lennart Poettering 2011-02-14 15:47:07 UTC
Another simple solution would be to simply fork off and exec mutter from g-s with the right command line. If there's value in having g-s and mutter be two processes that would be a pretty good solution. 

I am mostly pushing for eradication of python and shells scripts from our default boot  path. Whether mutter/g-s are two or one processes doesn't really matter too much...
Comment 6 Dan Winship 2011-02-14 15:57:36 UTC
It would be just exec, not fork+exec. The problem isn't multiple processes, it's that the actual thing-which-is-GNOME-Shell is not a standalone binary, it's a plugin that gets loaded by mutter. So (as things are organized now) if there's a /usr/bin/gnome-shell, it would be something that execs mutter with the correct arguments to cause it to load libgnome-shell.so.
Comment 7 Lennart Poettering 2011-02-14 16:11:43 UTC
Ah, I see, so there is no such thing as a gnome-shell binary? 

Maybe a solution would be to add some code to mutter to simply check whether argv[0] includes "gnome-shell" and in this case just hardcode that libgnome-shell.so is loaded?

Or, just give up on the gnome-shell executable name and just directly invoke mutter with the right parameters everywhere, like in the .desktop file.
Comment 8 Colin Walters 2011-02-14 17:56:03 UTC
See also bug 642084.
Comment 9 Colin Walters 2011-02-18 19:25:00 UTC

*** This bug has been marked as a duplicate of bug 642084 ***
Comment 10 Dan Winship 2011-03-03 17:24:09 UTC
reopening to consider the possibility of actually getting rid of the need for a wrapper
Comment 11 Dan Winship 2011-03-03 17:24:53 UTC
Created attachment 182373 [details] [review]
initial libmutter work
Comment 12 Dan Winship 2011-03-03 17:40:26 UTC
oops, I apparently never committed the working state of the gnome-shell side of that patch, and only have a later, broken, state now.

The idea here is: make the absolute minimal change to turn mutter into a library, and then make a tiny tiny gnome-shell binary that just links to that library (but handles things like setting NO_AT_BRIDGE and adding --mutter-plugins=libgnome-shell.la to argv before calling meta_main, and which can be forcibly linked-with-rpath to the right libmozjs, per bug 643799, and which will save us having to repeatedly explain why "why does gnome-shell only work with mutter?" is not even a meaningful question)

Step two was going to be to get rid of the dynamic loading of the plugin, and just link what is currently libgnome-shell.la into the gnome-shell binary, but my first attempt at that didn't work, because the introspection was getting messed up somehow (the running binary couldn't find St) and then I didn't get back to it.

Thoughts?
Comment 13 Dan Winship 2011-03-03 17:41:34 UTC
Created attachment 182382 [details] [review]
the mutter side of step 2
Comment 14 Dan Winship 2011-03-03 17:42:54 UTC
Created attachment 182383 [details] [review]
the non-working gnome-shell side of the step 2 mutter patch.

this predates Colin's wrapper script changes and won't apply cleanly to HEAD
Comment 15 Colin Walters 2011-03-03 18:15:21 UTC
Review of attachment 182373 [details] [review]:

Hah, a very simple patch.  How hard would it be though to change main.c to call mutter_main()?  The way things are right now, everything gets built twice.
Comment 16 Dan Winship 2011-03-03 18:41:46 UTC
(In reply to comment #15)
> Hah, a very simple patch.  How hard would it be though to change main.c to call
> mutter_main()?  The way things are right now, everything gets built twice.

Well, I wasn't sure if we would want to keep the mutter binary around at all in this case.
Comment 17 Colin Walters 2011-03-03 18:47:41 UTC
(In reply to comment #16)
> (In reply to comment #15)
> > Hah, a very simple patch.  How hard would it be though to change main.c to call
> > mutter_main()?  The way things are right now, everything gets built twice.
> 
> Well, I wasn't sure if we would want to keep the mutter binary around at all in
> this case.

We've been using it for testing, so it seems to have some value, and it can't be that hard to keep around is it?
Comment 18 Dan Winship 2011-03-03 19:07:29 UTC
No, I just meant, I didn't know if we were going to decide "we should keep the mutter binary and make it link against libmutter-wm" or "we should ditch the mutter binary", and so I didn't make any attempt to implement either of those for now.
Comment 19 Owen Taylor 2011-03-03 19:10:29 UTC
Having raw mutter available has been useful to try and sort out different classes of bugs, so I think it's worth a small amount of effort to maintain it.
Comment 20 Colin Walters 2011-03-03 19:29:20 UTC
(In reply to comment #19)
> Having raw mutter available has been useful to try and sort out different
> classes of bugs, so I think it's worth a small amount of effort to maintain it.

Though, we could also have a gnome-shell command line options to disable various things (--disable-javascript --disable-animations).
Comment 21 Owen Taylor 2011-03-04 19:11:07 UTC
Review of attachment 182373 [details] [review]:

I think this approach is worth pursuing - going to make the system look more "real" and being able to use RPATH will likely help with spidermonkey for gnome-shell. (Though doesn't help at all if people people put libmozjs.so in a directory that moves with ffox security updates.) 

I'd like to keep the "mutter" binary around, so  it's worth the extra few lines of Makefile.am changes and the mutter-main.c file to make everything not build twice.
Comment 22 Owen Taylor 2011-03-04 19:37:27 UTC
Review of attachment 182382 [details] [review]:

Seems OK as an intermediate step to some less pluginified future. Will need work for changes in the preceding patch requested to make mutter use libmutter-wm

::: src/include/meta-plugin.h
@@ +223,2 @@
 void
+meta_plugin_register (MetaPlugin *plugin);

I think i'd call this (and the MPM method) install, not register
Comment 23 Owen Taylor 2011-03-04 19:39:41 UTC
Review of attachment 182383 [details] [review]:

Not sure if there are other problems, but you've clearly killed the --introspect functionality. You need to do something like do a manual preparse of the argv and look for --introspect and then call 
g_irepository_dump() yourself.

Or alternatively, make the init()/register()/run() thing work for --introspect mode
Comment 24 Dan Winship 2011-03-05 16:08:35 UTC
Created attachment 182557 [details] [review]
Use libmutter-wm, and build a real gnome-shell binary

Build gnome-shell as a binary linked against libmutter-wm, instead of
a module to be loaded by libmutter-wm. Move the majority of
initialization-type stuff from gnome_shell_plugin_start() into main().

We still build libgnome-shell as a shared library, so that the linker
doesn't discard all the methods that are never called from C.

===

see bug 643959 for the corresponding mutter patch
Comment 25 Dan Winship 2011-03-05 16:08:46 UTC
Created attachment 182558 [details] [review]
src: update for mutter include reorganization
Comment 26 Dan Winship 2011-03-05 16:10:37 UTC
(In reply to comment #23)
> Review of attachment 182383 [details] [review]:
> 
> Not sure if there are other problems, but you've clearly killed the
> --introspect functionality.

Yes, but it doesn't matter, because we don't introspect via --program any more.
Comment 27 Owen Taylor 2011-03-05 16:49:49 UTC
Review of attachment 182557 [details] [review]:

> We still build libgnome-shell as a shared library, so that the linker doesn't discard all the methods that are never called from C.

The -export-dynamic flag should take care of that. It's used in Mutter currently and if it wasn't working, then we'd be unable to add a new method to Mutter that is used only from gnome-shell and then use it from gnome-shell, and we do that all the time.

::: src/Makefile.am
@@ +18,3 @@
 CLEANFILES += $(gir_DATA) $(typelib_DATA)
 
+bin_SCRIPTS += gnome-shell gnome-shell-extension-tool

Is using bin_SCRIPTS for an executable going to break the relinking thing that libtool does on installation? Do we risk installing a libtool wrapper script or a binary with a rpath to the build directory? Maybe we should always have gnome-shell-real in bin_PROGRAMS then have install-exec-hook move gnome-shell-real to gnome-shell?

@@ +58,3 @@
 gnome_shell_cflags =				\
+	$(GNOME_SHELL_CFLAGS)			\
+	$(BLUETOOTH_CFLAGS)			\

I don't see anything in configure.ac that would cause BLUETOOTH_CFLAGS to be defined (in general, the existence of <SOME_DEP>_CFLAGS is a bug from improper use of pkg-config, but the bluetooth stuff is pretty messy, so maybe it's necessary. But still don't see where it is set.)

::: src/gnome-shell-jhbuild.in
@@ +167,3 @@
     # run".  See bug #642084
     env.update({'GNOME_SHELL_JS'      : js_dir,
+                'PATH'                : '@bindir@:' + os.environ.get('PATH', ''),

Hmm, it's pretty confusing to have @bindir@ and bin_dir be different things. Maybe it's OK though, since @bindir@ has a well established meaning. Possibly consider renaming bin_dir to executable_dir or something? (just a thought)

@@ +499,3 @@
                   help="Run under a debugger")
 parser.add_option("", "--debug-command", metavar="COMMAND",
+                  help="Command to use for debugging (defaults to 'libtool --mode=execute gdb --args')")

I think the libtool --mode=execute should be added automatically to whatever debug command is passed when running uninstalled. If I want to valgrind I should be able to 'gnome-shell --debug-command=valgrind' not 'gnome-shell --debug-comand="libtool --mode=execute valgrind"'

::: src/main.c
@@ +2,3 @@
+
+/*
+ * Copyright (c) 2011 Red Hat, Inc.

no '(c)' Not sure what is best here - this is accurate in that there is some small amount of stuff here which is copyright 2011 Red Hat, but there's a fair bit of stuff from other copyright holders and most stuff is from other years. (Less concerned about the other years... I think I've even seen recommendations that you can just put the last year.) Don't have a better recommendation other than tracking down copyright holders fully, which it probably doesn't make sense to do for one file in isolation.

::: tests/run-test.sh.in
@@ +31,3 @@
 srcdir=`cd $srcdir && pwd`
 
+GI_TYPELIB_PATH="@MUTTER_TYPELIB_DIR@:$builddir/../src"

You need to adjust tests/Makefile.am for this
Comment 28 Owen Taylor 2011-03-05 16:52:28 UTC
Review of attachment 182558 [details] [review]:

Looks fine. There's still a gap between the metacity headers and what would be OK for a real first class library, but its fine to group things as if it were such a thing.
Comment 29 Dan Winship 2011-03-06 01:17:37 UTC
(In reply to comment #27)
> > We still build libgnome-shell as a shared library, so that the linker doesn't discard all the methods that are never called from C.
> 
> The -export-dynamic flag should take care of that.

-export-dynamic causes all of gnome-shell's own unused symbols to be kept (eg, shell_generic_container_*), but doesn't cause libst, libgvc, etc, symbols to be kept. -whole-archive might work for that, but it's incompatible with automake.

Another possibility would be to get rid of all of the intermediate libraries, and just link everything directly into gnome-shell directly...

> Is using bin_SCRIPTS for an executable going to break the relinking thing that
> libtool does on installation?

Meh. Probably.

> I don't see anything in configure.ac that would cause BLUETOOTH_CFLAGS to be
> defined

Yes, I saw BLUETOOTH_LIBS and assumed they had just forgotten BLUETOOTH_CFLAGS, and didn't notice because it was just "-I$(prefix)" which we already had. But yes, it doesn't exist. (Nor does $(LIBECAL_CFLAGS), which appears elsewhere...)

> consider renaming bin_dir to executable_dir or something? (just a thought)

I renamed it to self_dir, since it's the directory where the script is.

> + * Copyright (c) 2011 Red Hat, Inc.
> 
> no '(c)'

oops, I just copied it from mutter/src/core/mutter.c, which I copied from mutter/src/core/main.c...

Maybe I'll just remove it, since we don't have copyright notices on most of the other files...
Comment 30 Owen Taylor 2011-03-06 01:49:23 UTC
(In reply to comment #29)
> (In reply to comment #27)
> > > We still build libgnome-shell as a shared library, so that the linker doesn't discard all the methods that are never called from C.
> > 
> > The -export-dynamic flag should take care of that.
> 
> -export-dynamic causes all of gnome-shell's own unused symbols to be kept (eg,
> shell_generic_container_*), but doesn't cause libst, libgvc, etc, symbols to be
> kept. -whole-archive might work for that, but it's incompatible with automake.
> 
> Another possibility would be to get rid of all of the intermediate libraries,
> and just link everything directly into gnome-shell directly...

Ugh. Don't think it's worth a lot of fiddling - I can't think of any real harm from the private library approach other than maybe a tiny bit of slowdown from -fPIC code and being unable to set breakpoints in gdb before main() is hit.
Comment 31 Dan Winship 2011-03-06 18:07:55 UTC
Created attachment 182626 [details] [review]
Use libmutter-wm, and build a real gnome-shell binary

Build gnome-shell as a binary linked against libmutter-wm, instead of
a module to be loaded by libmutter-wm. Move the majority of
initialization-type stuff from gnome_shell_plugin_start() into main().

We still build libgnome-shell as a shared library, so that the linker
doesn't discard all the methods that are never called from C.
Comment 32 Dan Winship 2011-03-06 18:08:02 UTC
Created attachment 182627 [details] [review]
src: update for mutter include reorganization
Comment 33 Owen Taylor 2011-03-07 02:46:57 UTC
Review of attachment 182626 [details] [review]:

Looks good to me
Comment 34 Owen Taylor 2011-03-07 02:47:50 UTC
Review of attachment 182627 [details] [review]:

Looks good
Comment 35 Dan Winship 2011-03-07 23:36:39 UTC
Attachment 182626 [details] pushed as ae96b0c - Use libmutter-wm, and build a real gnome-shell binary
Attachment 182627 [details] pushed as e187961 - src: update for mutter include reorganization
Comment 36 Quentin "Sardem FF7" Glidic 2011-03-14 15:13:30 UTC
Created attachment 183350 [details] [review]
Simplify and bashify gnome-shell-extension-tool

Python is not really needed here as Bash can easily do all the work
Comment 37 Owen Taylor 2011-03-14 15:57:12 UTC
* Don't attach patches to closed bugs, open a new bug.
* What's the advantage? GNOME Shell is going to keep on shipping Python utility scripts for performance testing, so having a Python script for extension creation seems like not a big deal.
* I could certainly imagine this getting more complex over time.