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

[binary addons] move PVR addons to our binary addons buildsystem #6227

Merged
merged 18 commits into from
Mar 2, 2015

Conversation

Montellese
Copy link
Member

These commits (and all the PVR addons in my github account) are my initial take at moving PVR addons out of the xbmc-pvr-addons repository and into our binary addons buildsystem. A few notes

  • I've only really tested compiling as I don't have any PVR setup. I only tested the pvr.demo addon which lead to the first commit.
  • I've only tested this on WIN32
  • all PVR addons in my github account (see https://github.com/Montellese?tab=repositories) are based on the repositores created by and kept in sync by @notspiff so a big thank you to him.
  • all PVR addons depend on https://github.com/Montellese/kodi-platform which in turn depends on tinyxml which are added as common dependencies to all PVR addon repositories.

TODOs

  • move kodi-platform into kodi's depends buildsystem
  • test building on all platforms
  • test every PVR addon
  • fix detection of OpenGL/OpenGLES2 in pvr.vdr.vnsi
  • pvr.dvblink depends on libcurl
  • pvr.filmon depends on libcurl
  • libjansson and cppmyth includes and libs are installed into the wrong directory
  • pvr.iptvsimple depends on zlib which is downloaded from our mirrors and built using the supplied CMakeLists.txt. Unfortunately it builds shared and static libs and the builtin FindZlib.cmake provided with cmake picks up the shared lib instead of the static lib (at least on WIN32) and I haven't found a way around that yet.
  • sync all PVR addons to xbmc-pvr-addons
  • get the dependency handling right
  • move kodi-platform to xbmc's github account

@Montellese Montellese added Type: Improvement non-breaking change which improves existing functionality RFC PR submitted for gathering feedback Component: PVR labels Jan 19, 2015
@opdenkamp
Copy link
Member

'kodi-platform' is actually a copy of the code that i wrote for libCEC, and isn't fully synced anymore (both the copy in pvr-addons and libCEC have changed in the meantime). let's sync them up again and rename it to something that doesn't contain 'kodi' in the name and both use the shared lib/headers.

@opdenkamp
Copy link
Member

thanks for starting this. it was on my todo list for this release, so one more thing to scratch of it ;)

@Montellese
Copy link
Member Author

Can I see the platform library used by libCEC somewhere? Apart from the include path changes (done by @notspiff) I've only added more complete atomics support (based on the code in Kodi).

@notspiff
Copy link
Contributor

@Montellese
Copy link
Member Author

@notspiff: Thanks, just find it myself a minute ago.

@notspiff
Copy link
Contributor

Cool. You be upstream now for convenience?

@Montellese
Copy link
Member Author

Sure, at least until we move it to the xbmc account.

@Montellese
Copy link
Member Author

I've added a TODO list to the PR description with everything that came to mind. Let me know if there are other steps/things that need to be taken care of before this can be merged.

@notspiff
Copy link
Contributor

the zlib can possibly be fixed by doing

set(CMAKE_FIND_LIBRARY_SUFFIXES .lib)
before calling find_package(ZLIB)

don't forget to set it back.

@opdenkamp
Copy link
Member

re platform lib: the platform lib that @notspiff linked is an older copy too. the one in p8's repos is the correct one: https://github.com/Pulse-Eight/libcec/tree/release/src/lib/platform

re codec descriptor header: yeah let's do that

opdenkamp referenced this pull request in Memphiz/xbmc Jan 20, 2015
@notspiff
Copy link
Contributor

it would be nice if the one responsible for the mess could clean it up so we get it correct ;)

@opdenkamp
Copy link
Member

getting it synced and extracted from libCEC and this repos can be done
pretty quickly. i just never bothered to write separate makefiles for it.

On 20-01-15 10:38, Arne Morten Kvarving wrote:

it would be nice if the one responsible for the mess could clean it up
so we get it correct ;)


Reply to this email directly or view it on GitHub
#6227 (comment).

@notspiff
Copy link
Contributor

if you can just sync the files i'll do the buildsystem. just not sure what is outdated where etc - i have continuously synced it with the pvr repo and it won't just be taking whole files from one repo i reckon.

@wsnipex
Copy link
Member

wsnipex commented Jan 20, 2015

@Montellese for linux standalone building I need those:
wsnipex@0907ab6
wsnipex/kodi-platform@9e786c5

furthermore, there seems to be a conflict with ubuntu system libtinyxml. kodi-platform compiles fine with it standalone. But from within the addon build env there is a clash with the downloaded one. If I remove the system tinyxml, it builds fine, when applying the 2 commits above.

@Montellese
Copy link
Member Author

@wsnipex: Do we handle dependencies of dependencies? If so we could probably get rid of all the tinyxml dependecy definitions in all the PVR addons and only add kodi-platform as a dependency.
I also just pushed a commit to kodi-platform that removes the need for TARGET_POSIX. I copied that from Kodi core code and didn't know it wasn't set. The question is if we want to use TARGET_FOO in there as well (in which case we need additional logic in the buildsystem so that the defines are also set for TARGET_DARWIN et al.) or not. There are only very few places where they are used.

Concerning tinyxml on linux standalone: How does this work right now for libraries that are already installed on the system? Should it still be downloaded and built by the cmake buildsystem (even though it will then pick the system library) or not?

@opdenkamp
Copy link
Member

tinyxml should be a separate thing? it doesn't have anything to do with
the rest of the code in the platform lib

On 20-01-15 14:15, Wolfgang Schupp wrote:

@Montellese https://github.com/Montellese for linux standalone
building I need those:
wsnipex/xbmc@0907ab6
wsnipex@0907ab6
wsnipex/kodi-platform@9e786c5
wsnipex/kodi-platform@9e786c5

furthermore, there seems to be a conflict with ubuntu system
libtinyxml. kodi-platform compiles fine with it standalone. But from
within the addon build env there is a clash with the downloaded one.
If I remove the system tinyxml, it builds fine, when applying the 2
commits above.


Reply to this email directly or view it on GitHub
#6227 (comment).

@notspiff
Copy link
Contributor

there is code in the platform lib that depends on tinyxml (XMLUtils). this is handled by pkgconfig/cmake config mode (or shared linking) on linux, but on static platforms AND non-pkg-config platforms (=win32) you get into this mess, ie, having to handle deps of deps manually.

@opdenkamp
Copy link
Member

xmlutils isn't supposed to be in there either ;-) the platform code was
written as an abstraction for the standard library things (like threads
and mutexes) that need platform specific code.

On 20-01-15 14:35, Arne Morten Kvarving wrote:

there is code in the platform lib that depends on tinyxml (XMLUtils).
this is handled by pkgconfig (or shared linking) on linux, but on
static platforms AND non-pkg-config platforms (=win32) you get into
this mess, ie, having to handle deps of deps manually.


Reply to this email directly or view it on GitHub
#6227 (comment).

@notspiff
Copy link
Contributor

argh, hit post too early. continuing.

this means that as long as everything uses cmake, even win32 should be handled through the config-mode. can you confirm that kodiplatform-config.cmake holds the -ltinyxml et al on win32 montellese? that's what is supposed to save you so you do not have to handle it in downstream modules.

opdenkamp, we just did our best to keep up with the mess in upstream.

@opdenkamp
Copy link
Member

@notspiff sure, but let's try to clean up the mess now and not make a
bigger mess out of it, so both libCEC and Kodi can keep using this code
without copying stuff over all the time. I don't want libCEC to suddenly
get a dep on tinyxml when dists start packaging this.

On 20-01-15 14:39, Arne Morten Kvarving wrote:

argh, hit post too early. continuing.

this means that as long as everything uses cmake, even win32 should be
handled through the config-mode. can you confirm that
kodiplatform-config.cmake holds the -ltinyxml et al on win32
montellese? that's what is supposed to save you so you do not have to
handle it in downstream modules.

opdenkamp, we just did our best to keep up with the mess in upstream.


Reply to this email directly or view it on GitHub
#6227 (comment).

@notspiff
Copy link
Contributor

right. well, since that shit is used in the pvr addons, we have to split this in two libraries then. one pure platform, and one kodi helper library (= kodi-platform).

@Montellese
Copy link
Member Author

Ah I thought kodi-platform was simply a utility library that contained utility stuff that is useful to multiple projects. Didn't know it was meant to be limited to platform-specific stuff only. If we want to keep that, we probably need to introduce a separate library containing utility classes (which can depend on the platform library).

AFAIK XMLUtils is/was only moved into kodi-platform because it depends on StdString.h. The core version of XMLUtils works with std::string now so if that one could be fixed in the PVR addons XMLUtils could go into the new utility library which would then depend on tinyxml.

EDIT: @notspiff beat me to it.

@notspiff
Copy link
Contributor

as i have suggested before montellese, my approach to this would be to separate those utils from mainline and put them in the support library, then have kodi iself depend on the support library. this to avoid code duplication, which seems to have been the approach thus far.

@opdenkamp
Copy link
Member

that would be even better, but a lot more work. so let's do that as a second step, after this is in. and then we should take kodi's implementation of the mutex, locking, threading, etc. and change the code in libCEC and add-ons to use that implementation instead. that'll be much less intrusive and more complete, as it's pretty much the bare minimum that was included for libCEC and the add-ons. writing a wrapper to keep it backwards compatible isn't much work either.

@notspiff
Copy link
Contributor

i've done so much work on this over the years, that it is peanuts. i do agree though that we can postpone it.

@hudokkow
Copy link
Member

hudokkow commented Mar 1, 2015

You're welcome!

Not all. We're still missing https://github.com/xbmc/pvr.demo and https://github.com/janbar/pvr.mythtv

@FernetMenta
Copy link
Contributor

thanks, added those two

@Montellese
Copy link
Member Author

Updated all of the github links and commit hashes. jenkins build addons please

@opdenkamp
Copy link
Member

great, thanks everyone!

@FernetMenta
Copy link
Contributor

green button time?

@wsnipex
Copy link
Member

wsnipex commented Mar 2, 2015

didn't you want to point to addons master branch, instead of individual commits?
For release branches, we can still point to a "stable" hash.

@notspiff
Copy link
Contributor

notspiff commented Mar 2, 2015

i'm confused; are you actually planning to keep the addon definitions in the main git ?

@Montellese
Copy link
Member Author

@wsnipex: Pointing to a branch doesn't work unless you disable/remove the logic that checks if a tarball has already been downloaded.

@notspiff: Until we have setup something else IMO yes. But if we hold off because of that now this keeps dragging on and on. IMO if we merge this as-is we already get a testing phase and we can still improve the whole integration.

I also have another improvement but I don't want to push it into this PR because if it doesn't work immediately it will prevent this from being merged.

But that's just my opinion.

@notspiff
Copy link
Contributor

notspiff commented Mar 2, 2015

right'o. as long as it ain't the long term plan..

@opdenkamp
Copy link
Member

do we have a solution for dependencies on something not statically linked into the add-on? and can we install binaries on android that don't end up on a noexec mounted partition?

@notspiff
Copy link
Contributor

notspiff commented Mar 2, 2015

yeah, solution is very simple: no you don't unless you are on linux.

droid needs to be fixed on a general level, ie, it needs to do like linux and look for resources and binaries in separate directories. then that needs to be handled in the installer, since kodi handles the installation on droid on its own (as opposed to apt which handles it on debian).

@opdenkamp
Copy link
Member

right, let's discuss both in another thread later before we reach 600 here ;-)

@FernetMenta
Copy link
Contributor

I agree, this is fine for now. who wants to push the button? please do :)

@opdenkamp
Copy link
Member

you? or let's see if jenkins can be triggered from email nowadays ;)
jenkins build and merge

On 02-03-15 14:14, Rainer Hochecker wrote:

I agree, this is fine for now. who wants to push the button? please do :)


Reply to this email directly or view it on GitHub
#6227 (comment).

@wsnipex
Copy link
Member

wsnipex commented Mar 2, 2015

I cancelled that since mirrors were down. Now again: jenkins build and merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PVR RFC PR submitted for gathering feedback Type: Improvement non-breaking change which improves existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet