Created attachment 38590 [details] [review]
Check /bin/bzip2

Uh, well, yeah, I guess that would stop the error message. But don't think it'll
actually make it work...

$ grep -n /usr/bin/bzip2 gnome-theme-installer.c
179:    command = g_strdup_printf ("sh -c 'cd \"%s\"; /usr/bin/bzip2 -d -c <
\"%s\" | /bin/tar xf - '",
218:            command = g_strdup_printf ("sh -c '/usr/bin/bzip2 -d -c < \"%s\"
| /bin/tar ft - | head -1'",
257:            if (!g_file_test ("/usr/bin/bzip2", G_FILE_TEST_EXISTS)) {


 

I've submitted at patch for the Gentoo packages at:
http://bugs.gentoo.org/show_bug.cgi?id=84997

*** Bug 170150 has been marked as a duplicate of this bug. ***

*** Bug 301788 has been marked as a duplicate of this bug. ***

as mentionned by comment #2, that doesn't fix the issue

the proper solution would probably be to use g_find_program_in_path. The same
should also be done for tar.

Created attachment 46910 [details] [review]
fix hardcoded paths

Here is the proposed fix that's going into Gentoo.  It replaces all the
hardcoded paths with g_find_program_in_path() calls.

I'm not the maintainer, but from a quick look the patch looks ok for a quick fix
of this bug... however the whole code in there seems quite scary to me... for
instance why the utilities have to be located more than once? In fact why are
the commands run twice at all (once in the function and once in the idle
handlers)? Even worse those are not idle handlers anymore and the function name
and the comments are just misleading!

I can also spot various other issues... like the use of the access() function to
test the exitence of directories which is racy and insecure by nature...

David, by looking at the cvs commit logs it looks like you were the one
reworking this code, can you fix things up a bit?

Yes, I noticed some problems with the code, and had some others pointed out to
me when I got it reviewed by other Gentoo Gnome herd members, but I think that
kind of extensive rework is beyond the scope of a bugfix for 2.10.1.  I'm going
to look into general cleanup against HEAD, but someone more familiar with the
code could probably do a better job.

I'm working on a patch to clean up the file a bit. Let me some time to come with
something useful.

Created attachment 47166 [details] [review]
Set the correct paths and cleanup the process

Paolo: There are two command but one of them not uncompress the file (it have a
| head -1), it's only to see the name of the dir. I will try to fix this too and
get this info from the uncompressed dir.

ok, thanks for looking into this... just glanced at the patch, but it doesn't
seem to adress all of my points: 

- access() is still there, really don't use it
- transfer_done_targz_idle_cb is not an idle handler at all afaics so it must be
renamed. Beside bzip_idle_cb is almost the same function... can't they be
combined? Also remove the misleading stale comments about idle handlers
- I seem to recall that there are cases where the code tries to write in
~/.themes without making sure that the directory exists
- don't use "\n" inside the error messages, use the primary and secondary text
labels
- maybe other stuff (I'm going by memory, haven't checked the code

Created attachment 47243 [details] [review]
More work in the patch

Paolo: I have comined the two funtions, use the primary and secondary text for
the dialog and remove some comments.

The access call was there before my patch ;) but, what function we must use
instead?

You just try to open it and handle the error if it doesn't exits or cannot be
opened. The problem with access() is that it is racy:

- program 1 calls access() of foo
- program 1 is put to sleep by the kernel
- program 2 removes foo
- program 1 is screwed because it has wrong information on the current state of foo

Created attachment 47267 [details] [review]
Remove access func. for gnome-vfs calls

The access call it's checking the permission of .themes directory. I have
replaced this call with gnome-vfs get info call.

I'm trying to detect errors after g_spawn_command_line_sync but I don't know how
because the command returns 0 in status var when there are a problem with
permission for example.

Changing status

Hey, is that so hard? It's december already. I have it in /bin/bzip2 and it can't find it. The funny part is, tar and gzip are there too and it has no problems with tar.gz themes. Gnome 2.12

*** Bug 337269 has been marked as a duplicate of this bug. ***

Was there actually any issue with patch #46910 (fix hardcoded paths)? If not then I will apply it.

I think the problem the gnome-vfs patch is trying to address is a separate issue.

Thomas, that change looks fine from a quick glance, feel free to commit if you try it with one archive of each sort and it still works fine with the change

2006-04-10  Thomas Wood  <thos@gnome.org>

        * gnome-theme-installer.c: (transfer_done_targz_idle_cb),
        (transfer_done_tarbz2_idle_cb), (transfer_done_cb): Applied patch from
        bug 170058 - bzip2 location hardcoded

this bug is far from fixed... that code is crappy to say the least and the patch that was committed is a bandaid.

Closing this bug is just sweeping dirt under the carpet, throwing away many useful comments and remarks accumulated in this bug and also throwing away some patches that even if not 100% ready are a step forward.

I agree with comment #9 that the theme install code is a mess, and needs clearing up. However, that was not the purpose of this bug report. The issue this bug was opened for was to fix the hardcoded location of the bzip2 program. The comitted patch did this (and also for gzip and tar), so the issue the bug was opened for has been resolved. Please feel free to open another bug report to address the state of the theme install code.