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

[cmake] Change the way how shared libraries are built #10130

Merged
merged 9 commits into from Jul 21, 2016

Conversation

@fetzerch
Copy link
Member

commented Jul 17, 2016

This is the main prerequisites to make the VS and Xcode projects more usable. I haven't found a better way than getting rid of the static libs per directory and build kodi in one go. My plan is to change core_add_library for Xcode and VS to use target_sources and add all sources to the main executable here. In order to allow this core_add_library must not be used for anything else (for example for libraries).
I've put some info in http://jira.kodi.tv/browse/KODI-16651.

The main change is cbafd20. Even though it contains a few things that are independent, most of the things are somehow connected, so I put it in one commit for simplicity. @hudokkow, @notspiff: Could you review this please?

Tested for Linux, OSX (GNU Makefiles) and Android (GNU Makefiles). More testing needed for Windows (NMake and VS), OSX (Xcode) and Ninja generators.

The PR includes also a few minor changes and improvements. d0befd2 is certainly not the best way to do it, because the addon build system is also CMake based. I've mainly done it now for testing so that I can locally build osx and android packages that contain a few binary addons to see if the libraries still load fine after installation.


if(CORE_SYSTEM_NAME STREQUAL windows)
add_library(unrarxlib STATIC ${SOURCES} ${HEADERS})
if(NOT CORE_SYSTEM_NAME STREQUAL windows)

This comment has been minimized.

Copy link
@Paxxi

Paxxi Jul 17, 2016

Member

isn't it easier to use if(NOT WIN32) ?

This comment has been minimized.

Copy link
@wsnipex

wsnipex Jul 17, 2016

Member

we prefer to use our own CORE_SYSTEM_NAME instead of the cmake builtins here.

@wsnipex

This comment has been minimized.

Copy link
Member

commented Jul 17, 2016

Very nice. Makes the whole buildsys easier to understand and maintain as well.

@hudokkow

This comment has been minimized.

Copy link
Member

commented Jul 18, 2016

@fetzerch, please pick hudokkow@4e458a4

Rest looks OK. Great work!

@fetzerch fetzerch force-pushed the fetzerch:cmake_splitlibs branch from cbafd20 to 96e8201 Jul 19, 2016

@fetzerch

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2016

Thanks! I've squashed @hudokkow's fix and make binary-addons ADDONS=visualization.spectrum now works on linux as well. Will test the missing generators now.

@fetzerch fetzerch force-pushed the fetzerch:cmake_splitlibs branch 3 times, most recently from 78c6951 to 5b995f3 Jul 19, 2016

@fetzerch fetzerch referenced this pull request Jul 19, 2016

@fetzerch fetzerch force-pushed the fetzerch:cmake_splitlibs branch 2 times, most recently from 88d1dcb to 0e034c1 Jul 20, 2016

@fetzerch

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2016

Tested on all platforms now. Windows works as well, due to @Paxxi's wdt/depends fixes. If there are no objections, I'll merge it tomorrow.

@hudokkow: as discussed, I've removed the generator expressions for the libs again because we cannot get the relative directories at configure time. That also makes the android bundle target a bit easier again.

@hudokkow

This comment has been minimized.

Copy link
Member

commented Jul 20, 2016

Merge today or window is closed...

fetzerch added 9 commits Jul 8, 2016
[cmake] Add project wide dependency to libcpluff and ffmpeg
The two dependencies libcpluff and ffmpeg are widely required
throughout the whole project. Adding a project wide dependency ensures
that they are built before all libraries added with core_add_library.
This removes the need to specify them per directory which was error
prone in the past (and up to now only fully worked by chance).
[cmake] Add binary-addons target for makefile generator
Adds a target that allows to build the binary addons:
make binary-addons ADDONS="visualization.spectrum"

This can be used if local addons should be built for packaging (for
example on osx or android).
[cmake] Let ExternalProject_Add figure out the right 'make' command
ExternalProject.cmake sets a default BUILD_COMMAND to 'make'. Not
overriding it has the advantage that it figures out the best option
itself (including passing down parallel build parameters).

This fixes the Ninja build on Linux due to a problem introduced in
8108954 where ninja would be used to
build ffmpeg and cpluff (which doesn't work).
[cmake] Change the way shared libraries are built
Before this change shared libraries were built by creating a static
library using core_add_library and then linking that into a
shared/module one using core_link_libraries. This has a few drawbacks:

 - Meanwhile most libraries don't use wrapping (for VFS support)
   anymore, and we can use CMake mechanisms to create those libraries.
 - The approach doesn't work with MSVC as there is no "whole-archive"
   option. On windows these symbols would need different exporting.
 - The main usage of core_add_library is to generate small libraries
   for the Kodi main application. Let's use it only for that.
   This is in preparation to disable building of per folder static
   libraries because they don't work well with VS and Xcode.

This commit introduces the following changes:
 - Shared/Module libraries are now created with core_add_shared_library,
   addon callback libraries are now created with core_add_addon_library.
 - Fix dependencies: make kodi now builds everything needed to run kodi
   including all dl-loaded libraries.
 - Only use wrapping for libraries where it's also done with Autotools
 - WRAP_FILES and wrap-libraries are renamed to LIBRARY_FILES and
   kodi-libraries.
 - Library wrapping and generation now works in all subdirectories.
   Previously core_link_libraries had to be called in the main
   CMakeLists.txt because file dependencies in add_custom_command only
   work in the same directory. This also made it necessary to create
   wrap_* targets.
 - Generator expressions in core_link_libraries make the function
   less error prone.

@fetzerch fetzerch force-pushed the fetzerch:cmake_splitlibs branch from 0e034c1 to 67d335a Jul 21, 2016

@fetzerch fetzerch merged commit 985af0b into xbmc:master Jul 21, 2016

1 check was pending

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

@fetzerch fetzerch deleted the fetzerch:cmake_splitlibs branch Jul 21, 2016

@hudokkow hudokkow added this to the Krypton 17.0-alpha3 milestone Jul 21, 2016

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