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.

Sign up for GitHub

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

Jump to bottom

fix native libraries search path #441

Merged
merged 1 commit into from Oct 9, 2023
Merged

fix native libraries search path #441

merged 1 commit into from Oct 9, 2023

Conversation

benoit-pierre
Copy link
Contributor

@benoit-pierre benoit-pierre commented Oct 7, 2023 •
edited by Frenzie

Cf. koreader/koreader#10533.

This change is Reviewable

@benoit-pierre benoit-pierre mentioned this pull request Oct 7, 2023
Closed
Frenzie
Frenzie approved these changes Oct 7, 2023
View reviewed changes
Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

lgtm in principle, but are you sure they have to be excluded?

NiLuJe
NiLuJe reviewed Oct 7, 2023
View reviewed changes
assets/android.lua
android.dl.library_path = android.nativeLibraryDir..":"..
android.dir..":"..android.dir.."/libs:"..
"/lib:/system/lib:/lib/lib?.so:/system/lib/lib?.so"
string.gsub("@:@/lib?.so", "@", android.dl.system_libdir)
Copy link
Member

Choose a reason for hiding this comment

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

I don't recall why the /lib & /lib/lib stuff was in there in the first place, but is it safe to drop those?

(For context, it might have been for broken Chinese ROMs or something).

Copy link
Member

Choose a reason for hiding this comment

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

(FWIW, I'm in the "fuck it" train if that was the case, just wanted to know if you had thoughts on that ;)).

@NiLuJe
Copy link
Member

NiLuJe commented Oct 7, 2023

but are you sure they have to be excluded?

Yep, see the comments, the dynamic loader will bounce you if you try ;).

@hugleo
Copy link
Contributor

hugleo commented Oct 7, 2023 •
edited

Here the /lib/lib commit: 64caed0

An here the lib commit 9c95c44

@benoit-pierre
fix native libraries search path
bf30bee 
The canonical path for native system libraries is `/system/lib` for 32bits and `/system/lib64`
for 64bits (cf. https://source.android.com/docs/core/permissions/namespaces_libraries).
@benoit-pierre
Copy link
Contributor Author

Here the /lib/lib commit: 64caed0

An here the lib commit 9c95c44

Going back further: 7d0c1fc

So /lib was already there, no idea why. The canonical directory is /system/lib on 32bits and /system/lib64 for 64bits, with the possible addition of /vendor/lib / /vendor/lib64.

I checked with the oldest available emulator image for x86 (API level 10, Gingerbread), and /system/lib is already used, no /lib.

Without a clear example of "broken Chinese ROMs or something", I would stick to the default.

@benoit-pierre benoit-pierre changed the title fix native libraries search path on 64bits systems fix native libraries search path Oct 8, 2023
@NiLuJe NiLuJe merged commit c553909 into koreader:master Oct 9, 2023
1 check passed
@hugleo hugleo mentioned this pull request Oct 9, 2023
Closed
@benoit-pierre benoit-pierre deleted the pr/fix_64bits_system_libdir branch October 9, 2023 16:22
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Oct 9, 2023
@NiLuJe
Bump android/luajit-launcher
28370d0 
koreader/android-luajit-launcher#441

Fix koreader#10533, koreader#10150, koreader#10587, koreader#10501

koreader/android-luajit-launcher#442
NiLuJe added a commit to koreader/koreader that referenced this pull request Oct 9, 2023
@NiLuJe
Bump android/luajit-launcher
4eac18f 
koreader/android-luajit-launcher#441

Fix #10533, #10150, #10587, #10501

koreader/android-luajit-launcher#442
@NiLuJe NiLuJe mentioned this pull request Oct 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NiLuJe NiLuJe NiLuJe left review comments

@Frenzie Frenzie Frenzie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@benoit-pierre @NiLuJe @hugleo @Frenzie