[CORE UPDATE - PART II] Fix bootstraps for webview and service_only #1541
Conversation
To make use of it on non sdl2 bootstraps when those are adapted to be gradle compatible
This requires to move some files to a new location, but the files remain untouched for now.
This requires to move some files to a new location, but the files remain untouched for now.
To fix the bootstraps later, we will use the sdl2's bootstrap files as a base because they are already prepared to support the new python's build system.
To fix the bootstraps later.
Based on sdl2 bootstrap.
Based on webview's bootstrap.
This is almost the same than the equivalent for sdl2 bootstrap, without some hardcoded python2 entries (we don't need them anymore...so removed) and without the sdl2 libs
…enables some missing features Note: Some files has been removed because we will use the generic ones of bootstraps/common
…es some missing features (aars) Note: Some files has been removed because we will use the generic ones of bootstraps/common
This is almost a clone from one of @inclement's test apps, with the addition of a function which prints the current date/time.
| PythonUtil.loadLibraries(getFilesDir()); | ||
| String app_root = new String(getAppRoot()); | ||
| File app_root_file = new File(app_root); | ||
| PythonUtil.loadLibraries(app_root_file); |
There was a problem hiding this comment.
This, together with the entire pull request https://github.com/kivy/python-for-android/pull/1543/files , is for me definitely the most critical part.
I like the loading mechanism as a general construct since it seems well enough done, however, I don't understand why it is necessary. Shouldn't this all just work through dynamic linking? Because this and the entire code of https://github.com/kivy/python-for-android/pull/1543/files do add quite a lot of additional complexity in the loading process, which to me at a first glance shouldn't be needed, so I'd like to understand what is going on and if we really need & want this
Ignore this remark, I misread the code. It's just the existing library loading moved to a different place / adapted slightly for the new locations
|
@opacam could you possibly take out the library loading related things of this pull request? It is very useful to have the bootstraps repaired, but since I think the library loading really needs some deeper investigation first, I would suggest that should be merged in later (if it turns out to be necessary). So if you split it up from this one, we could move ahead with all the awesome bootstrap refactoring which definitely looks like stuff we want to have in as soon as possible |
Yes, Edit: reviewing the commits I see that the calls to load libs were already there, I changed the lines because I unified the PythonUtil.java based on the one from sdl2 bootstrap, and the calls to PythonUtil.loadLibraries needed to be adapted to the new circumstance. Don't confuse this changes with the ones made in the original pr with the new loading mechanism (in there I pass PythonActivity not a directory path like here), I rewrote the stuff without the new mechanism so this calls seems good to me. Maybe we should test without any loading mechanism but I think that better not to do it here..don't you agree? |
I may have misunderstood, are you saying you merely adapted an existing mechanism? Let me reread the changes then, maybe I didn't see that properly |
|
Sorry, it appears this is my mistake! It really looks like you just moved the existing somewhat hardcoded library list to another place, but it's essentially the same thing. Looks good from my side then 👍 |
…nto python-core-bootstraps
| if get_bootstrap_name() == "sdl2": | ||
| args.add_activity = args.add_activity or [] | ||
| args.activity_launch_mode = args.activity_launch_mode or '' | ||
| # if get_bootstrap_name() == "sdl2": |
There was a problem hiding this comment.
Minor: I would simply delete that comment
| args.activity_launch_mode = args.activity_launch_mode or '' | ||
| # if get_bootstrap_name() == "sdl2": | ||
| args.add_activity = args.add_activity or [] | ||
| args.activity_launch_mode = args.activity_launch_mode or '' |
There was a problem hiding this comment.
Minor: I know it was already that way before, but people misuse this as ternary operator, this is not the python ternary operator
There was a problem hiding this comment.
So with Python-style ternary that would be:
args.add_activity = args.add_activity if args.add_activity else []
args.activity_launch_mode = args.activity_launch_mode if args.activity_launch_mode or ''
| @@ -708,7 +687,7 @@ def _read_configuration(): | |||
| if args.meta_data is None: | |||
| args.meta_data = [] | |||
|
|
|||
| if args.services is None: | |||
| if (not hasattr(args, 'services')) or args.services is None: | |||
There was a problem hiding this comment.
minor: if getattr(args, 'service', None) is None
We merged by mistake the mentioned pr with some minor issues that should had been fixed in there. We solve that in here. ¡¡¡Thanks @AndreMiras!!!
We merged by mistake the mentioned pr with some minor issues that should had been fixed in there. We solve that in here. ¡¡¡Thanks @AndreMiras!!!
This is the second part for the pr #1537 (discussed previously in #1460 and #1534)
I created a parallel branch with the part I in the current state and the part II, with two more commits that allow us to view if the build is success. The first commit adds travis tests. The second one applies the docker image change (from ubuntu to debian). This last one is mandatory to pass the webview test...we need python 3.7 and ubuntu ships 3.6. I put this on here for all us to know and because we already started a discussion about that in the previous pr. We don't need those changes in master branch because the webview tests will not be enabled but I think that this information can safe time to anyone who wants to build those tests in ci.
Here is the travis build with the tests enabled: https://travis-ci.org/opacam/python-for-android/builds/469953483
For now I leave out the libraries loading mechanism, it should be the part 3, but for that we need to merge this pr because we touch files modified in here so maybe better do it after merged this don't you think?
If you think that we must implement in here let me know and I will apply the changes on the top.
Additional info for future reference: