search

kivy / python-for-android

Turn your Python application into an Android APK
https://python-for-android.readthedocs.io
MIT License
8.08k stars 1.8k forks source link

[WIP] Remove the libraries loading mechanism #1543

Open opacam opened 5 years ago

opacam commented 5 years ago

This is the third part for the pr #1537 (discussed previously in #1460 and #1534)

Here we remove the library loading mechanism because we don't need that, we only need to load a single library libmain.so. By removing this mechanism we also can remove the PythonUtil.java file because in there, we only have the loading mechanism.

opacam commented 5 years ago

Ei @JonasT, I changed the title of this pr and applied the necessary changes to remove the loading mechanism...but I suspect that this may conflict with your sdl2 update (#1528)...that is why I leave the [WIP] status. Take a look on this when you have time, we are not hurry in this. But your sdl2 update is important for us (we need that as soon as we can). So it's your call to decide if we remove the [WIP] status in here because I think that will be easier to fix conflicts in here than in there :wink:

ghost commented 5 years ago

@opacam the SDL update is ready for merging, it's just that kivy in its current state will break with it. So how fast we can get it in depends on how fast @inclement and @tshirtman will address the remaining kivy issues (these being the windowed/fullscreen startup behavior that needs to obey the env vars, and the config file location which mustn't default to /sdcard/), so it's really kind of out of my hands at this point :woman_shrugging:

But I agree, it might be easier to resolve conflicts here. But I really don't know how fast the SDL2 will get in

opacam commented 5 years ago

Ok, no problem at all, then we leave this in WIP, this will not affect at all at the current functionality of p4a, it's only a clean up. Thanks for the info about your sdl2 update status by the way :smile: I'm sure that @inclement and @tshirtman will fix the kivy issues you mention as soon as they can.

opacam commented 5 years ago

Ui, I forgot to remove the wip status on this...sorry...It should had done some days ago... :grimacing:

@JonasT, can you take a look on this when you have some time?

Note: please look at it carefully, because my C programing level is far to be as good as yours so I can easily make mistakes in here :wink:

inclement commented 5 years ago

The reason the loadLibraries mechanism exists is that it used to be necessary on some devices, i.e. they were not able to dynamically load for some reason. I don't know if it was a library path problem, or an Android version problem, or something else.

Does anyone know what was the case with that, do we have to worry about it any more?

opacam commented 5 years ago

My guess is that the library loading problem was solved at some point after the gradle update was introduced (for sdl2), I think that, right now, the following line solves the problem for bootstraps sdl2, webview and service_only:

https://github.com/kivy/python-for-android/blob/master/pythonforandroid/bootstraps/common/build/templates/build.tmpl.gradle#L62

See also: https://developer.android.com/studio/projects/gradle-external-native-builds

As a side note: I can say that I've been testing with the same device for a quite while, more than a year without any system update and I can say that I had the library loading problem before, and not now (with the same device and the same android version)

ghost commented 5 years ago

@opacam looks promising, I'm all for removing unnecessary code! I'll build my app with this and see what happens, but given this doesn't break anything then this is a really good change :+1:

inclement commented 5 years ago

I'm concerned that if we don't know whether it's actually fixed, merging this could break things in a really annoying way for some users. Do you know why the gradle change would have fixed things, does it actually signal Android to load the libraries in some different way? I don't think the problem related to libraries not being present in the APK.

ghost commented 5 years ago

https://github.com/realm/realm-java/pull/1771 yeah it sounds a little like it is very device dependent.

For what it's worth, does p4a use relinker? Because SDL references it as well: https://github.com/kivy/python-for-android/blob/dec1badc3bd134a9a1c69275339423a95d63413e/pythonforandroid/bootstraps/sdl2/build/src/main/java/org/libsdl/app/SDL.java#L45

Maybe if we used that library, we could let go of our custom loading hacks?

(it sounds like it'd be enough to make sure gradle includes it, SDL appears to be written to automatically pick it up if it's available)

opacam commented 5 years ago

For what it's worth, does p4a use relinker?

As far as I know we don't use relinker

Maybe if we used that library, we could let go of our custom loading hacks?

Oh yes, it may simplify our code..maybe we should test relinker. And if we keep going with this pr maybe we should do that in here...@inclement, what do you thing about that?

inclement commented 5 years ago

I have nothing against using relinker, it looks like there is no real cost to doing so.

It does say in its README "Note that this library fixes intermittent link errors; if you get an error every time you use your app, you may have a configuration issue". I think the errors we were seeing were more the latter, although there may also be intermittent problems I'm not really aware of that this would fix. Most users would have no idea what happened if the app randomly failed, so it could definitely be happening.

Most of all though, I'm reluctant to change the current code without at least a test case that it fixes (i.e. an actual device with problems before but not after), or some specific documented change in the Android build tools that explains why the issue occurred and when it was fixed.

ghost commented 5 years ago

Well, the jniLibs.srcDir dir definitely looks like it could change things. If you check out the docs link you're definitely supposed to set it to where System.loadLibrary() should load things from, so if that was missing previously I'd expect that to lead to trouble. But of course it's impossible to be sure about that being really the only issue without retesting a wider range of devices

inclement commented 5 years ago

My understanding was that jniLibs.srcDir doesn't affect where System.loadLibrary loads things from on the device, it just tells gradle what folders to pull .so files from to package into the APK. Am I wrong about that? I also didn't check yet, did we add jniLibs.srcDir to the gradle project only some time after the original gradle support PR, or some time afterwards?

The change in the current code that made things start working was specifically to explicitly add System.loadLibrary for each different library in turn, because calling System.loadLibrary only for the main one wasn't successfully automatically finding the others even though they were present in the expected place. That's consistent with the System.loadLibrary failures in the realm issue you linked, except that it was happening every time for some users rather than being due to System.loadLibrary flakiness as mentioned in the relinker docs.

inclement commented 5 years ago

https://github.com/kivy/python-for-android/pull/1071/files#diff-e6d6c9254ef94f6f1ea119b95c029668R55

^^ looks like jniLibs.srcDir was set even in the original gradle PR, so if @opacam is correct that the loading issue was solved only some time after the original gradle introduction, we probably can't associate it with that.

ghost commented 5 years ago

because calling System.loadLibrary only for the main one wasn't successfully automatically finding the others even though they were present in the expected place

Did it lead to libmain.so loading failures with errors that the dependencies cannot be found? That sounds like ReLinker might help

If they just weren't loaded at all and that lead to missing symbols, then maybe it's rather a toolchain failure/build problem with the dynamic linking...?

I don't think we'll get anywhere unless someone can recall a device where this originally broke, or puts in the time to find one

inclement commented 5 years ago

@JonasT I think what we actually saw was libpython.so failing to load the openssl and sqlite libs, but I don't remember for sure. That might just be down to the ordering of the libs loading.

I do think it's very likely that any of these things might solve the issue, if it still happens at all, but it was a nightmare to fix/understand last time and I just really don't want to introduce linking issues even if it's a very rare problem. It could even be something to do with old android versions that aren't being used much any more.

I guess I'm really saying that I think we should leave it for now, partly because I don't think PythonUtil.java causes any maintenance problems even if it would be better to get rid of it. However, if everyone else wants to proceed, I think we should introduce relinker at least.

opacam commented 5 years ago

Well...after the conversation...it seems risky to remove the loading mechanism plus is harmless if you don't suffer of the loading problem...so...I was thinking:

What if we do a removal in steps?

  1. remove a library of the loading mechanism (sqlite3 I was thinking)
  2. wait some days/weeks/months, staying alert of any complain about sqlite3
  3. if we start to get complains about sqlite3 loading problems it will mean that we need the loading mechanism and then we quickly add sqlite3 again or we test relinker to see if we can use that...and if not ... :scissors:

Maybe it's a crazy idea...

ghost commented 5 years ago

That could mean it's broken for some end users for months. I don't think that's a good idea if we don't have some basic clue about what's going on

opacam commented 5 years ago

That could mean it's broken for some end users for months.

:scream:, no please, If we merge this and we really need the libraries loading mechanism...then that will be the situation for us and then will be hard to fix it, as @inclement pointed, plus we could have issues with multiple libraries (not only sqlite3) and it may be related with this or not...

I don't think that's a good idea if we don't have some basic clue about what's going on

That is the key, we don't have an idea what is going on (at least myself), that is why i propose this, a controlled :bomb: (we only have two issues regarding sqlite3...by the way...so many thanks @inclement :heart:...you closed a lot of the issues and you too @JonasT :heart:)...so...it would be easy to find any issue about that. Plus I think that we only affect the master branch (well...that can affect a lot of people but I'm confident that we would have some notice...of course I could be wrong) and we can provide a solution for this end user via discord or via issues section. Anyway, I agree with you @JonasT, we can cause some problems to some end users...but it will cause less troubles than merging the pr...

Maybe we should put this in wip again for starters?

inclement commented 5 years ago

Plus I think that we only affect the master branch

We're actually experimenting with having the master branch be the default for everyone, since that's consistently been a better experience recently.

Overall, I really support just not messing with that right now. There's no need to experiment on the users because this code isn't actually really a problem, there is very little cost to maintaining PythonUtils.java because it doesn't ever change.

opacam commented 5 years ago

Overall, I really support just not messing with that right now. There's no need to experiment on the users because this code isn't actually really a problem, there is very little cost to maintaining PythonUtils.java because it doesn't ever change.

@inclement, I totally agree with you

I close this pr because I don't see that we can safely remove the library loading mechanism

!!!Soo many thanks for your contributions in this pr @JonasT and @inclement!!!

inclement commented 5 years ago

@opacam If you'd like to leave this open as something for the future, that would be fine. There's nothing wrong with the idea of the change, and it would log that there's something to be done here, but also log the reason it hasn't been merged yet. I feel bad for the work to be lost, even if I think it's the safest way for now :)

opacam commented 5 years ago

Again, you are right, I will leave this open...so we have a record that we have something to look into and it's not solved...hopefully the time will give us the answer :wink:

¡¡¡Thanks @inclement!!!