Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

skin migration and addon compatibility checking #10335

Merged
merged 14 commits into from Sep 14, 2016

Conversation

@tamland
Copy link
Member

commented Aug 24, 2016

This adds compatibility checks for addons on startup, replacing the weird repository workarounds current in place. It will now checks all installed addons and disable them/notify.

Similarly, the skin (special treatment because it breaks every release, and it's critical that it works) will be auto-updated, or if that fails fallback to default skin, preventing incompatible skins from being loaded.

@ronie

This comment has been minimized.

Copy link
Member

commented Aug 24, 2016

nice one, thx!

@ronie ronie closed this Aug 24, 2016
@ronie ronie reopened this Aug 24, 2016
@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 24, 2016

awesome. Very much appreciated :)
Wanted to try on Windows but fails to build http://jenkins.kodi.tv/job/WIN-32/9775/console

@tamland tamland force-pushed the tamland:skinmigration branch from 74a94d2 to ec3a0d9 Aug 24, 2016
@tamland

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2016

jenkins build this please

@tamland

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2016

builds now;)

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 25, 2016

@tamland it seems indeed to mark skins incompatible and switches to default skin. However is does this check before it fetches data from the newer repo which does compatible skins/addons. So now all current installed versions are marked "disabled" while it should probably first fetch the updates before just hitting everything disabled and the users needs to get everything sorted again. Additionally users are now still able to enable "incompatible" add-ons again while this should not be possible?

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 25, 2016

Seems there's a small additional issue. Tvtunes (as example) is marked incompatible but it's not auto disabled while it should be.
screenshot012

@tamland

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2016

Hm, I think we have to make a choice to either accept that it doesn't automatically updates, or don't do any disabling, only warn about it. Auto updating everything before startup is not an option; this can take far too long time.

What you're seeing with tvtunes is expected. There's an update for it that is marked broken, but it's not incompatible or have unmet dependencies in any way. These are completely different things previously mushed together.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 25, 2016

Well if it's marked broken it should certainly be disabled. To me incompatible and broken are the same and disabled is the way to go without option to re-enable unless a fixed version is found.

Leaving user with a warning and don't disable doesn't sound like an option cause that will leave it in broken state. Forcing the user to go and hunt down Addons that do have working update but the Addon is disabled isn't a very good option as well.

Edit:
Preferred would certainly be update all available before startup. Not sure what should be done if usr has auto update Addons off.
Should go in same category as database upgrades and a "busy upgrading" splash with some indication it's not stuck would be ideal

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

I've added this PR in my test builds. In my test builds I include the Kodi Game repository.

This repository is being reported as incompatible:
s
which is fine, as it is incompatible.

However, this popup is appearing every time Kodi is started - is this expected behaviour?

@ronie

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

i've tested this and it all appears to work fine on my end.

kodi waits at the splash screen while the selected skin is being updated.
after switching to the home screen, all the other addons get updated.

nothing ends up disabled at my end, except the re-touched skin, which is correct.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

Seems my auto update setting was off for whatever reason, Sorry for that confusion.
Tried again and now it crashes on upgrade
thread log http://pastebin.com/1B0hWHgG
debuglog http://pastebin.com/yRhfSgrw

@ronie

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

yeah, i got the crashes as well.
http://paste.ubuntu.com/23095159/

the crashlog seems to indicate the skin helper addon is causing issues.
after deleting the addon and all skins that depends on it, the upgrade went fine.

i think what happens is the skin helper addon is started (as a service) before the skin has loaded, because kodi is still busy downloading the krypton version of the skin you're using.
the helper addon tries to evaluate a skin setting (Skin.HasSetting(SkinHelper.EnableAnimatedPosters)), but i assume these settings are not available as the skin hasn't been loaded yet?

result: crash-boom-bang.

@ronie

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

edit: it's not the addon itself thats evaluating the skin setting...

it's kodi reading the context menu section of the skin helper addon.xml file
https://github.com/xbmc/repo-scripts/blob/helix/script.skin.helper.service/addon.xml#L16

@tamland tamland force-pushed the tamland:skinmigration branch 2 times, most recently from 9f09b75 to ba1c029 Aug 28, 2016
@tamland

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2016

jenkins build this please

@tamland

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2016

Ok, the amount of unrelated fixes is piling up!, but as far as I can test it doesn't crash now.

@tamland

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2016

@MilhouseVH I can only guess but probably it's reenabled or cant be disabled for some reason (eg it's a required system addon).

Well if it's marked broken it should certainly be disabled. To me incompatible and broken are the same and disabled is the way to go without option to re-enable unless a fixed version is found.

The 'marked broken' stuff is not touched here. It should still show popups (all 50 of them). It just wont be part of this startup process because logically they are completely different things.

Leaving user with a warning and don't disable doesn't sound like an option cause that will leave it in broken state. Forcing the user to go and hunt down Addons that do have working update but the Addon is disabled isn't a very good option as well.

Edit:
Preferred would certainly be update all available before startup. Not sure what should be done if usr has auto update Addons off.

And if user have a lot of skins and addons installed, a slow connection or the mirrors are slow? It's not hard to imagine an update like that is going to take an hour. That's just an unacceptable wait time.

I think we need to pick our battles here. If an upgrade require some manual intervention, so be it. Any sort of migration we do is always going to be an 80% solution. I'd rather see the 'hunt down addons' and re-enabled process made easier and do the safe thing (it already lists all problem addons so there should be no surprises to the user).

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2016

The repository.kodi.game add-on is "built in" to the image (so not user installed) and is also present in the addons-manifest.xml - the disabled button is greyed out so doesn't appear possible to disable. If we can't avoid the repeated "incompatible" messages at startup then the best short-term option would be to drop this repository from LibreELEC (unless whoever maintains it can address the incompatibility, is this @garbear?)

Edit: Add note to clarify inability to disable this add-on. Also to add that the best option would be to ship a compatible add-on, we included this repo to facilitate testing but clearly things aren't progressing in that direction so dropping the repo will be best, at least in the short-term. It's not this PR's fault if we ship a broken add-on. I'll drop it from my own builds starting from tonight, and if repository.kodi.game remains broken then I guess we'll have no option but to eventually drop it from LE master.

@tamland tamland force-pushed the tamland:skinmigration branch from ba1c029 to 14e864d Aug 28, 2016
@tamland

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2016

Changed it to not include failed ones in the dialog, just log a warning. Should be ok in this case.

@tamland tamland force-pushed the tamland:skinmigration branch from 6a2944e to 5573eb9 Sep 13, 2016
@tamland

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2016

jenkins build this please

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Sep 13, 2016

Looks like non relevant fail on linux. Just to be sure...
jenkins build this please

@tamland

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2016

So, beta 2 or beta 3?

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

I don't see beta2 happening this week and we have several days of devcon. I'd say merge :)
Thx

@tamland tamland merged commit 8836740 into xbmc:master Sep 14, 2016
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
jenkins.kodi.tv You are a failure. Fix the code and try again......
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@bkuhls

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2016

Afaics this commit causes a segfault when starting Kodi.
This is the last working commit: fcce26e
This is the first broken commit: 8836740

The segfault also occurs when Kodi is started with an empty home directory so I guess the choice of addons installed does not play a role here.

System is Linux, kernel 3.18.36 i686, uClibc, gcc 4.9.2, cross-compiled using buildroot,
kodi.log was not created, so I can only provide a gdb backtrace:

Program received signal SIGSEGV, Segmentation fault.
0xb5b197ab in std::string::_Rep::_M_grab(std::allocator<char> const&, std::allocator<char> const&) ()
   from /usr/lib/libstdc++.so.6
(gdb) bt full
#0  0xb5b197ab in std::string::_Rep::_M_grab(std::allocator<char> const&, std::allocator<char> const&) ()
   from /usr/lib/libstdc++.so.6
No symbol table info available.
#1  0xb5b197f8 in std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(std::string const&) () from /usr/lib/libstdc++.so.6
No symbol table info available.
#2  0x082e7e09 in pair<ADDON::TYPE, void> (__x=<optimized out>, __y=..., this=0xbffffbbc)
    at /home/fli4l/.fbr/fbr-unknown-custom-x86/buildroot/output/host/usr/i586-buildroot-linux-uclibc/include/c++/4.9.2/bits/stl_pair.h:134
No locals.
#3  __static_initialization_and_destruction_0 (__priority=65535, __initialize_p=1)
    at AddonSystemSettings.cpp:66
No locals.
#4  _GLOBAL__sub_I_AddonSystemSettings.cpp(void) () at AddonSystemSettings.cpp:170
No locals.
#5  0x08e4c287 in __do_global_ctors_aux ()
No symbol table info available.
#6  0x082936f1 in _init ()
No symbol table info available.
#7  0xb5a5c4eb in __uClibc_main () from /lib/libc.so.0
No symbol table info available.
#8  0x08306c72 in _start () at libc/sysdeps/linux/i386/crt1.S:128
No locals.
@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Sep 17, 2016

@tamland
Probably same issue on Android
http://forum.kodi.tv/showthread.php?tid=290934

Same happens on my phone/tablet.

@koying

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2016

http://flightgear-devel.narkive.com/ytqWx9LM/static-array-of-std-string-valid
Basically, might be that the CSettings::SETTING_* std::strings are not constructed, yet

Probably compiler dependent.

@Paxxi

This comment has been minimized.

Copy link
Member

commented Sep 17, 2016

Could very well be, just move it into the class as a non-static member variable and we should be fine for beta

@tamland

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2016

@tamland tamland deleted the tamland:skinmigration branch Sep 17, 2016
MartijnKaijser added a commit that referenced this pull request Sep 17, 2016
maybe fix android crash after #10335
@BigNoid

This comment has been minimized.

Copy link
Member

commented Sep 24, 2016

@tamland with this PR there's something wrong with storing the skin settings. The log is not really interesting but you can easily reproduce by disabling a main menu item in Estuary and than doing a skin reload on the Home screen. Each reload hides/shows the disabled menu item.

@BigNoid

This comment has been minimized.

Copy link
Member

commented Oct 10, 2016

ping @tamland
The alternating skin settings on reloadskin() is quite annoying for skin development. Can you take a look please?

@Memphiz

This comment has been minimized.

Copy link
Member

commented on xbmc/addons/AddonSystemSettings.cpp in e78e13d Oct 12, 2016

For me the splash hangs on "Add-on migration in progress - please wait" in latest nightlies. Realised that on both my mac systems after starting a nightly build (didn't have a current nightly for maybe 2 month running on those). I feel that it only can be the await. Is there any good way i should debug this or can you already see a spot where that event is not set?

This comment has been minimized.

Copy link
Member Author

replied Oct 15, 2016

Not sure how that could deadlock. The only place it's reset is here, and then it's set again here. I.e this method can only 'block' between those two calls.

One easy way to check this is to just run it in gdb, pause, and look at the stack where the threads are waiting.

This comment has been minimized.

Copy link
Member

replied Oct 15, 2016

I would have done this already of course - but this only happens once. After the next restart its ok. So it seems its only hanging when something is to do for migration. Will have to wait for a while i guess until it migrates something again - and then i need to stop the debugger indeed.

This comment has been minimized.

Copy link
Member Author

replied Oct 15, 2016

Ok, then edit out the return 5 lines above and it should always run the migration code on statup. Or pick some installed addon and change any xbmc.*/kodi.* dependency to a version that can never be satisfied.

This comment has been minimized.

Copy link
Member

replied Oct 15, 2016

both approaches only show the migration text in the splash screen for a short time ... the second approach does disable the incompatible addon then ...

This comment has been minimized.

Copy link
Member Author

replied Oct 17, 2016

clear the repo table in the db, add some sleeps, wait 24 hours for a repo update. I dont have any more suggestions..

are you sure it's not just slow? it can take a very, very long time to install 2 months worth of updates..

This comment has been minimized.

Copy link
Member

replied Oct 17, 2016

Or change the repo timer code to much less :)

This comment has been minimized.

Copy link
Member

replied Oct 26, 2016

I think i have to retry to reproduce it - it is an issue in the end imo

http://forum.kodi.tv/showthread.php?tid=294880

This comment has been minimized.

Copy link
Member Author

replied Oct 29, 2016

Ok, I can confirm there is a deadlock, but it seem to in the addon installer (who figured!) between jobs waiting on dependencies that never completes.

This comment has been minimized.

Copy link
Member Author

replied Oct 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.