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] Add automatic dependency handling and move RSXS and some Visualizations to addons #5329

Merged
merged 11 commits into from Sep 4, 2015

Conversation

wsnipex
Copy link
Member

@wsnipex wsnipex commented Sep 6, 2014

This adds automatic dependency handling for cmake based addons by using the depends folder in existing binary addons.
Addons are first downloaded and extracted, then we check if the depends folder exists and handle addon deps dynamically. Tested on linux

Additionally a few screensavers and visualizations are moved over to addons.
@Montellese @jmarshallnz @notspiff ping for platform stuff and sanity checks

Note about projectm: After sinking hours on end into trying to fix the mess that projectms buildsys is and trying to get it compiled static without unresolved symbols, I gave up at let it build dynamically. On linux libprojectm.so is copied to the addon install path.

@Montellese
Copy link
Member

Does this mean that every addon comes with its dependencies as part of the addon? While I agree that it's probably more future proof it also means that we might build the same addon dependency multiple times as a dependency for multiple addons right?

I'm not sure if all these binary addons build on all platforms. At least we don't build them on all of them right now. Most of them are available on linux and osx, some also on android and some on win32. And there are also some that are only available on win32.

@wsnipex
Copy link
Member Author

wsnipex commented Sep 7, 2014

Already existing deps shouldn't be rebuilt. Cmake is smart enough for that. Its possible that stuff that already is in xbmc depends already is duplicated here (once), but we can probably get rid of many depends in xbmc, if we convert more stuff to bin addons.

@wsnipex wsnipex force-pushed the binary_addon_depends branch 4 times, most recently from 3345553 to 153c86c Compare September 9, 2014 14:28
wsnipex referenced this pull request in stefansaraev/xbmc Oct 17, 2014
@wsnipex
Copy link
Member Author

wsnipex commented Nov 3, 2014

@Montellese rebased and updated with your comments.

@wsnipex wsnipex force-pushed the binary_addon_depends branch 2 times, most recently from 9bb552e to 14aa325 Compare November 3, 2014 14:34
@Montellese
Copy link
Member

Downloading the addons and their depends works fine on win32 but patching obviously doesn't. I was hoping that cmake would have a builtin patch mechanism (like the builtin tar support) but couldn't find any info on it.

But the dependencies of the audioencoder addons (I've disabled building of the screensavers for now) in their current state won't work anyway because win32 uses pre-built dependencies for these (I'm working on getting the currently pre-built addon dependencies into cmake projects that can be built on the fly). The problem here is that the pre-built dependencies are called by a different batch script so when building the binary addons cmake doesn't know that these dependencies have already been taken care of and will try to build them again. I'll have to see if there's a better way to do this but IIRC you (@wsnipex) wanted the addon depends and addon build projects separated (but then again that dependency system is currently only used by win32).

What is the long-term goal here? To put all addon dependencies into the addons that need them and not rely on (linux/osx/android) depends at all? What happens if there are multiple addons requiring the same dependency? Do they all have to provide the build instructions? What if they rely on different versions?

@notspiff
Copy link
Contributor

notspiff commented Nov 3, 2014

there is no builtin patch in cmake - you must supply a 'patch 'binary.

@notspiff
Copy link
Contributor

notspiff commented Nov 3, 2014

As for the goal; Decoupling depends from main repo is the final (important) step in the process of making addons true addons. Consider bumping outside mainline release cycles.

First; I would not worry too much about dupe builds. Building as part of a full build is a special case remember, not the one true way.

Particular versions should be done by numbering the target name. The final bit missing is that deps should not be installed into a common prefix; one prefix per target, only added to the prefix paths of particular targets and specific versions is pie.

@wsnipex
Copy link
Member Author

wsnipex commented Nov 3, 2014

What is the long-term goal here? To put all addon dependencies into the addons that need them and not rely on (linux/osx/android) depends at all?

yes, the goal is to get rid of unified deps for bin addons. Every addon should be buildable on its own, without any prerequisits other then what the addon itself specifies.

Do they all have to provide the build instructions?

I take it you mean "dependency instructions". In that case:
yes, cause every addon should be buildable on its own

If you mean that they need to provide install.cmake, flags.txt etc: those are not mandatory.

What happens if there are multiple addons requiring the same dependency?

they will be built only once.

What if they rely on different versions?

if the versions are sufficiently different(read: different include paths and different sonames), it should not collide. Otherwise you can still build those addons seperately or one after another. Still better then current unified deps, where such a scenario is a no-go

about win32: We can add another var/cmake option that indicates to skip building depends. That way we can skip them when needed

@Montellese
Copy link
Member

I've taken another stab at this and here's what I did (see the top 6 commits in https://github.com/Montellese/xbmc/commits/binary_addon_depends):

  • changed the addon cmake project to include the "depends" subdirectory (on win32 only) to get all pre-defined depends
  • manually add kodi as a dependency to every addon (on win32 only)
  • adjust handle-depends.cmake to not do anything if a target with the dependency name/ID already exists. IMO there's no need to read additinal files or apply patches if there already is a target with that name

That way I can build audioencoder.lame, audioencoder.vorbis and audioencoder.wav (which doesn't have any dependencies anyway) and the required dependencies are automatically built before the addon that needs them. audioencoder.flac fails to build because it depends on flac and ogg (at least there's a FindOgg.cmake) but there's only a flac directory in depends/common so it fails to build the ogg dependency.

Do these changes sound acceptable until I have converted all existing pre-built dependencies into cmake-based dependencies?

I haven't tried with the screensaver and visualization addons yet because most of them were never built on win32 before anyway and it will need additional dependency work.

@wsnipex
Copy link
Member Author

wsnipex commented Nov 5, 2014

thanks for looking into it, works fine :)

While testing your changes, I noticed that audio encoders insist on linking against shared libs, even though static ones are available. Iirc this should have worked already in the past, but I'm not 100% sure. I've tried setting CMAKE_FIND_LIBRARY_SUFFIXES to no avail.
using wsnipex/audioencoder.lame@6970207 it links lame statically just fine.
@notspiff any idea whats going on?

@Montellese
Copy link
Member

Nice that they work fine. I'll have to properly cleanup win32 building (make-addon-depends.bat is not needed anymore).

On win32 we only have static libraries available right now so that's not a problem there.

@notspiff
Copy link
Contributor

notspiff commented Nov 5, 2014

not sure what changed, but if you have to force static, use the linker switch - set e.g. LAME_LIBRARIES to (pseudo) -Bstatic -lmp3lame -Bdynamic on relevant platforms.

@wsnipex
Copy link
Member Author

wsnipex commented Nov 5, 2014

this seems to work: wsnipex/audioencoder.lame@03f79aa

edit: omg, ubuntu packages....
Linking CXX shared library audioencoder.lame.so
/usr/bin/ld: /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libmp3lame.a(VbrTag.o): relocation R_X86_64_32S against `bitrate_table' can not be used when making a shared object; recompile with -fPIC
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libmp3lame.a: error adding symbols: Bad value

@Montellese
Copy link
Member

I have added two more commits on top of this at https://github.com/Montellese/xbmc/commits/binary_addon_depends to clean up the win32 buildsystem for binary addons. I've also submitted a PR to the audioencoder.flac repo to fix the ogg dependency. I'll look into visualizations next.

@notspiff
Copy link
Contributor

notspiff commented Nov 5, 2014

the only vizes that are relevant for win32 are spectrum and waveform. the rest are opengl. you have to write a milkdrop addon for win32 (i believe you or wiso started at some point?) and for vortex. note that i have done a blind restructuring of the mildrop 2 POC; https://github.com/notspiff/visualization.milkdrop2

spectrum is visualization.glspectrum and visualization.dxspectrum merged.

@notspiff
Copy link
Contributor

notspiff commented Nov 5, 2014

@wsnipex tons of those all over ubuntu (non-pic static libs). but we don't really want to static link on "normal" linux do we?

@wsnipex
Copy link
Member Author

wsnipex commented Nov 5, 2014

@notspiff yeah, on "normal/standalone", we want dynamic link. Which is now a problem, as the with my last lame change it will just fail.
Maybe we can just drop the find_library(lame) and set LAME_LIBRARIES manually?

@notspiff
Copy link
Contributor

notspiff commented Nov 5, 2014

i'd strongly disencourage that, if only on principle :) there are so many ways it can fail without a proper probe - for instance an user can add extra prefix paths, which won't be handed to the linker, it's only used as part of the searching process.

the way i dealt with this in my POC depends was to not override and deal on the other end of things - only build static libs in the depends.

@wsnipex
Copy link
Member Author

wsnipex commented Nov 5, 2014

thats what I had before, just --disable-shared for lame. I'm ok with it if you are. Its the easiest fix as well, but then we need to make sure all addons depends are static only.

@notspiff
Copy link
Contributor

notspiff commented Nov 5, 2014

yes, i think that is the sensible solution. we have all of those under our control, so it won't be that bad. worst case we have to patch some buildsystems (or delete some files after the install).

@wsnipex
Copy link
Member Author

wsnipex commented Nov 5, 2014

It won't be an issue for most depends, but there already is one that doesn't work: projectm. I've given up on making static work

@notspiff
Copy link
Contributor

notspiff commented Nov 5, 2014

yes i know. for now the .so solution will have to suffice. i can look into it once i have a "stable" setup to base it on. did you try 2.1 btw ?

@notspiff
Copy link
Contributor

notspiff commented Sep 3, 2015

rsxs shall certainly not build on windows. s/all/linux osx in platforms.txt
or thereabout?
3. sep. 2015 18:05 skrev "Memphiz" notifications@github.com:

And now windows said "nope - suckers" :D


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

@notspiff
Copy link
Contributor

notspiff commented Sep 3, 2015

Same for goom
3. sep. 2015 18:12 skrev "Arne Morten Kvarving" cptspiff@gmail.com:

rsxs shall certainly not build on windows. s/all/linux osx in
platforms.txt or thereabout?
3. sep. 2015 18:05 skrev "Memphiz" notifications@github.com:

And now windows said "nope - suckers" :D


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

@wsnipex
Copy link
Member Author

wsnipex commented Sep 3, 2015

uhm, you can check the commits..

@wsnipex
Copy link
Member Author

wsnipex commented Sep 3, 2015

CMake Error at CMakeLists.txt:298 (file):
file RENAME failed to rename

C:/jenkins/slave/workspace/BINARY-ADDONS/platform_label/win32/project/cmake/addons/build/visualization.milkdrop2-dd4e17f677b9759ca330e624f786e0ae82a8531d

to

C:/jenkins/slave/workspace/BINARY-ADDONS/platform_label/win32/project/cmake/addons/build/visualization.milkdrop2

because: No such file or directory

@notspiff
Copy link
Contributor

notspiff commented Sep 3, 2015

such a pain on a phone. if that isn't the cause, sorry for the noise. but
it definitely tried to build those on windows according to the log.

On Thu, Sep 3, 2015 at 6:28 PM, Wolfgang Schupp notifications@github.com
wrote:

uhm, you can check the commits..


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

@notspiff
Copy link
Contributor

notspiff commented Sep 3, 2015

ah bugger sascha mentioned this one. one of the presets ends up with > 256
chars in the path - filesystem limitations.

On Thu, Sep 3, 2015 at 6:30 PM, Wolfgang Schupp notifications@github.com
wrote:

CMake Error at CMakeLists.txt:298 (file):
file RENAME failed to rename

C:/jenkins/slave/workspace/BINARY-ADDONS/platform_label/win32/project/cmake/addons/build/visualization.milkdrop2-dd4e17f677b9759ca330e624f786e0ae82a8531d

to

C:/jenkins/slave/workspace/BINARY-ADDONS/platform_label/win32/project/cmake/addons/build/visualization.milkdrop2

because: No such file or directory


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

@wsnipex
Copy link
Member Author

wsnipex commented Sep 3, 2015

its not really trying to build rsxs or goom, but there is a bug that we seem to show all addons, instead of the platform filtered ones:

Building following addons: adsp.basic adsp.biquad.filters adsp.freesurround audiodecoder.modplug audiodecoder.nosefart audiodecoder.sidplay audiodecoder.snesapu audiodecoder.stsound audiodecoder.timidity audiodecoder.vgmstream audioencoder.flac audioencoder.lame audioencoder.vorbis audioencoder.wav pvr.argustv pvr.demo pvr.dvblink pvr.dvbviewer pvr.filmon pvr.hts pvr.iptvsimple pvr.mediaportal.tvserver pvr.mythtv pvr.nextpvr pvr.njoy pvr.pctv pvr.stalker pvr.vbox pvr.vdr.vnsi pvr.vuplus pvr.wmc screensavers.rsxs visualization.fishbmc visualization.goom visualization.milkdrop visualization.milkdrop2 visualization.projectm visualization.spectrum visualization.vortex visualization.waveform

@Montellese
Copy link
Member

Hm this is a different build error on win32 and one I've never seen before but it's not really related to this PR. When we downloaded a binary addon as a ZIP from github, we extract it and then rename it which in this case failed.

@wsnipex
Copy link
Member Author

wsnipex commented Sep 3, 2015

strange indeed. lets see if we can reproduce it.
jenkins build addons please

@hudokkow
Copy link
Member

hudokkow commented Sep 3, 2015

Stale build env on win VM?
jenkins build this please

@hudokkow
Copy link
Member

hudokkow commented Sep 4, 2015

🍺

@Montellese
Copy link
Member

Now we only have to decide where to put the addon repositories. And which of the binary addon PRs to merge first (I suggest this one).

@wsnipex
Copy link
Member Author

wsnipex commented Sep 4, 2015

I'd also like to get this one in first. I have no preference on where to put the repositories. Guess you can take care of that in your PR.

One last build after rebase: jenkins build this please

@MartijnKaijser MartijnKaijser added this to the Jarvis 16.0-alpha3 milestone Sep 4, 2015
@Montellese
Copy link
Member

Great work everyone. I need to get this in before this PR kills my "PR-with-most-comments" record from #6227 ;-)

Montellese added a commit that referenced this pull request Sep 4, 2015
[binary addons] move RSXS and some visualizations to binary addons
@Montellese Montellese merged commit 4eb1437 into xbmc:master Sep 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Improvement non-breaking change which improves existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet