Skip to content

[fix] Uninstall services correctly #2071

Merged
merged 4 commits into from Jan 15, 2013

5 participants

@amet
amet commented Jan 13, 2013

this is an attempt to fix http://trac.xbmc.org/ticket/13620, works fine here

2 commits:
1) check if another instance of the service is running before starting it
2) stop the service before uninstalling it/deleting it
3) cosmetics, duplicate code
4) correctly handle service addon rollback process

@MartijnKaijser , @ronie if you could test please

@ronie
Team Kodi member
ronie commented Jan 13, 2013

looks good to me.

one remaining issue, if you enable a service addon (that's been disabled previously)
we don't start it.

@amet
amet commented Jan 14, 2013

@ronie was that issue introduced with this commits?

@MartijnKaijser
Team Kodi member

iirc it used to start on my current install.
Will test tonight.

@ronie
Team Kodi member
ronie commented Jan 14, 2013

@amet yes

@amet
amet commented Jan 14, 2013

@ronie @MartijnKaijser if you dont mind trying again, looks okay here

@davilla 2d95a2d doesnt need to go in, but its "nicer" if it does. its not really a fix

@davilla
davilla commented Jan 14, 2013

I'd rather things be complete and match mainline in commits.

@MartijnKaijser
Team Kodi member

hit the little green button

@ronie
Team Kodi member
ronie commented Jan 14, 2013

noooooooooooo!!!!!!

@amet
amet commented Jan 15, 2013

@ronie I just tested, even without these we cant roll back the service. so this going in wont be bad but it wont be complete, I am working on it

@ronie
Team Kodi member
ronie commented Jan 15, 2013

sure, fine with me.

hit it!

@amet
amet commented Jan 15, 2013

@ronie if you wouldn't mind ;) ^^^^

@MartijnKaijser
Team Kodi member

Second try. hit the button ;)

@ronie
Team Kodi member
ronie commented Jan 15, 2013

..0...go!

@amet amet merged commit 79e043b into xbmc:master Jan 15, 2013
@amet amet deleted the unknown repository branch Jan 15, 2013
@jmarshallnz

OnPreInstall() ?

amet replied Jan 22, 2013

of course, typo

@jmarshallnz
Team Kodi member

Can you explain why we uninstall in OnPreInstall() and not just when rolling back? I don't think you need to uninstall when updating as it will install over the top and c-pluff handles updating?

@amet
amet commented Jan 22, 2013

@jmarshallnz
you are talking about this one? -> https://github.com/xbmc/xbmc/pull/2071/files#L1R569

I know its not ideal, but to avoid bigger changes and to keep it safe I have decided to go this route. rollback and install both use CAddonInstaller::Get().InstallFromZip() so to keep it simple we just remove it and install a new one.

we have tested extensively and seen no ill effects, can you think of a problem with it?

EDIT: just reread your question, we have seen issues with not being able to install over it unless we remove it(hence the comment in amet/xbmc@199b753#L1R298 , but I decided to remove it in PreInstall() so that the state of it can get saved and reused later

@jmarshallnz
Team Kodi member

Which cases are these, and would they also apply to non-service add-ons? IIRC RemoveAddon() just tells c-pluff that it's not there - it doesn't actually delete it off disk, right?

i.e. why not remove before update in general instead?

@amet
amet commented Jan 22, 2013

it would not apply to non service addons as they would not be running.

removing before installation is not possible without more changes that I was not sure would be safe this late in RC stage.

previous flow was:
rollback -> disable-> install -> start

in which case the old one doesnt get stopped as it was marked disabled and new one gets started ... so we end up with 2 addons running

@jmarshallnz
Team Kodi member

I'm commenting only about the RemoveAddon() call that was moved. This should not have anything to do with whether an add-on is running or not, so shouldn't depend on add-on type, right?

@amet
amet commented Jan 23, 2013

@jmarshallnz if we RemoveAddon() for services we dont know if addon was enabled or not before rollback, we have no idea what to do with it after we installed a new one, enabled/disabled ... plus for service addons, we need to enable them to be able to uninstall them(not sure why that is) so even disabled addons need to be enabled and then removed. in OnPreInstall() I tracked the pre-rollback state of the addon and that gets used in postInstall().

@jmarshallnz
Team Kodi member

Does RemoveAddon() touch the addons database?

I don't mind if it's what's needed - It just seems to me that it if works for service add-ons then it'll work for all add-ons. i.e. no special case is needed in the addoninfo dialog - just do it all in OnPreInstall()

@amet
amet commented Jan 23, 2013

@jmarshallnz I get what you mean now, its possible that it would be ok but I was not sure and we didnt want it in frodo this late. this change only hits service addons and it fixes them without touching any other addons. If you think its safe, I am ok to change it for all.

@jmarshallnz
Team Kodi member

I wouldn't change it for Frodo - you made the correct decision there. But there's no reason now not to get to the bottom of the issue. It seems there's some questions floating about still:

  1. Why do service add-ons need to be enabled in order to be uninstalled/rollbacked/updated?
  2. Why don't other add-ons have the same restrictions (or do they and noone noticed?)
  3. Why does updating an add-on affect the disabled status? Should it? (e.g. disabled due to broken -> auto enabled when unbroken?)

If we get to the bottom of exactly what is going on, then it should apply across the board.

@amet
amet commented Jan 23, 2013
  1. not sure, they need to be enabled to get the AddonPtr so we can remove it. By design I think?
  2. issue with service addons is that if enabled, they are running in the background and we need to stop them in order to remove/install/rollback. in order to uninstall we need to have any addon enabled, but if we enable service addon it starts running, so we need to stop as well :) see how it just got complicated
  3. updating it affects the disabled status as per above 1 and 2, we need to enable it to install and we have to save the current state
@jmarshallnz

Mind commenting exactly what this is needed for? In particular, why do we enable the add-on just to remove it?

i.e. what was the problem that this was fixing? (I commented on it, but have no idea what it was for...)

@jmarshallnz
Team Kodi member

Found the PR in case you're looking for it @amet. Seems there was some outstanding issues with this that we never resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.