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

android: don't version shared objects #1907

Merged

Conversation

benoit-pierre
Copy link
Contributor

@benoit-pierre benoit-pierre commented Aug 17, 2024

  • autotools doesn't by default
  • ditto with meson
  • gradle will ignore versioned libraries, and not include them in the APK

This change is Reviewable

@benoit-pierre benoit-pierre marked this pull request as ready for review August 17, 2024 15:49
@benoit-pierre benoit-pierre marked this pull request as draft August 17, 2024 15:49
@Frenzie
Copy link
Member

Frenzie commented Aug 17, 2024

I believe the reason I made them versioned was because unversioned caused the extremely obvious kinds of problems you might expect, particularly because Bionic doesn't support RPATH in conjunction with issues pertaining to SONAME and NEEDED.

If we're lucky I'm wrong in predicting that this will cause terrible breakage primarily on devices we don't own, but if I'm right it should be possible to alternatively fool idiotic Gradle with filenames like libharfbuzz_0.so. (This can be safely changed in SONAME and NEEDED because it's the same length.)

@NiLuJe
Copy link
Member

NiLuJe commented Aug 17, 2024

I imagine as long as the actual in-ELF-header SONAME & SOVER match and this doesn't just change the filename, we should mostly be okay?

@benoit-pierre
Copy link
Contributor Author

Yes, and we use our own loader anyway.

@pazos
Copy link
Member

pazos commented Aug 21, 2024

Came from koreader/koreader#12348 (comment)

Just to point that I've tried it in the past and reverted it in #1272 because koreader/koreader#7040 (comment)

I can't state if something above is true or not, but I'm willing to trust my old self.

@NiLuJe
Copy link
Member

NiLuJe commented Aug 21, 2024

Random stupid idea: does it behave any better if you load everything in the global namespace in dl.lua?

@benoit-pierre
Copy link
Contributor Author

Nope, but I'm not sure it's even correctly supported:

ffi.cdef[[
void *dlopen(const char *filename, int flag);
char *dlerror(void);
const static int RTLD_LOCAL    = 0;
const static int RTLD_LAZY     = 0x00001;
const static int RTLD_NOLOAD   = 0x00004;
const static int RTLD_NODELETE = 0x01000;
]]
-- There's a bit of heinous hackery going on on 32-bit ABIs with RTLD_NOW & RTLD_GLOBAL...
-- c.f., https://android.googlesource.com/platform/bionic/+/refs/heads/master/libc/include/dlfcn.h
if ffi.abi("64bit") then
    ffi.cdef[[
    const static int RTLD_NOW      = 0x00002;
    const static int RTLD_GLOBAL   = 0x00100;
    ]]
else
    ffi.cdef[[
    const static int RTLD_NOW      = 0x00000;
    const static int RTLD_GLOBAL   = 0x00002;
    ]]
end

@NiLuJe
Copy link
Member

NiLuJe commented Aug 21, 2024

It should, that comment was just about bionic mangling the constants depending on the bitness of the arch, but there's another comment later that mentions old API levels not using it implicitly for some things that really really should; and that vaguely rings a bell.

@NiLuJe
Copy link
Member

NiLuJe commented Aug 21, 2024

mangling the constants depending on the bitness of the arch

Or, as the upstream comment says, "/* LP32 has historical ABI breakage. */", lol.

The android linker ignores `DT_RPATH` / `DT_RUNPATH` entries anyway.
- autotools doesn't by default
- ditto with meson
- gradle will ignore versioned libraries, and not include them in the APK
@benoit-pierre benoit-pierre force-pushed the pr/no_android_versioned_shared_objects branch from b64fb2c to bb70127 Compare August 31, 2024 09:51
@benoit-pierre benoit-pierre marked this pull request as ready for review August 31, 2024 09:51
@Frenzie Frenzie merged commit c9cbbca into koreader:master Aug 31, 2024
3 checks passed
@benoit-pierre benoit-pierre deleted the pr/no_android_versioned_shared_objects branch August 31, 2024 20:24
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.

4 participants