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
Kodi #3277
Kodi #3277
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this long-overdue update :)
targets/kodi
Outdated
|
||
### Append to prepare.sh: | ||
if [ "$DISTRO" = 'debian' ]; then | ||
echo "deb http://http.debian.net/debian jessie-backports main" | sudo tee /etc/apt/sources.list.d/kodi.list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prepare already runs as root, so you can just use >
to write the file.
targets/kodi
Outdated
. "${TARGETSDIR:="$PWD"}/common" | ||
|
||
### Append to prepare.sh: | ||
if [ "$DISTRO" = 'debian' ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be using jessie-backports in sid, should we? The right condition would then be if release -le jessie; then
, which will apply only to debian releases jessie or earlier.
targets/kodi
Outdated
fi | ||
|
||
if [ "$DISTRO" = 'ubuntu' ]; then | ||
sudo apt-get install -y software-properties-common |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you never need sudo in a target; you already have it
targets/kodi
Outdated
fi | ||
|
||
if [ "$DISTRO" = 'ubuntu' ]; then | ||
sudo apt-get install -y software-properties-common |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer to this old xfce target for how to pull in PPAs. We don't want to unnecessarily install things.
targets/kodi
Outdated
fi | ||
|
||
if [ "$DISTRO" = 'kali' ]; then | ||
error 99 "Kodi not supported on Kali Linux" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check should be done in the pre-prepare area, before you've gone through most of the crouton install. See the ever-shrinking unity target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not certain what " if [ "${TARGETNOINSTALL:-c}" = 'c' ]; then " means..
Here is what i put at the top:
if [ "${TARGETNOINSTALL:-c}" = 'c' ]; then
if [ "$DISTRO" = 'kali' ]; then
error 99 "Kodi not supported on Kali Linux."
fi
fi
Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! It's not well-documented.
targets/kodi
Outdated
error 99 "Kodi not supported on Kali Linux" | ||
fi | ||
|
||
sudo apt-get update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should only be run on distros that add the repository (e.g., put it in the if statements above). Also, it seems apt-get update should be || true
'd in case the user has broken repos.
targets/kodi
Outdated
|
||
keyboardxmldir="$HOME/.kodi/userdata/keymaps" | ||
# Do not install if user is root, or $HOME does not exist | ||
if [ "`id -u`" -ne 0 -a -d "$HOME" -a ! -e "$keyboardxmldir/keyboard.xml" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be "$(id -u)"
targets/xbmc
Outdated
@@ -1,35 +0,0 @@ | |||
#!/bin/sh -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can outright delete this target; we need to provide a migration path.
One quick-and-dirty solution would be to make xbmc a minimal target like x11, with an unconditional REQUIRES=kodi
, and add PROVIDES='xbmc'
to the kodi target (like in xorg). Perhaps even leave DESCRIPTION blank and update installer/main.sh to avoid printing out any targets that have a blank DESCRIPTION for -t list
.
The best solution would be to make xbmc a symlink to kodi, and then add some smarts to installer/main.sh to interpret a symlinked target as if it were the above. This would be a little tricky (but not too hard) because the script would have to automatically populate PROVIDES by detecting any target symlinks that point to the selected target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created the minimal target, but i'm unsure what exactly to add to installer/main.sh.
I assume it would be around line 306, but i'm not familiar with the syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like in the help case, we set TARGETS='help'
, on line 309 we set TARGETNOINSTALL='y'
and then source the target file. So it looks like the right tweak would be actually in the target common file, line 29, where it prints out the help text. If DESCRIPTION is blank, we don't want to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm sorry, this went right over my head... please dumb it down a bit ;)
I believe this is the only remaining item to resolve before this is good to merge. Thanks a ton for your help! :)
Once merged, i'll reach out to the kodi guys and see if they'll feature it in their blog ;)
I also plan to make a wiki page that talks about it a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap the common target lines 30 through 34 in a if [ -n "$DESCRIPTION" ]
block :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Thanks for the suggestions! I'll work through the list and resubmit when i've got them resolved. |
- Fixed debian source to only add for jessie and older - switched ubuntu to pull ppa source and key directly. this avoids requiring "software-properties-common" - moved kali error to pre-prepare area - moved 'apt-get update' to distro specific area to avoid running unnecessarily.
minimal target to allow migration path from xbmc to kodi.
Ok @dnschneid, I've made the requested changes. I think everything is solid except for my questions regarding the kali linux and the xbmc migration stuff. I haven't tested yet, as my chromebook is at home. I will test it tonight and report back. Thanks for your help getting this setup! kodi is one of the main reasons i use crouton, and i'm glad to see it returning! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like almost everything is fixed. Thanks!
targets/xbmc
Outdated
CHROOTETC='xbmc-keyboard.xml xbmc-cycle.py' | ||
. "${TARGETSDIR:="$PWD"}/common" | ||
REQUIRES='kodi' | ||
PROVIDES='xbmc' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PROVIDES would go in the kodi target, not the xbmc target.
targets/kodi
Outdated
fi | ||
|
||
if [ "$DISTRO" = 'kali' ]; then | ||
error 99 "Kodi not supported on Kali Linux" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! It's not well-documented.
targets/xbmc
Outdated
@@ -1,35 +0,0 @@ | |||
#!/bin/sh -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like in the help case, we set TARGETS='help'
, on line 309 we set TARGETNOINSTALL='y'
and then source the target file. So it looks like the right tweak would be actually in the target common file, line 29, where it prints out the help text. If DESCRIPTION is blank, we don't want to do that.
provides='xbmc' goes in the kodi target. duh!
PROVIDES='xbmc' belongs in the kodi target. DUH!
added a check to only list targets if they have info in the DESCRIPTION
modern kodi is not available in debian repositories. I've set the target to ubuntu only.
ok @dnschneid I think i've resolved all of the issues. I've tested installing to target xbmc and it installs kodi. also, xbmc does not display in the target list due to it's blank description. I tested on sid, and kodi was unable to start. kodi in debian repositories is very old and i decided it would be better to just make it ubuntu only, as that is the only official repository. I edited the description to: "Installs the KODI media player. Ubuntu Only. (Approx. 140MB)". Let me know if you think i should handle anything differently. |
one thing i noticed... when i was testing, it would bail out for kali & debian before creating the chroot & downloading the bootstrap. but now it's downloading the whole thing before stopping. I'm not sure what i did to change it, and i can't duplicate the quick bailout. Any thoughts? am i just crazy? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the code, it seems like it should be bailing out early. You can test by adding a set -x
at the top of the target file and shell will spit out the commands it runs at the beginning; maybe that's provide a clue.
targets/kodi
Outdated
@@ -5,27 +5,19 @@ | |||
if [ "${TARGETNOINSTALL:-c}" = 'c' ]; then | |||
if [ "$DISTRO" = 'kali' ]; then | |||
error 99 "Kodi not supported on Kali Linux." | |||
elif [ "$DISTRO" = 'debian' ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be current in sid (17), and jessie-backports seems to be version 16, which doesn't seem that old. Can you get it to work so we don't artificially limit targets?
Generally we avoid creating targets that are only available in certain distros (unity being the obvious exception to that rule).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll play around with it... When i tried to install it on sid, it wasn't able to connect to X and couldn't start kodi. I'll see if i can figure out why tonight.
catch up master from source
catch up to master
replaced with startkodi
@dnschneid xfce is also broken in debian (and i assume kali) releases. I'm sure there is a better way to solve this on the release script side, and i defer to your wisdom on this. but the quickfix hammer i found was to add "remove xserver-xorg-legacy" to the target file. Since it is such a detrimental package, and running that command doesn't have negative effects if the package is not installed; i just had it run for all releases I'm running a few tests on various releases to see if i need to set anything specific. I'll probably add back the jessie backports for jessie and older. I'll update when i think it's ready to merge. FYI, looking through the log, it looks like the package xserver-xorg-legacy was installed along with a bunch of xserver-xorg packages. right after "Compiling /usr/local/lib/croutonfreon.so" |
turns out kodi doesn't work on Wheezy or Jessie. I'm testing on trusty and precise right now. I'll have it bail out on any releases that don't work. should have final changes tomorrow |
I think i have everything resolved now @dnschneid Thanks for all your help! |
OK, I've made the requested changes. are we good to go now? :) |
targets/kodi
Outdated
|
||
## Debian Overrides | ||
# Kodi wiki recommends using the Jessie Backports repository | ||
if release = jessie; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed one :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, there were 2 instances of "=", but now i'm not sure if i am supposed to switch both of them...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, always use -eq instead of = for the release command
Also, I think the xbmc test needs to be changed into kodi: https://github.com/dnschneid/crouton/blob/master/test/tests/w6-xbmc |
ok, i changed the text to say kodi... I'm not 100% sure what this is or does, so let me know if more is needed. :) |
targets/kodi
Outdated
|
||
## Ubuntu Overrides | ||
# Kodi has an official ppa for the latest stable version | ||
if [ "$DISTRO" -eq 'ubuntu' ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, sorry, but string comparisons should stick to ='s.
release
is a special command that converts the release name into a number and does numerical comparisons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i figured it was something like that...
ok, that's all the pending changes requested... anything else? Thanks a ton for being so helpful! :) |
Looks good to me! Now I run some tests and merge if things pass! Thanks for making and cleaning up the patches! |
i checked the size difference between just the x11 target & having the kodi target, the difference was 200mb.
It looks like the PPA doesn't have any armhf/arm64 packages, so ARM devices will have to go without the PPA. Currently, adding the PPA breaks install on ARM devices, since the metapackages are for any architecture and reference packages that cannot be fulfilled:
|
I'll set it to not add the PPA for armhf/arm64 devices. |
i added [ "$ARCH" = 'amd64' -o "$ARCH" = 'i386' ] to the check for adding the ppa
ok, i set it to only add the ppa on x86, can you test to see if it installs, or if i should just block arm? @dnschneid |
I'll give it a shot. Thanks! |
@dnschneid i've been using kodi and the one annoyance that i still have is the screen timeout... reading the page on the wiki here: https://github.com/dnschneid/crouton/wiki/Power-manager-overrides Regarding which settings to adjust. I believe we want to disable the dim_ms & off_ms for both plugged and unplugged. probably need to adjust the suspend_ms as well... Alternatively, is there a way for kodi to report that it's being "active" when playing video, so that it doesn't trigger any timeouts? |
I created the Kodi wiki page here: https://github.com/dnschneid/crouton/wiki/Kodi-Media-Center I'll fluff it out in the next few weeks. |
host-bin/startkodi
Outdated
exec sh -e "`dirname "\`readlink -f -- "$0"\`"`/enter-chroot" -t xbmc "$@" "" \ | ||
exec xinit /usr/bin/xbmc | ||
exec sh -e "`dirname "\`readlink -f -- "$0"\`"`/enter-chroot" -t kodi "$@" "" \ | ||
exec xinit /usr/bin/kodi --standalone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exec croutonpowerd -i xinit /usr/bin/kodi --standalone
might do what you want with respect to power management. It will prevent the system from idly going to sleep as long as the command (xinit /usr/bin/kodi in this case) is running.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a ton! I'll check it out!
using croutonpowerd -i command
I have made the change that disables power management while kodi is running. I believe this was my last remaining issue to resolve. How has your testing gone @dnschneid ? Do you think it's ready to merge with master? |
Looks good from a testing point of view, although we need to fix the xserver-xorg-legacy thing. Thanks again for this patch! |
Thanks so much for your help @dnschneid ! I'll keep it updated if i find anything that needs fixed. |
RE: Unable to locate package kodi #3395 Not sure exactly what device this is since |
Thanks for pointing it out to me, i responded to the issue. I think the problem is that he was using trusty, and kodi isn't being maintained on arm trusty. |
Kodi is the new name for XBMC.
These changes will rename the target and supporting files to kodi. I also set it up to add the repository in order to get the latest version.
Ubuntu - Official TeamKodi PPA
Debian - Jessie Backports (recommended for debian in kodi wiki)
Kali - not supported. will bail out of install with error.
I've tested with several releases, and it seems to work best with modern ubuntu (xenial+).
Features/Notes
p.s. this is my first pull request ever, let me know if i did something wrong ;)