Bugzilla Bug 155993

As discussed on #gentoo-desktop earlier today, I'm attaching my proposed patch
for the gnome2.eclass to prevent it from touching anything in /var/db/pkg.

Created an attachment (id=102582) [details]
gnome2.eclass.diff

Proposed patch to gnome2.eclass, first draft.

Created an attachment (id=102585) [details]
gnome2.eclass.diff

A second pass at the patch. This just stores the data in env vars, which I'm
told should be preserved between phases.

As an additional note, my patch doesn't properly handle $ROOT at the moment,
but I'm not sure if the old version did, either. I'll need to look more at
gconf to be sure.

Also, these ebuilds use the gnome2.eclass and provide their own pkg_preinst
function. They will have to be modified to call gnome2_pkg_preinst:

  dev-libs/gmime/gmime-2.1.9-r2.ebuild
  gnome-base/gconf/gconf-2.14.0.ebuild
  media-tv/zapping/zapping-0.10_rc6.ebuild
  media-tv/zapping/zapping-0.9.6.ebuild

I'm not sure if this is a very outdated requirement, but shouldnt phases be
self-contained ? 

(In reply to comment #4)
> I'm not sure if this is a very outdated requirement, but shouldnt phases be
> self-contained ? 

In talking with a few portage devs, that doesn't seem to be a problem.
Environment vars are saved between phases.

Looks like this was fixed in a different way. Hoorah.

(In reply to comment #6)
> Looks like this was fixed in a different way. Hoorah.

Reopen, it's been just moved to gnome2-utils.eclass :=(

<snip>
local contents="${ROOT}var/db/pkg/*/${PN}-${PVR}/CONTENTS"
local contents="${ROOT}/var/db/pkg/*/${PN}-${PVR}/CONTENTS"
if ! grep -q "obj /usr/share/icons" ${ROOT}var/db/pkg/*/${PF}/CONTENTS
</snip>

Please fix gnome2-utils.eclass eclass as it is used by great many packages.
Current implementation doesn't even work with paludis as VDB files are not
present there during pkg_postrm phase, leaving icon cache in broken state.

I'll take a look at Mike's patch tomorrow and let them live for a day or two in
our experimental overlay. If I manage not to break anything, I'll commit them.

ETA: end of this week.

I'll post the updated patches here for review anyway.

Thanks

Mike, I haven't put your patch in my tree yet, but I want to make sure I'm
getting this right.

Your fix basically goes through the whole /etc/gconf schemas and reinstalls all
of them, right?

If that's the case then it's a regression compared to our current eclass which
only installs/uninstalls schemas brought over by the package. I don't know what
the speed penalty is for doing this (or if it even does something "bad", like
reset some saved prefs) but if there's a way to avoid this, I'd like to pursue
that.

@all,

Is there a way in pkg_postinst and pkg_prerm to get the list of files that have
been installed by portage (basically the content of VDB) ? I can't find
anything about it.

The icon cache fix looks right, I'll probably commit that as soon as I can.

Thanks :)

(In reply to comment #10)
> Mike, I haven't put your patch in my tree yet, but I want to make sure I'm
> getting this right.
> 
> Your fix basically goes through the whole /etc/gconf schemas and reinstalls all
> of them, right?

No, it should (at least if I remember right) just look at all the gconf stuff
that's about to be installed by the package (see the pushd "${D}"?) and save
those into that variable.

Right. So looking a bit more closely, 2 things come to mind (you'll have
guessed I'm still a bit new to advanced stuff) :
 - GNOME2_SCHEMAS is probably saved automatically in environment.bz2 otherwise
it'll be empty when you remove a given package, right?
 - I don't really get why you dropped the  root "/" in "export
GNOME2_SCHEMAS=etc/gconf/schemas/*schemas" only to add it later in "/${F}". Is
there a reason for this?

As a side not, I just saw we're not currently taking care of uninstalling gconf
schemas, it's nicely commented out...

Anyway, thanks for your explanations.

(In reply to comment #12)
>  - GNOME2_SCHEMAS is probably saved automatically in environment.bz2 otherwise
> it'll be empty when you remove a given package, right?

Yes it's saved in environment.bz2, but portage doesn't currently have support
for reloading old environments (bug 56407 and bug 56408).  Before pkg_prerm()
is called, the ebuild from /var/db/pkg is sourced and live eclasses from the
tree are used for inherits.

Created an attachment (id=113025) [details]
Non-working patch to fix gnome2_icon_cache_update()

Here's the part of Mike's patch that improves gnome2_icon_cache_update().
However it's not working. It looks as if "export GNOME2_HAS_ICONS" done in
pkg_preinst() is not picked up in gnome2_icon_cache_update() called from
pkg_postinst().

Zac, how is sourcing done between two separate steps of the ebuild process? Is
it normal is GNOME2_HAS_ICONS is always empty in pkg_postinst() ?

(In reply to comment #14)
> Zac, how is sourcing done between two separate steps of the ebuild process?

ebuild.sh sources ${T}/environment at the beginning of each phase (if it
exists).  After that it also sources the build and inherited eclasses (for
every single phase).

> Is it normal is GNOME2_HAS_ICONS is always empty in pkg_postinst() ?

If GNOME2_HAS_ICONS is set in pkg_preinst(), it should persist to
pkg_postinst() via ${T}/environment.  It may be helpful for you to put `set -x`
at the top of /usr/lib/portage/bin/ebuild.sh in order to trace how that
variable is getting lost.

Created an attachment (id=113208) [details]
Emerge log of app-misc/tomboy with "set -x"

Here's the emerge log, I'm not really sure what to look for.

All I can see is that the variable is indeed set in pkg_preinst() but never
appears beyond that point. Look for the string "moo", I have added extra echos
in my patch in pkg_postinst().

Thanks

Created an attachment (id=113209) [details]
Emerge log of app-misc/tomboy with "set -x"

Here's the emerge log, I'm not really sure what to look for.

All I can see is that the variable is indeed set in pkg_preinst() but never
appears beyond that point. Look for the string "moo", I have added extra echos
in my patch in pkg_postinst().

Thanks

That logfile seems to be corrupted, I'm just getting garbage here (before and
after uncomressing).

Created an attachment (id=113217) [details]
Emerge log of app-misc/tomboy with "set -x"

Should be ok this time ...

(In reply to comment #19)
> Created an attachment (id=113217) [edit] [details]
> Emerge log of app-misc/tomboy with "set -x"

Your log shows 2 calls to gnome2_icon_cache_update().  The first one, in the
prerm phase, looks like this:

+ gnome2_icon_cache_update
++ type -p gtk-update-icon-cache
+ local updater=/usr/bin/gtk-update-icon-cache
+ [[ ! -x /usr/bin/gtk-update-icon-cache ]]
+ [[ '' = \n\o ]]
+ ebegin 'Updating icons cache (moo:)'

It's normal for GNOME2_HAS_ICONS to be unset in the prerm phase (because
portage never reloads environment.bz2).

The second one in the postinst phase looks like this:

+ gnome2_icon_cache_update
++ type -p gtk-update-icon-cache
+ local updater=/usr/bin/gtk-update-icon-cache
+ [[ ! -x /usr/bin/gtk-update-icon-cache ]]
+ [[ no = \n\o ]]
+ debug-print 'No items to update'

It appears that GNOME2_HAS_ICONS=no has correctly persisted from the preinst
phase (note that the [[ no = \n\o ]] test succeeds, leading to early return
from gnome2_icon_cache_update().

(In reply to comment #20)
> Your log shows 2 calls to gnome2_icon_cache_update().  The first one, in the
> prerm phase, looks like this:

The first one was actually in the postrm phase, but it would be normal for
GNOME2_HAS_ICONS to be unset there, as well.

Ok, so basically, the patch works, but doesn't do what I'd like it to do.
That's some progress though :)

Now, there's one thing this patch and the gconf part both need : how does the
eclass know which files are about to be uninstalled ?

I really don't feel like duplicating /var/db with our own list of icons and
schemas to be added/removed. Can portage do this for us?

@gnome: basically the situation is this :

With the current eclass, we are clever when updating the icon cache by looking
at CONTENTS to see if the package actually installs any icons.

With the new eclass, we can do this for the install phase (by looking at ${D}
instead of the CONTENTS file) but this won't work for the uninstall phase which
currently always regenerates the cache even if no icons were removed.

Ideas more than welcome, as I am not a portage guru :)

Thanks

It seems like the most elegant way to save that information would be via
environment.bz2, but that has to wait for bugs 56407 and 56408 to get fixed. 
That will solve bug 46223 too.

At the moment, that leaves 2 options:

1) Manually install a text file containing the needed icon info.
2) Regenerate the cache regardless of whether or not it's necessary.

If the performance hit isn't unbearable, 2 is probably the best option.  If
performance optimization is absolutely necessary, then use 1.

I would personally have gone with option #2 as it's easier to deal with, but
unfortunately the perf hit is too big when doing several emerge at a time (and
it's not a gnome problem alone anymore, as xfce ebuilds are using this too).

Anyway, where would be the best place for the eclass to put such a text file?
/var/db/gnome2-{icons,schemas}/* ?

Thanks for all help with this :)

I'd rather put it in /usr/share/???/$PVR as this isn't really variable data,
just don't know what value to use for ???

Actually, maybe it's not necessary to install the information.  Can't you
create a list of files in the prerm phase, and then compare it to a list of
files in the postrm phase?  For example:

pkg_prerm() {
 GNOME_ICONS_PRERM=$(find "${ROOT}"/usr/share/icons)
}

pkg_postrm() {
 if [ "${GNOME_ICONS_PRERM}" != "$(find "${ROOT}"/usr/share/icons)" ] ; then
  # regen icons cache
 fi
}

The environment variables would persist from prerm to postrm via
${T}/environment (temporary files in ${T} could also be used to store the
information, but it's the same principle).

It could work for the icon cache, but it wouldn't work with the gconf schemas
as the gconf command needs to be run in pkg_prerm().

But it can surely wait for bugs 56407 and 56408 to be fixed as gconf
unregistering was disabled 2 years ago anyway.

I'm a bit worried about performance though. On my system, I almost have 18,000
files and directories in /usr/share/icons. It does take a few seconds on a cold
cache to list them all.

I'll give it a try though, it's certainly better than running the cache updater
all the time.

Mind if I make this bug depend on bug 56408, for a clean and sustainable fix?

Thanks

Created an attachment (id=115655) [details]
Fix gnome2_icon_cache_update

I finally had some time to work on this again.

All logic concerning whether icon_cache_update should be called has been moved
to gnome2.eclass. gnome2_icon_cache_update is now really dumb and updates the
cache when called.

QA, gnome herd, xfce folks, and others, please review this patch. I'd like to
commit this before gnome 2.18 :) Thanks

PS, more to come wrt gconf stuff

One thing though, I'm using "mkdir -p" in the patch, and I'm pretty sure that's
a big no-no on bsd if I'm not mistaken.

What's the proper way to do the same? I couldn't find anything relating to that
in the devmanual.

dodir, presumably.  You may have to do it in src_install, tho; it should still
be there in pkg_postinst.

I've modified the icon cache scanning to not check the VDB.  Instead, it always
updates the cache, but doesn't force re-creation like it was doing before. 
We've tested this for a while, and it works correctly, and works fine for
slower systems (tested on an efika).

(In reply to comment #31)
> I've modified the icon cache scanning to not check the VDB.  Instead, it always
> updates the cache, but doesn't force re-creation like it was doing before. 
> We've tested this for a while, and it works correctly, and works fine for
> slower systems (tested on an efika).
> 

It seems to be fine on my system too but users are reporting they don't get all
icons now.

11:59 < b00> hey, how about to add icon to Xarchivier to the menu ?
11:59 < b00> i think , you could use the file icon, that is currently used
12:18 <+jakub> hmm? it's displayed in menu just fine here...
12:20 <+drac> xfce-extra/xarchiver
(/usr/share/icons/hicolor/48x48/apps/xarchiver.png)
12:20 <+drac> which is Icon=xarchiver in .desktop file
12:22 <+nightmorph> there's a bug or two going around on the forums that users
are missing several icons
12:22 <+nightmorph> and they had to emerge hicolor-theme to get that
icon-update-cache command to be run

but xarchiver runs the icon cache update from gnome2-utils..

IIRC (we'd talked about it with Daniel) gnome-menu doesn't use the icon cache
for application icons. Globally it seems the icon cache is only used for theme
icons (menu bars, menus, dialogs, ...)

Maybe this user could try rebuilding the cache with the "-f" flag. Something
worth looking into.

(In reply to comment #33)
> IIRC (we'd talked about it with Daniel) gnome-menu doesn't use the icon cache
> for application icons. Globally it seems the icon cache is only used for theme
> icons (menu bars, menus, dialogs, ...)
> 
> Maybe this user could try rebuilding the cache with the "-f" flag. Something
> worth looking into.

http://forums.gentoo.org/viewtopic-t-556161-highlight-xfce.html

Readding -f to .eclass fixes this issue, but then we are back to accessing VDB
CONTENTS to avoid damage in performance because it's the culprit why it takes
so long to update the cache.

back to square one with 2 options :

1) save in a file the paths of added icons
or
2) save those paths in a env variable

1) sucks because we pollute the users file system
2) sucks because portage can't do it yet

Going with number 1) can then lead us to 2) with a nice migration path when
portage actually supports it.

We have a third option: make incremental icon updates work properly...  I'm
looking into patching gtk-update-icon-cache to check more stats than the
top-level directory.  It should be easy.

Created an attachment (id=117982) [details]
patch gtk-update-icon-cache to be sane

Here's a patch to gtk-update-icon-cache to add a -s parameter.  This parameter
causes it to scan subdirs of subdirs of the themedir, rather than just the
themedir, when determining if it should update the cache.  The eclass needs to
be modified to add -s to the call, of course.  I'll send this upstream.

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

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

So...we are back to square one on this one.  What about the patch that dang
attached here?  Does that make it so you guys don't need to access VDB?

In portage-2.1.4 bug 56408 is fixed so you can use environment variables from
environment.bz2 in pkg_prerm and pkg_postrm.

Zac, should we wait for 2.1.4 or greater to go stable before moving on this? If
so, do you have a time frame for this?

And thanks for fixing portage :)

I plan to release 2.1.4 final in about 1 week, and if every thing goes well
then we can probably have it marked stable about 3 weeks from now.

(In reply to comment #43)
> I plan to release 2.1.4 final in about 1 week, and if every thing goes well
> then we can probably have it marked stable about 3 weeks from now.
> 

So, we are past that mark now :)  Any idea when we might see a 2.1.4 final,
just so I know when to start bugging people again?

I've just added 2.1.4 final to the tree. I can ask for it to be marked stable
in about 2 weeks as long as no major regressions are discovered before then.

Thanks Zac.

I'll post an updated ebuild for review here when I get to it. I'd like to close
this once and for all :)

Created an attachment (id=142783) [details]
gnome2.eclass.patch

Alright! I've finally found some time to work on this. This patch only takes
care of the GConf schemas and doesn't not touch the icon cache regen stuff. I
want to make sure this works 100% before moving on.

Pros of new patch:
- works for initial install, updates and rebuild
- drops VDB access (at least I think it does :) )

Cons of new patch:
- does not work with uninstall

And that's what I was talking about with genone yesterday. He told me that env
saving didn't actually work from pkg_preinst() : 

<genone> remi`: back to your problem, we have to do the env saving in
dyn_install (otherwise it wouldn't work for binpkgs), but maybe we can update
it after preinst, I'll check with Zac if I see him (if you see him you should
ask too)

Comments greatly appreciated :)

Created an attachment (id=142944) [details]
make portage update environment.bz2 after pkg_postinst()

I'll do a portage-2.1.4.2 release later today with this patch.

Thanks Zac.

Just a quick question, I see in your patch that you save the env vars during
the postinst phase. I'm trying to save them in preinst. Won't that be a
problem? (I've seen so far that my envs are carried from preinst to postinst,
so I'm thinking it should work) Anyway, just wondering :)

I'll finish my patches as soon as .1 hits the tree.

By updating environment.bz2 after postinst it should give us continuous
coverage of all phases from pkg_setup all the way to pkg_postrm. If there are
any phases left uncovered then that would be a bug.

Newer Portage is stable on all arches...closing

(In reply to comment #51)
> Newer Portage is stable on all arches...closing

Well that doesn't magically fix this bug. 

I've committed the gconf part of the patch to the gnome overlay for broader
testing. I'll try to get to the gtk icon part later (probably after FOSDEM).
Any additional help is more than welcome :)

Final eclasses in the gnome overlay. Please review :)

http://git.overlays.gentoo.org/gitweb/?p=proj/gnome.git;a=tree;f=eclass;hb=HEAD

Thanks

Alright, the new eclasses are in portage \o/ yay!

Here's a list of the ebuilds I looked at based on scripts I had gotten through
-dev.

app-i18n/scim-pinyin            N/A
dev-util/devhelp                N/A
games-arcade/blobwars           done
games-board/gnono               done
games-mud/gnome-mud             done
games-puzzle/gtetrinet          done (but ebuild needs review, it's
weird-looking)
gnome-base/gconf                N/A (doesn't install any icons)
gnome-extra/drwright            N/A
gnome-extra/gnome-games         done (ebuilds need review, pkg_preinst is
weird, doesn't use games_pkg_preinst...)
gnome-extra/libgsf              done (ebuilds need review, are old preserve_lib
still needed?)
media-gfx/comix                 N/A
media-video/jubler              not done (too many inherits)
media-video/vlc                 not done (ebuild is huuuge)
net-im/pidgin                   N/A
sci-astronomy/celestia          N/A

@Java Herd, could you guys check out jubler?

@Samuli, I've modified the xfce44 eclass, but I haven't checked the ebuilds
that used that eclass. Maybe I'll get to it later on, I just wanted to let you
know what I had done.

We're getting close :)

Samuli told me that the Xfce ebuilds were all fine. So that's done.

@Java Herd, only jubler left :)

(In reply to comment #56)
> Samuli told me that the Xfce ebuilds were all fine. So that's done.
> 
> @Java Herd, only jubler left :)
> 

I have no idea what we would be supposed to do but as the API should be the
same and jubler seems to work I presume we are good.

Jubler shouldn't probably use the gnome2 eclass, so either way it looks
unaffected.

Closing FIXED \o/ !

Thanks