Make KItinerary work as a static library
AbandonedPublic

Authored by vkrause on Jun 30 2018, 6:19 PM.

Details

Reviewers
None
Group Reviewers
Frameworks
Summary

The tricky part is static construction (needed for the Qt resource system
among other things), that isn't available with static libraries. I didn't
find a more elegant solution than an explicit init function for that.

Putting this up for review for the general concept, assuming we want static
library support in other libraries/frameworks too, as it has some benefits
for minimizing app bundles (such as Android APKs).

Diff Detail

Repository
R1003 KItinerary: Travel Reservation handling library
Branch
static-build
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 502
Build 502: arc lint + arc unit
vkrause created this revision.Jun 30 2018, 6:19 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptJun 30 2018, 6:19 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
vkrause requested review of this revision.Jun 30 2018, 6:19 PM
apol added a subscriber: apol.Jul 3 2018, 10:56 PM

It does have some advantages, I personally thing though that we shouldn't be compromising code readability and convenience for size, but I don't know how much better it is shared vs static.

If we need to have a list of <frameworkname>::init() with all frameworks we depend on, it will be a bit odd and error prone, but it could be useful.

src/init.cpp
34

This function should be static too.

In D13816#286689, @apol wrote:

It does have some advantages, I personally thing though that we shouldn't be compromising code readability and convenience for size, but I don't know how much better it is shared vs static.

Yep. I don't have numbers yet for KDE examples (as we can't build a working test-case at this point), from what I have seen at work I'd expect something in the 2x-3x range compared to a dynamic build, as well as some small startup speed gains (in the 100ms range).
Maybe it's just me, but our APKs still feel a bit heavy to me (KDE Itinerary is now at 27M here, for what's essentially a list view), therefore I'm exploring this option a bit.

If we need to have a list of <frameworkname>::init() with all frameworks we depend on, it will be a bit odd and error prone, but it could be useful.

Yep, I don't like this at all either. However, you will only need this in your application code, and only if you explicitly want to statically link it (ie. no impact for "normal" applications). And it wont be needed for all libs either, only those that rely on static construction somehow (qrc, qm catalog loaders, etc) and don't have a natural entry point or (internal) singleton already anyway (my best guess at this point, maybe 1/3). Not that this makes it less error prone though, on the contrary even.

So for me this boils down to: do we want to enable users of (some of) our frameworks to do static builds and are we willing to accept the ugliness this brings with it?

Or does anyone have an idea how to solve this more elegantly? :)

+1

I know @lepagevalleeemmanuel looked into this issue as well, maybe he has some insights.

Hello,

assuming we want static library support in other libraries/frameworks too,

I can't talk for everybody, but I do want it. I actually maintain a huge patchset to fix static frameworks for ~30 of them. Getting it merged require face to face support with some people. Hopefully I can fix some of "all frameworks need this patch" cases at Akademy. I already merged the Kirigami patches and there is an "official" branch for qqc2-desktop-style too.

Or does anyone have an idea how to solve this more elegantly? :)

Hi, we talked about this at Randa and QtWorldSummit last year. Sorry for stopping working on this after I made enough progress to make it work for me. I was assigned to work on other things. The clean way is using:

Q_IMPORT_PLUGIN(KirigamiPlugin)

Like other Qt static modules, This can be semi-automated using MOC magic and CMake-foo. All the magic is there https://github.com/ring-project/ring-kde-appimage-builder/tree/gentoo_base . It's not very clean, but it works. I have to catch a plane later today, so I can't write a lengthy comment right now.

Just to say, fixing static frameworks and LTO bugs reduce the size of the KF5 based APK/AppImage by 86% and they load 6-8 time faster when PGO/FDO is enabled. Of course all of this is not "free". There is a non-trivial amount of work before @apol SDK or mine can work with random KF5 apps. Things like https://bugs.kde.org/show_bug.cgi?id=387820 become real crashers instead of unrefined behaviors. I brute force fixed all of them using horrible hacks to Ring-KDE could ship on Android/Plasma_Mobile/Appimage/macOS with a 13-18mb (with or without breeze icons) size instead of ~185-200mb (with or without breeze icons).

See how me and @mart patched kirigami for queues how to do it "right". The definitions of "right" here is "exactly compatible with Qt static SDK module format". There is zero documentation on that topic. I reverse engineered it from the final Qt .a using gcc-nm and reading the auto-generated code qmake produces. I began to work on a blog post last year about that, but because I had to move on, it was never finished. It only document about 10%, read the code and patches for the other 90% https://gist.github.com/Elv13/a5805f2c979f3f86dfad0eb16d03c388

Or does anyone have an idea how to solve this more elegantly? :)

Hi, we talked about this at Randa and QtWorldSummit last year. Sorry for stopping working on this after I made enough progress to make it work for me. I was assigned to work on other things. The clean way is using:

Q_IMPORT_PLUGIN(KirigamiPlugin)

Right, that works for plugins. There is even https://codereview.qt-project.org/#/c/195680/ since 5.11 allowing us to hook "plugin loading", for things covered by static construction in the dynamic case. The patch here is looking more at something like the bug report you mentioned below, ie. use of static construction in libraries rather than plugins.

Like other Qt static modules, This can be semi-automated using MOC magic and CMake-foo. All the magic is there https://github.com/ring-project/ring-kde-appimage-builder/tree/gentoo_base . It's not very clean, but it works. I have to catch a plane later today, so I can't write a lengthy comment right now.

Most of those patches seem to be dealing with the Qt5::Test dependency or other dependency-related changes, ie. this should be pretty harmless to integrate?

Just to say, fixing static frameworks and LTO bugs reduce the size of the KF5 based APK/AppImage by 86% and they load 6-8 time faster when PGO/FDO is enabled. Of course all of this is not "free". There is a non-trivial amount of work before @apol SDK or mine can work with random KF5 apps. Things like https://bugs.kde.org/show_bug.cgi?id=387820 become real crashers instead of unrefined behaviors. I brute force fixed all of them using horrible hacks to Ring-KDE could ship on Android/Plasma_Mobile/Appimage/macOS with a 13-18mb (with or without breeze icons) size instead of ~185-200mb (with or without breeze icons).

That bug report is more what I'm looking at here. I disagree with your assessment though that this is undefined behavior or broken in the current dynamic code. Static construction is actually a pattern used all over the place, among them are qrc, the qml compiler and the ECM qm catalog loader, as well as custom code as you found in KConfig, or as this patch tries to support in KItinerary. So I think we need a proper solution for this.

lepagevalleeemmanuel added a comment.EditedJul 9 2018, 7:52 PM

Right, that works for plugins. There is even https://codereview.qt-project.org/#/c/195680/ since 5.11 allowing us to hook "plugin loading", for things covered by static construction in the dynamic case. The patch here is looking more at something like the bug report you mentioned below, ie. use of static construction in libraries rather than plugins.

Sorry, my bad, it has been a while and I totally got confused about that part. Yes, indeed, only some frameworks like Kirigami and QQC2-ds are plugins. The others are just normal libs.

That bug report is more what I'm looking at here. I disagree with your assessment though that this is undefined behavior or broken in the current dynamic code. Static construction is actually a pattern used all over the place, among them are qrc, the qml compiler and the ECM qm catalog loader, as well as custom code as you found in KConfig, or as this patch tries to support in KItinerary. So I think we need a proper solution for this.

Static construction is fine. The issue here is a lot more subtle. Let's take a similar case. Back when the distributions started to use the --as-needed LDFLAGS, there was a lot of this issue too. If a library is not used, then you can't "trust" that the static code will get executed because the linker has the "right" to drop whole .so. When using LTO, you get this issue on the compilation unit level. So a .o can be dropped entirely if nothing points to that CU, even if static init from that CU point to other CU. This is why static code needs to be handled with care on them. It isn't forbidden, it just can't be "trusted" to be executed. In the case of the kconfig bug, it clearly isn't executed with very bad consequences. It is not a gold linker bug, it's just how LTO dead code elimination works.

Most of those patches seem to be dealing with the Qt5::Test dependency or other dependency-related changes, ie. this should be pretty harmless to integrate?

Yes and no. The majority is easy, but there is a very hard minority of changes that will be enormously controversial.

The easy basic:

  • Allow to disable Qt::Test because LTO-ing test takes too much resources to be ran in every CI build
  • Stop using "SHARED" in add_library and always use the "BUILD_SHARED_LIBS ON" option
  • Fix all cmake "targets" to have the correct dependencies exported to avoid adding manual -lkf5foo due to misexported deps
  • For the framework that are Qt plugins, add the necessary MOC arguments

The much harder parts:

  • There is some new classes of bugs like dead code elimination having side effects
  • Some things like Kio "hard" dependencies have to go or undergo large redesign because they often are used in a single line of code (file picker widget) and pull 10 frameworks that make no sense for static binaries. Another one is KColorScheme being in KConfigWidgets and pulling a bunch of stuff into QML apps
  • Everything using client-daemon or multi-process model become very impractical on Android and other bundles
  • Everything that uses dlopen is potentially broken
  • All KDE deamons like kdeinit5/kded5/knotify need to be reconsidered because they make no sense in sandboxes and have bad side effects like increased binary size instead of a reduction due to each binary having its own Qt
  • Things like the icon theme and resource should to have some ways of getting bundled to have "portable" binaries
  • Some external customization like icon theme, color scheme and theme break

Ideally / eventually / hopefully:

  • The kconfig kcfg compiler need either to be rewritten without Qt or exposed as web service because it is "incompatible"/"impractical" with cross-compilation
  • The breeze icon pack generator and translation compiler also need some work to be easier to integrate
  • We need a CI to validate the result and detect regression otherwise it will break again after 1 week

For the last 2, I mix static builds with "container driven cross compilation" issues. It's not fully relevant to static libs, but rather the use case for them,

So in the end, it's not simple and will make some people very unhappy. The patches I maintain is the strict minimum for ring-kde to link and work. It isn't a clean job and it isn't the complete picture. I felt like this isn't something I can just start submitting patches and eventually finish. It needs buy-in from a lot of people and I am not sure it can happen without face to face discussion at Akademy or something.

Edit: adding the KColorScheme issue discussed at QtWorldSummit

vkrause added a subscriber: arojas.Jul 10 2018, 8:07 AM

That bug report is more what I'm looking at here. I disagree with your assessment though that this is undefined behavior or broken in the current dynamic code. Static construction is actually a pattern used all over the place, among them are qrc, the qml compiler and the ECM qm catalog loader, as well as custom code as you found in KConfig, or as this patch tries to support in KItinerary. So I think we need a proper solution for this.

Static construction is fine. The issue here is a lot more subtle. Let's take a similar case. Back when the distributions started to use the --as-needed LDFLAGS, there was a lot of this issue too. If a library is not used, then you can't "trust" that the static code will get executed because the linker has the "right" to drop whole .so. When using LTO, you get this issue on the compilation unit level. So a .o can be dropped entirely if nothing points to that CU, even if static init from that CU point to other CU. This is why static code needs to be handled with care on them. It isn't forbidden, it just can't be "trusted" to be executed. In the case of the kconfig bug, it clearly isn't executed with very bad consequences. It is not a gold linker bug, it's just how LTO dead code elimination works.

Yep, and that's what this patch proposes a solution for. Not a pretty one, but it at least addresses this reliably and platform-independent.

Most of those patches seem to be dealing with the Qt5::Test dependency or other dependency-related changes, ie. this should be pretty harmless to integrate?

Yes and no. The majority is easy, but there is a very hard minority of changes that will be enormously controversial.

The easy basic:

  • Allow to disable Qt::Test because LTO-ing test takes too much resources to be ran in every CI build

@arojas is currently integrating a large set of patches for this I think.

  • Stop using "SHARED" in add_library and always use the "BUILD_SHARED_LIBS ON" option
  • Fix all cmake "targets" to have the correct dependencies exported to avoid adding manual -lkf5foo due to misexported deps
  • For the framework that are Qt plugins, add the necessary MOC arguments

    The much harder parts:
  • There is some new classes of bugs like dead code elimination having side effects
  • Some things like Kio "hard" dependencies have to go or undergo large redesign because they often are used in a single line of code (file picker widget) and pull 10 frameworks that make no sense for static binaries.
  • Everything using client-daemon or multi-process model become very impractical on Android and other bundles
  • Everything that uses dlopen is potentially broken
  • All KDE deamons like kdeinit5/kded5/knotify need to be reconsidered because they make no sense in sandboxes and have bad side effects like increased binary size instead of a reduction due to each binary having its own Qt
  • Things like the icon theme and resource should to have some ways of getting bundled to have "portable" binaries
  • Some external customization like icon theme, color scheme and theme break

Valid points but mostly affecting tier 3(+) frameworks I think, most tier 1/2 frameworks are at most affected by static construction. So I think we can look at this independently, as for some use-cases you don't need the full set of frameworks (e.g. newly written applications targeting Android from the start).

Ideally / eventually / hopefully:

  • The kconfig kcfg compiler need either to be rewritten without Qt or exposed as web service because it is "incompatible"/"impractical" with cross-compilation
  • The breeze icon pack generator and translation compiler also need some work to be easier to integrate
  • We need a CI to validate the result and detect regression otherwise it will break again after 1 week

    For the last 2, I mix static builds with "container driven cross compilation" issues. It's not fully relevant to static libs, but rather the use case for them,

    So in the end, it's not simple and will make some people very unhappy. The patches I maintain is the strict minimum for ring-kde to link and work. It isn't a clean job and it isn't the complete picture. I felt like this isn't something I can just start submitting patches and eventually finish. It needs buy-in from a lot of people and I am not sure it can happen without face to face discussion at Akademy or something.

That's why I'm not trying to look at this as one monolithic change, but one piece at a time :) I don't think we need to wait until Akademy to discuss e.g. this proposed solution for the static construction problem.

src/CMakeLists.txt
50

You *might* want to add

OPTION(BUILD_SHARED_LIBS   "Build the shared library (instead of static)" ON )

near the top of the CMakeLists.txt for the sake of making sure it's the default and for readability?

src/init.cpp
24

unused?

krop added a subscriber: krop.Jul 10 2018, 12:52 PM
krop added inline comments.
src/CMakeLists.txt
50

No need, extra-cmake-modules already sets it to TRUE if KDECMakeSettings is included.

src/CMakeLists.txt
50

Ok, thanks. So it's a "Ship it!" for my point of view (if it matter at all)

vkrause abandoned this revision.Aug 24 2018, 4:35 PM

Partly integrated, partly replaced by implicit on-demand initialization to avoid the ugly explicit init() call.