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 642084 - Replace Python wrapper script with shell script exec by default, add jhbuild option
Replace Python wrapper script with shell script exec by default, add jhbuild ...
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-11 04:50 UTC by Colin Walters
Modified: 2011-03-01 15:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gnome-shell-full: Rename Python wrapper, add new fast shell script (54.10 KB, patch)
2011-02-14 16:23 UTC, Colin Walters
none Details | Review
gnome-shell.in: Use jhbuild automatically, avoid setting bad environment variables (4.67 KB, patch)
2011-02-16 15:16 UTC, Colin Walters
none Details | Review
gnome-shell.in: Only start dconf in --replace mode (952 bytes, patch)
2011-02-17 15:58 UTC, Colin Walters
none Details | Review
gnome-shell.in: Move --create-extension to separate tool (4.80 KB, patch)
2011-02-17 15:58 UTC, Colin Walters
accepted-commit_now Details | Review
gnome-shell.in: In the normal case, use exec() (1.90 KB, patch)
2011-02-17 15:58 UTC, Colin Walters
none Details | Review
gnome-shell.in: Move extension creation to gnome-shell-extension-tool (10.22 KB, patch)
2011-02-18 18:39 UTC, Colin Walters
committed Details | Review
gnome-shell.in: Drop TFP checks (3.28 KB, patch)
2011-02-18 18:40 UTC, Colin Walters
committed Details | Review
gnome-shell.in: Drop --eval-file option (1.63 KB, patch)
2011-02-18 18:46 UTC, Colin Walters
committed Details | Review
Add a configure option --enable-jhbuild-wrapper-script (7.36 KB, patch)
2011-02-18 18:46 UTC, Colin Walters
reviewed Details | Review
Shell script for gnome-shell-extension-tool (2.54 KB, text/plain)
2011-02-18 23:50 UTC, Quentin "Sardem FF7" Glidic
  Details
Shell script for gnome-shell-extension-tool (2.58 KB, text/plain)
2011-02-19 11:47 UTC, Quentin "Sardem FF7" Glidic
  Details
Shell script for gnome-shell-extension-tool (2.56 KB, text/plain)
2011-02-24 18:13 UTC, Quentin "Sardem FF7" Glidic
  Details
updated (44.18 KB, patch)
2011-02-25 20:57 UTC, Colin Walters
committed Details | Review

Description Colin Walters 2011-02-11 04:50:37 UTC
Basically every environment variable we set in gnome-shell.in that happens in the "normal" case (as opposed to run-from-source-tree) is wrong, I think.  But in particular:

        env.update({'GSETTINGS_SCHEMA_DIR' : os.path.join('@datadir@', 'glib-2.0', 'schemas')})

has the seriously undesirable effect that a normal gnome-shell install will tell gsettings to ONLY load schemas from that path, ignoring XDG_DATA_DIRS.

I'm trying to recall again why the gnome-shell wrapper needs to have half of a reimplementation of jhbuild?  It was something like symlinking gnome-shell.desktop in your autostart?  If that's the case I think we just need to special case that one; basically provide a gnome-shell-jhbuild.desktop which does:

Exec=jhbuild run gnome-shell
Comment 1 Dan Winship 2011-02-11 12:21:37 UTC
(In reply to comment #0)
>         env.update({'GSETTINGS_SCHEMA_DIR' : os.path.join('@datadir@',
> 'glib-2.0', 'schemas')})
> 
> has the seriously undesirable effect that a normal gnome-shell install will
> tell gsettings to ONLY load schemas from that path, ignoring XDG_DATA_DIRS.

Looking at gsettingsschema.c, that doesn't seem to be the case. However, you're right that gsettings looks as XDG_DATA_DIRS (as well), and we don't need to be setting GSETTINGS_SCHEMA_DIR at all in this case.

> I'm trying to recall again why the gnome-shell wrapper needs to have half of a
> reimplementation of jhbuild?  It was something like symlinking
> gnome-shell.desktop in your autostart?

IIRC, it's just that at the time I added all that stuff, gnome-shell would generally *mostly* work if you didn't say "jhbuild run", but various random things would fail sporadically in non-obvious ways. Making it not depend on the user remembering to say "jhbuild run" seemed to make life simpler for everyone.

These days, if we removed the jhbuild-ish stuff, and the user didn't say "jhbuild run", it would most likely crash instantly, due to missing gsettings schemas and stuff, so we wouldn't run into that problem any more.
Comment 2 Colin Walters 2011-02-11 15:08:14 UTC
(In reply to comment #1)
> (In reply to comment #0)
> >         env.update({'GSETTINGS_SCHEMA_DIR' : os.path.join('@datadir@',
> > 'glib-2.0', 'schemas')})
> > 
> > has the seriously undesirable effect that a normal gnome-shell install will
> > tell gsettings to ONLY load schemas from that path, ignoring XDG_DATA_DIRS.
> 
> Looking at gsettingsschema.c, that doesn't seem to be the case. 

Yes, I was wrong - if you're running gnome-shell from the system, but inside a "jhbuild shell" in a terminal emulator, the effect is that the *system* wins, because GSETTINGS_SCHEMA_DIR is processed *after* XDG_DATA_DIRS.

So "ignoring" was wrong, but anyways it's not what we want and it caused me to be really confused why my schema changes weren't appearing.

> These days, if we removed the jhbuild-ish stuff, and the user didn't say
> "jhbuild run", it would most likely crash instantly, due to missing gsettings
> schemas and stuff, so we wouldn't run into that problem any more.

Okay, so let me outline a rough plan in my mind here:

* Create three different things:
  - gnome-shell-tool: This script holds all of the random stuff like the extension creation, profiling mode, etc.
  - gnome-shell-replace-wrapper: This tool takes care of all of the details of killing GNOME 2 on Fedora 14 and Ubuntu whatever, babysitting a mutter process, then restoring GNOME 2 when that exits.  gnome-shell-tool --replace will re-exec itself with this.  It also takes care of detecting whether we're running uninstalled, and re-execs itself with "jhbuild run" if necessary.
  - gnome-shell: This is just:  exec mutter --plugin=gnome-shell.la, *except* that if it detects *any* other arguments (e.g. --replace) it instead defers to gnome-shell-tool which is the full on Python script.
Comment 3 Jasper St. Pierre (not reading bugmail) 2011-02-11 15:13:46 UTC
The --create-extension switch isn't needed (and out of date), with the example in http://git.gnome.org/browse/gnome-shell-extensions/tree/extensions/example

It might be a bit better to create a gnome-shell-perf script in Python, and drop gnome-shell-tool completely.
Comment 4 Colin Walters 2011-02-14 16:23:44 UTC
Created attachment 180837 [details] [review]
gnome-shell-full: Rename Python wrapper, add new fast shell script

In the normal installed case, we don't want:

1) A heavyweight Python process sitting around doing nothing
2) Pollution from environment variables only needed when running
   uninstalled

Add a new shell script which checks to see if we have no arguments;
if so, just exec mutter and we're done.  In the uninstalled or
jhbuild case, one needs to specify --replace, so we'll drop into
the full complexity.
Comment 5 Owen Taylor 2011-02-14 16:54:34 UTC
(In reply to comment #4)
> Created an attachment (id=180837) [details] [review]
> gnome-shell-full: Rename Python wrapper, add new fast shell script
> 
> In the normal installed case, we don't want:
> 
> 1) A heavyweight Python process sitting around doing nothing
> 2) Pollution from environment variables only needed when running
>    uninstalled
> 
> Add a new shell script which checks to see if we have no arguments;
> if so, just exec mutter and we're done.  In the uninstalled or
> jhbuild case, one needs to specify --replace, so we'll drop into
> the full complexity.

Not really seeing this as the right way forward:

 * There's clearly no way we want gnome-shell and gnome-shell --replace to do something different for setting environment variables.

 * running uninstalled, similarly, it can't be the case that ./src/gnome-shell --replace has the logic for properly running uninstalled and ./src/gnome-shell without the --replace doesn't.

 * So if we do a two-part split it has to be *only* about optimization. It can have no other effects. We can't solve whether we set GSETTINGS_SCHEMA_DIR or not by making it set it some time and not other times depending on how things are invoked. In other words, it could conceivably go someway along the way of addressing bug 641724 but it can't have any function for this bug.

And as a minor thing:

 * I don't want gnome-shell *and* gnome-shell-full in bindir. If we want to do something two-part like this, one part has to be hidden in libexecdir.
Comment 6 Owen Taylor 2011-02-14 17:06:10 UTC
Also, note the setting of NO_GAIL and NO_AT_BRIDGE documented in gnome-shell.in.
Comment 7 Colin Walters 2011-02-14 17:43:37 UTC
(In reply to comment #5)
> 
>  * There's clearly no way we want gnome-shell and gnome-shell --replace to do
> something different for setting environment variables.

I agree, but I wanted to avoid regressions.

>  * running uninstalled, similarly, it can't be the case that ./src/gnome-shell
> --replace has the logic for properly running uninstalled and ./src/gnome-shell
> without the --replace doesn't.

But in what scenario is ./src/gnome-shell run uninstalled without --replace?  You pretty much have to be running a desktop to type it into a terminal.  The only one I can think of is symlinking the gnome-shell.desktop file, but I don't see how that works without --replace too, since gnome-session will be running another WM by default.

I guess I'm not sure what concrete regressions you're thinking of here.
Comment 8 Colin Walters 2011-02-16 15:16:34 UTC
Created attachment 181007 [details] [review]
gnome-shell.in: Use jhbuild automatically, avoid setting bad environment variables

When installed in the normal case, we want to avoid setting most
environment variables.  The particular problematic one I hit was
GSETTINGS_SCHEMA_DIR.

The reason we appeared to be doing this before was to handle the
uninstalled case and automatically set some jhbuild-like variables.
Make this sane by detecting the uninstalled case, and running the real
jhbuild.

In the installed case, we only set a few accessibility variables.
Comment 9 Colin Walters 2011-02-17 15:58:45 UTC
Created attachment 181140 [details] [review]
gnome-shell.in: Only start dconf in --replace mode

Avoids unnecessary round trips to the bus, and moves
us closer to refactoring out all the jhbuild
bits from the normal installed case.
Comment 10 Colin Walters 2011-02-17 15:58:48 UTC
Created attachment 181141 [details] [review]
gnome-shell.in: Move --create-extension to separate tool

It will live in gnome-shell-extensions/gnome-shell-extension-tool.
Comment 11 Colin Walters 2011-02-17 15:58:51 UTC
Created attachment 181142 [details] [review]
gnome-shell.in: In the normal case, use exec()

If we're not --replace or --debug, all that this Python script
currently does is stay blocked in waitpid() on the shell, and then
eventually when it exits, prints its exit status.  This is just a
superfluous line in ~/.xsession errors and definitely memory
inefficient, so avoid doing it by using exec().
Comment 12 Dan Winship 2011-02-17 16:42:35 UTC
Having "--replace" control these behaviors feels wrong to me. There are use cases for --replace that don't involve jhbuild, running out of srcdir, etc. (Otherwise there'd be no "metacity --replace".)

My theory for a split would be something like:

    - $(bindir)/gnome-shell just runs gnome-shell and does not make
      any attempt to compensate for jhbuild or GNOME 2 (but does set
      the a11y env vars, and maybe deals with the Ubuntu spidermonkey
      problem). This is what you run if you're in a GNOME 3
      environment (eg, the Rawhide GNOME session would run this).

    - $(bindir)/gnome-shell-jhbuild does GNOME 2 compensation (start
      dconf, etc), and then execs "jhbuild run $(bindir)/gnome-shell".
      It is only built/installed when you're building from git. This is
      what you run if you are using GNOME 2, but want to have
      GNOME Shell as your default session

    - src/gnome-shell has the logic for running a jhbuilt gnome-shell
      out of the source directory and then restoring the previous
      environment when it exits. It never gets installed (and might
      get renamed, or moved to tools/, to avoid conflicting with the
      script that gets installed in $(bindir)).
Comment 13 Dan Winship 2011-02-17 16:45:10 UTC
Comment on attachment 181141 [details] [review]
gnome-shell.in: Move --create-extension to separate tool

I think we all agree on this part though
Comment 14 Colin Walters 2011-02-17 17:16:21 UTC
(In reply to comment #12)
> Having "--replace" control these behaviors feels wrong to me. There are use
> cases for --replace that don't involve jhbuild, running out of srcdir, etc.
> (Otherwise there'd be no "metacity --replace".)

I do agree with you (and Owen said the same).  But this is a big swamp; my goal here is basically to iterate on paring down the script.  I really want to avoid things like having the wrapper script try to respawn GNOME 2 itself and the like in the installed case, along with adding broken environment variables.  (The XDG_MENU_PREFIX we set breaks gnome-panel, but that's a harder bug).

> My theory for a split would be something like:

I like this approach.  My only uncertainty is about making src/gnome-shell and $(bindir)/gnome-shell different things; but I guess it'll only be a short term confusion.

But so, my series of patches here I think moves the script behavior closer to the above; it will make things easier to review for working towards your proposal.  So what do you think about applying these, and then going from there?
Comment 15 Owen Taylor 2011-02-17 18:42:17 UTC
Functions of gnome-shell script:

 1) Attaching to existing gnome-session if run without DISPLAY set
 2) Start dconf manually since dconf might be installed in a directory
    not found by D-Bus activation
 3) Use glxinfo to decide if we need to set LIBGL_ALWAYS_INDIRECT=1
    to force indirect rendering when TFP is not available with
    direct rendering (DRI1 at least)
 4) Set environment variables to enable runnning uninstalled with the
    correct JS and CSS file, etc.
 5) Set PATH/XDG_CONFIG_DIRS/XDG_DATA_DIRS/GSETTINGS_SCHEMA_DIR to make things 
    work if we are run from a non-system install
 6) Set GCONF_DEFAULT_SOURCE_PATH to allow for our jhbuild merged GConf setup
    (also done by jhbuild now when running under jhbuild)
 7) Set the NO_GAIL and NO_ATK environment variables
 8) Disable bug-buddy so we don't get hung up waiting for bug-buddy which 
    can't be mapped (GNOME_DISABLE_CRASH_DIALOG)
 9) Work around Ubuntu not having JS libs in search path
 10) Set GJS_DEBUG_OUTPUT and GJS_DEBUG_TOPICS to get the right level of logging
 11) Implement the --debug option
 12) Pass through the --replace and --sync options
 13) Run the shell in performance mode, collect, collate and possibly upload the results
 14) Restore system GNOME at exit
 15) --create-extension option
 16) --eval-file option
 17) and of coure: start mutter with the right plugin
 
So, we can roughly divide these into different portions:

Stuff needed for correct operation of gnome-shell:

  3 (though somewhat obsolete), 7, 8 (on some distros? was the gtk+ module ported to gtk3?), 9 (on some distros), 10, 12, 17

Stuff used to make a non-system built GNOME Shell work correctly

  2, 5, 6

Stuff useful for working on GNOME Sheld

  1, 4, 11, 14

Stuff that's just wedged in as a convenient place

 15, 16

Performance measurement framework

  13

It makes some sense to me to make $bindir/gnome-shell as "what the gnome-shell binary would do if we had libmutter" - but what does that mean? 

stuff needed for correct operation: needs to be there  (for items are still useful)

The stuff needed to make a non-system build of GNOME Shell work with system GNOME: really should be there if we care about such a non-system install of GNOME Shell. We can replace some of it with a jhbuild re-exec hack if we want, but well, we already figured out what we need to set, right? I don't see a point in calling this something different. Maybe we have a configure option we can set in the gnome-shell moduleset?

Stuff useful for working on gnome shell. Run uninstalled clearly only matters if you aren't installed - no point in having that in $bindir. The other three are somewhat useful for debugging problems with an installed GNOME Shell but not essential. They are essential for hacking on gnome shell.

--create-extension and --exec-file. Not sure anybody uses --exec-file, we could just drop it. --create-extension can certainly certainly be separate - I'm not sure I totally agree with it living in gnome-shell-extensions ... seems to be mixing what gnome-shell-extensions is about, but OK.

Perf framework - I want this to be available with gnome-shell without having to grab the sources. (A distro could subpackage it, I guess.) I don't really care if it's gnome-shell <perf arguments>  or gnome-shell-perf. It needs to share the restart-enclosing-session logic.

So, my idea of a plan:

 * Write two shell scripts in the source tree:

    gnome-shell-systembuild.in
    gnome-shell-localbuild.in

   one of which gets installed as $bindir/gnome-shell depending on a configure option. gnome-shell-systembuild.in is as close as possible replication of the gnome-shell binary, and execs gnome-shell at the end. We drop the support for LIBGL_ALWAYS_INDIRECT=1 since we don't want to write the glxinfo checks in shell this probably prevents us from running on VIA hardware and maybe some other similar stuff, but really the GNOME Shell on DRI1 experience is something we don't want to be responsible for. (there's also much sloppier implementations around in shell that could be stolen used for compiz I guess... But not doing the check shaves a few tends of millseconds at startup.) The gnome-shell-localbuild could use the current envvar settings (what I would probably do), or if it makes the person doing the work happier could be called gnome-shell-jhbuild and do some magic to run under jhbuild.

 * Have an uninstalled ~/src/run-gnome-shell script that adds the re-exec and hook-to-running-GNOME-Session, and run-uninstalled logic.

 * Have an installed gnome-shell-perf script.

 * Have a python module 'gnome_shell_launch.py' with the bits shared between the debugging launcher script and gnome-shell-perf

I think that's roughly a days work to do. If that's an agreed upon plan, I think we should just execute on it, and not do patches that are sort of going in vaguely the right direction.
Comment 16 Colin Walters 2011-02-17 19:07:08 UTC
(In reply to comment #15)
> Functions of gnome-shell script:

This is very useful =)

> I think that's roughly a days work to do. If that's an agreed upon plan, I
> think we should just execute on it, and not do patches that are sort of going
> in vaguely the right direction.

So you want one big "boil the oceans" patch which reworks all this code into separate pieces and e.g. drops the TFP stuff?

The GSETTINGS_SCHEMA_DIR thing was continually screwing me every time I logged out and logged back in on Fedora rawhide with system gnome-shell, so I just had to fix it.  Fixing the Python-pointlessly-in-waitpid() thing was just icing while I had the patient open.
Comment 17 Colin Walters 2011-02-17 19:08:42 UTC
(And for completeness, I don't really care about the dconf part of this patch series; totally fine with dropping it out.)
Comment 18 Owen Taylor 2011-02-17 19:15:29 UTC
(In reply to comment #16)
> (In reply to comment #15)
> > Functions of gnome-shell script:
> 
> This is very useful =)
> 
> > I think that's roughly a days work to do. If that's an agreed upon plan, I
> > think we should just execute on it, and not do patches that are sort of going
> > in vaguely the right direction.
> 
> So you want one big "boil the oceans" patch which reworks all this code into
> separate pieces and e.g. drops the TFP stuff?

Patch structure could be various ways. I could see split into:;

 - Remove TFP checks
 - Remove --exec-file
 - Rename gnome-shell to run-gnome-shell and don't install it, and add the two shell-script wrappers instead that get conditionally installed as ~/gnome-shell/bin
 - Refactor run-gnome-shell into a module, run-gnome-shell and gnome-shell-perf and install gnome-shell-perf and the module.

The first three could even be done without the last one - I don't mind if the ability to do shell perf installed is temporarily broken.

But what I don't want to see is some changes that set environment variables in a different way, and then some more changes that set environment variables in a different different way. Because if someone hits problems we're just going to be confused.

> The GSETTINGS_SCHEMA_DIR thing was continually screwing me every time I logged
> out and logged back in on Fedora rawhide with system gnome-shell, so I just had
> to fix it.  Fixing the Python-pointlessly-in-waitpid() thing was just icing
> while I had the patient open.

But if you don't mind me saying, you *didn't* fix it. You made some changes and now it does something a little different for that and a little different for a lot of other things. And basically, I don't want to evaluate the details of "a little different" - it's hard enough to think about the end state!
Comment 19 Colin Walters 2011-02-18 18:39:42 UTC
Created attachment 181256 [details] [review]
gnome-shell.in: Move extension creation to gnome-shell-extension-tool
Comment 20 Colin Walters 2011-02-18 18:40:06 UTC
Created attachment 181257 [details] [review]
gnome-shell.in: Drop TFP checks

These were only necessary for DRI1, and we just aren't going to
work well there anyways.
Comment 21 Colin Walters 2011-02-18 18:46:05 UTC
Created attachment 181259 [details] [review]
gnome-shell.in: Drop --eval-file option

Not really used, and makes the script smaller.  We could probably
put a sample script on the wiki instead.
Comment 22 Colin Walters 2011-02-18 18:46:45 UTC
Created attachment 181260 [details] [review]
Add a configure option --enable-jhbuild-wrapper-script

The current gnome-shell.in script has a huge amount of
unnecessary complexity for the installed, normal case.  Fix
this by adding a configure option (defaulting to false) that
installs a simple, obvious wrapper script around mutter.

We do change the gnome-shell build setup to pass this option
by default for jhbuild.
Comment 23 Colin Walters 2011-02-18 19:25:00 UTC
*** Bug 641724 has been marked as a duplicate of this bug. ***
Comment 24 Quentin "Sardem FF7" Glidic 2011-02-18 23:50:14 UTC
Created attachment 181288 [details]
Shell script for gnome-shell-extension-tool

(In reply to comment #19)
> Created an attachment (id=181256) [details] [review]
> gnome-shell.in: Move extension creation to gnome-shell-extension-tool

Maybe the gnome-shell-extension-tool could be a shell script to avoid using python where it is not needed.

I think the attached file could do the work.
Comment 25 Quentin "Sardem FF7" Glidic 2011-02-19 11:47:48 UTC
Created attachment 181311 [details]
Shell script for gnome-shell-extension-tool

It's better with valid JSON. Also fix the gnome-open path.
Comment 26 Colin Walters 2011-02-19 16:05:22 UTC
(In reply to comment #25)
> Created an attachment (id=181311) [details]
> Shell script for gnome-shell-extension-tool
> 
> It's better with valid JSON. Also fix the gnome-open path.

Personally I don't know all the details of "POSIX /bin/sh", but basically unless you do, I'd just use /bin/bash and avoid being screwed by Ubuntu /bin/sh=/bin/dash here.
Comment 27 Quentin "Sardem FF7" Glidic 2011-02-24 18:13:21 UTC
Created attachment 181856 [details]
Shell script for gnome-shell-extension-tool

(In reply to comment #26)
> Personally I don't know all the details of "POSIX /bin/sh", but basically
> unless you do, I'd just use /bin/bash and avoid being screwed by Ubuntu
> /bin/sh=/bin/dash here.

I don't know either, just wanted to avoid a bash dep, but I used some bash feature ( [[ instead of test/[ ).


Is the --create-extension option needed any more? I think it's useless as the tool only does that. Or maybe I should add some features so?
Comment 28 Dan Winship 2011-02-25 13:22:19 UTC
Comment on attachment 181256 [details] [review]
gnome-shell.in: Move extension creation to gnome-shell-extension-tool

>+parser.add_option("", "--create-extension", action="store_true",
>+                  help="Create a new GNOME Shell extension")

yeah, I don't think there's much point in keeping that flag. Otherwise good.
Comment 29 Dan Winship 2011-02-25 13:24:17 UTC
Comment on attachment 181257 [details] [review]
gnome-shell.in: Drop TFP checks

looks right
Comment 30 Dan Winship 2011-02-25 13:32:15 UTC
Comment on attachment 181260 [details] [review]
Add a configure option --enable-jhbuild-wrapper-script

>+src/gnome-shell-extension-tool

oops, that belongs in the first patch

>+if USE_JHBUILD_WRAPPER_SCRIPT
>+gnome-shell: gnome-shell-jhbuild Makefile
>+	cp $< $@.tmp && mv $@.tmp $@
>+else
>+gnome-shell: gnome-shell-installed Makefile

I don't think there's any reason to make these depend on Makefile.

>+++ b/src/gnome-shell-installed.in

If the ubuntu spidermonkey stuff belongs anywhere, it belongs here.


The rest all looks right... I think it would be better to have gnome-shell-jhbuild run gnome-shell-installed though, rather than duplicating its logic and giving us a chance to get them out of sync.
Comment 31 Florian Müllner 2011-02-25 15:08:30 UTC
Review of attachment 181256 [details] [review]:

::: src/gnome-shell.in
@@ -562,3 @@
                   help="Evaluate the contents of the given JavaScript file")
-parser.add_option("", "--create-extension", action="store_true",
-                  help="Create a new GNOME Shell extension")

Nitpick: if you remove the command line option, you should also update the gnome-shell man page (even though it's probably quite outdated already ...)
Comment 32 Colin Walters 2011-02-25 15:13:18 UTC
(In reply to comment #30)

Thanks for the review!

> >+if USE_JHBUILD_WRAPPER_SCRIPT
> >+gnome-shell: gnome-shell-jhbuild Makefile
> >+	cp $< $@.tmp && mv $@.tmp $@
> >+else
> >+gnome-shell: gnome-shell-installed Makefile
> 
> I don't think there's any reason to make these depend on Makefile.

I always do that for custom rules; it's similar to what automake does.
> 
> >+++ b/src/gnome-shell-installed.in
> 
> If the ubuntu spidermonkey stuff belongs anywhere, it belongs here.

Hmm...I was sort of hoping that wasn't a problem anymore.  Maybe we need to make it a configure option.

> The rest all looks right... I think it would be better to have
> gnome-shell-jhbuild run gnome-shell-installed though, rather than duplicating
> its logic and giving us a chance to get them out of sync.

I'll look at doing that.
Comment 33 Colin Walters 2011-02-25 20:57:57 UTC
Created attachment 181952 [details] [review]
updated

I moved the ubuntu hack to be a configure option.  Not tested!

Also, gnome-shell-uninstalled now is a wrapper for gnome-shell-installed.
Comment 34 Colin Walters 2011-02-25 20:58:30 UTC
Dan can you commit the patches you marked ACN? I'm travelling at the moment and don't have git access from my netbook.
Comment 35 Dan Winship 2011-02-28 21:53:40 UTC
Comment on attachment 181952 [details] [review]
updated

looks good
Comment 36 Colin Walters 2011-03-01 15:02:47 UTC
Attachment 181256 [details] pushed as 3916b59 - gnome-shell.in: Move extension creation to gnome-shell-extension-tool
Attachment 181257 [details] pushed as 9616b45 - gnome-shell.in: Drop TFP checks
Attachment 181259 [details] pushed as 70fbfea - gnome-shell.in: Drop --eval-file option