[cmake] Complete target usage for dependency libs, and remove some old globals and other cmake cleanups #25228
+1,786
−1,731
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fuzzard
force-pushed
the
cmake_cleanup3
branch
5 times, most recently
from
5e1f35d
to
00cba2a
Compare
|
i‘ve picked those commits and didn‘t see any issues during build of my daily driver kodi on debian bookworm |
added 18 commits
May 20, 2024 20:37
added 12 commits
May 20, 2024 20:37
With full target usage, and removal of globals like *_LIBRARIES, *_INCLUDE_DIRS we introduce a wrapper find module that still uses cmakes system find_package(OpenSSL) and make an alias using "our" standard
With full target usage, and removal of globals like *_LIBRARIES, *_INCLUDE_DIRS we introduce a wrapper find module that still uses cmakes system find_package(ZLIB) and make an alias using "our" standard for macro usage
This function is only used in CPackConfigDEB, so move out of generic Macros.cmake
userstamp is only used in CPackConfigDEB. remove out of Macros.cmake
|
I've pulled this into my test builds. I'll let you know if anything comes up. |
|
Something came up, when applying this PR and building LibreELEC 12.0.0 I get the following error. Reverting the PR fixes the problem: |
|
So whats libreelec using, shared libs or static? I would have assumed they are meant to be using shared libs, therefore it shouldnt be trying to link a static lib. you could try the following to get more information about what is populated for link libraries A shared library should show up like whereas a static lib should show something along the lines of (obviously more platform specific) Either way, if libreelec intends to link a static version of libmicrohttpd, it is required to also provide static libs of its dependencies, as they are required to link. |
It is static for some reason https://github.com/LibreELEC/LibreELEC.tv/blob/e92d34bfcad590c172292a03b2ccc6dc31180fb9/packages/web/libmicrohttpd/package.mk#L15 |
|
I imagine thats not the normal? Looks like that flag has been enabled for 11+ years. oversight? |
|
|
im guess that wasnt a clean rerun of the cmake configure. cmakecache is playing games there, so not really useful. |
|
Clean build will follow in 1hr |
|
Either way, the information comes from pkg-config. libmicrohttpd says it needs gnutls, gnutls isnt made available for it to statically link. Id say you'll need the libreelec guys to look into whats best |
|
I've asked in their slack about static linking. In the past, the answer I received was that everything was static linked, everything, even including add-ons that ship with LE. This is probably due to performance constraints across their ecosystem of devices. I'll update if I hear back, but I think it's still safe to assume that everything is static linked for LE. |
|
their gnutls package says otherwise. https://github.com/LibreELEC/LibreELEC.tv/blob/e92d34bfcad590c172292a03b2ccc6dc31180fb9/packages/security/gnutls/package.mk#L15-L28 |
The libmicrohttpd is built as static (and only used by kodi) Here are the libraries of interest: |
|
Duplicating a static libmicrohttpd that is using a shared gnutls i get the following (using the above patch). It still shows the shared lib of gnutls. You can see a shared library is still made available from the pkgconfig output. Kodi successfully links with correctly Someone on the libreelec side is going to have to dig in further to whatever is happening on their side. As far as i can see, things work as intended. |
|
I’ve done test build with HEAD and this patch - successful with the .so (not with the .a) for interest the kodi.bin + the .do is actually smaller than that of the kodi.bin compiled against .a run testing this now - have written up a draft PR to allow some testing. |
14 tasks
jenkins4kodi
added
the
Rebase needed
|
@fuzzard this needs a rebase |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Well this PR got bigger than i first intended. This completes moving all find modules to target usage. I wouldnt consider all the modules "final" as such, but for this part, they are functional and complete (as best i could test/see).
This allowed to remove a number of global variable usage. Ive also done some minor cleanups/relocations of some cmake stuff i saw along the way.
Motivation and context
Modern cmake usage
How has this been tested?
Jenkins has been doing some work. Ive runtime tested macos, and spent a bit of time comparing master builds to outputs from this branch. As best i can see i believe ive covered everything, however this is obviously a massive change, so if anyone comes across cmake issues, or specific features from libs no longer functioning, let me know.
What is the effect on users?
N/A
Screenshots (if appropriate):
Types of change
Checklist: