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

Stop using -static-libstdc++ #1513

Merged
merged 23 commits into from
Sep 4, 2022
Merged

Stop using -static-libstdc++ #1513

merged 23 commits into from
Sep 4, 2022

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Sep 2, 2022

Opting instead on shipping the TC's shared STL, as it's bound to be much, much saner.

Modified how Lua modules are linked to avoid passing absolute paths to shared libraries, because, depending on binutils version, this matters.

Also stopped lniking sdcv to a static libz outside of Android (where I'm not even sure why we do that for glib...).


This change is Reviewable

We'll ship the TC's STL instead, to avoid breaking the ODR.
I'm not even quite sure why we'd do that there in the first place...
Force-feeding absolute path is known to do weird shit with some binutils
versions...
My idea is mildly spoiled by the fact that we only keep the SOVER
variant of the library, which means we still need to help the linker a
bit...
Oh, well, at least that documents the often-forgotten -l: syntax ;).
Apparently, if you pass a relative path to target_link_libraries, it
assumes it's a library name and just prpeends a -l...
@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 2, 2022

Depends on koreader/libk2pdfopt#40 & koreader/crengine#488

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 2, 2022

Also stopped lniking sdcv to a static libz outside of Android (where I'm not even sure why we do that for glib...).

Checking the history leads to #342, which limited the static mess to Android, and #317, which introduced it...

I'm pretty sure it's completely unnecessary nowadays, but, who knows...

@NiLuJe

This comment was marked as resolved.

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 3, 2022

Hey, it almost worked first try on Android ;o).

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 3, 2022

There, fixed Android.

Because it historically offered a choice of STL, everything is terrible:

  • libstdc++.so exists, but is NOT a STL. It basically just implements new & delete (which appears to be enough for the nnsvg & xtext modules).
  • So, you have to choose a STL. In practice, that's libc++ on Clang, and libstd++ on GCC; but, as stated above, they both have custom names, and since we're using a custom build-system, that means extra shenanigans to set that up.
  • And, of course, the STL is never a system lib, it always has to be vendored (which sorta makes sense on Android, actually).

The downside to that last point is that it's probably built w/ debug symbols in the prebuilt TC because it's ginormous (5.5M, compared to my ~1.5M on plain Linux).

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 3, 2022

Also stopped lniking sdcv to a static libz outside of Android (where I'm not even sure why we do that for glib...).

Checking the history leads to #342, which limited the static mess to Android, and #317, which introduced it...

I'm pretty sure it's completely unnecessary nowadays, but, who knows...

Oh, wait, I know why it was done this way. It's to deal with broken early (-ish) API levels where the dynamic loader is dumber than a bag of rocks (c.f., koreader/android-luajit-launcher#283).

@Frenzie
Copy link
Member

Frenzie commented Sep 3, 2022

It's to deal with broken early (-ish) API levels where the dynamic loader is dumber than a bag of rocks

Bionic is my mortal enemy.

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 3, 2022

Made sure to link xtext, cre.cpp & CRe to the STL on Android, juuust in case.

(It doesn't get thrown out by --as-needed, so I'm assuming it's useful).

Also implemented the PRIVATE/PUBLIC switch for CRe linking as discussed earlier ;).

Copy link
Member Author

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 6 files at r1, 3 of 4 files at r5, 1 of 1 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @poire-z)

@NiLuJe NiLuJe merged commit e09b79e into koreader:master Sep 4, 2022
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Sep 4, 2022
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Sep 4, 2022
NiLuJe added a commit to koreader/koreader that referenced this pull request Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants