Skip to content

Support multiarch in webview bootstrap #2596

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
Merged
merged 2 commits into from
May 22, 2022
Merged

Support multiarch in webview bootstrap #2596

merged 2 commits into from
May 22, 2022

Conversation

dbnicholson
Copy link
Contributor

This is essentially exactly the same as what the sdl2 bootstrap is
doing.

@dbnicholson
Copy link
Contributor Author

We've been using this to run APKs and distribute AABs with our webview based app for a couple weeks now and it seems to work fine. Are there any tests for this code? I couldn't tell.

misl6
misl6 reviewed May 17, 2022
View reviewed changes
Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

Nice! I did not test it yet, but the code looks good.

Regarding the tests:
IMHO the best test that we can add ATM, is to extend the ubuntu_build_apk, macos_build_apk, ubuntu_build_aab, macos_build_aab jobs to include the webview bootstrap. That allows us to know if the apk or aab artifact is correctly built for all the boostraps, and even test the produced artifacts on a real device without doing the build manually.

We can still merge the PR and address these tests later, otherwise, feel free to improve our test suite and even ping me on our Discord #dev channel to discuss about it :)

@dbnicholson
Copy link
Contributor Author

Regarding the tests: IMHO the best test that we can add ATM, is to extend the ubuntu_build_apk, macos_build_apk, ubuntu_build_aab, macos_build_aab jobs to include the webview bootstrap. That allows us to know if the apk or aab artifact is correctly built for all the boostraps, and even test the produced artifacts on a real device without doing the build manually.

We can still merge the PR and address these tests later, otherwise, feel free to improve our test suite and even ping me on our Discord #dev channel to discuss about it :)

I've been playing with on_device_tests for #2464, so I think I can manage it here. Do you think the added build should continue using numpy or just the basic webview requirements?

AndreMiras
AndreMiras previously approved these changes May 17, 2022
View reviewed changes
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

LGTM, same thing, code looks good, but I haven't tested it

@dbnicholson dbnicholson force-pushed the webview-multi-arch branch 3 times, most recently from c87cfcb to 5e74313 Compare May 18, 2022 12:11
@dbnicholson
Copy link
Contributor Author

I think this is working now to build the testapp with both the sdl2 and webview bootstraps and save them as artifacts. When I run the webview APK it seems fine. However, the unittests fail:

05-18 09:16:59.727  6072  6194 I python  : unittest result is:
05-18 09:16:59.727  6072  6194 I python  : .......E
05-18 09:16:59.727  6072  6194 I python  : ======================================================================
05-18 09:16:59.727  6072  6194 I python  : ERROR: test_run_module (tests.test_requirements.PyjniusTestCase)
05-18 09:16:59.727  6072  6194 I python  : ----------------------------------------------------------------------
05-18 09:16:59.727  6072  6194 I python  : Traceback (most recent call last):
05-18 09:16:59.727  6072  6194 I python  :   File "/home/user/app/testapps/on_device_unit_tests/build/bdist.android-x86/test_app/tests/test_requirements.py", line 51, in test_run_module
05-18 09:16:59.727  6072  6194 I python  :   File "/home/user/.local/share/python-for-android/build/python-installs/bdist_unit_tests_app/x86_64/jnius/reflect.py", line 229, in autoclass
05-18 09:16:59.727  6072  6194 I python  :   File "jnius/jnius_export_func.pxi", line 26, in jnius.jnius.find_javaclass
05-18 09:16:59.727  6072  6194 I python  :   File "jnius/jnius_utils.pxi", line 91, in jnius.jnius.check_exception
05-18 09:16:59.727  6072  6194 I python  : jnius.jnius.JavaException: JVM exception occurred: Didn't find class "org.kivy.android.PythonActivity" on path: DexPathList[[directory "."],nativeLibraryDirectories=[/system/lib64, /system_ext/lib64, /system/lib64, /system_ext/lib64]] java.lang.ClassNotFoundException

I don't understand that since the app is already loading the PythonActivity class earlier. I don't think that error is related to this change, though.

@misl6
Copy link
Member

misl6 commented May 18, 2022

I think this is working now to build the testapp with both the sdl2 and webview bootstraps and save them as artifacts. When I run the webview APK it seems fine. However, the unittests fail:

05-18 09:16:59.727  6072  6194 I python  : unittest result is:
05-18 09:16:59.727  6072  6194 I python  : .......E
05-18 09:16:59.727  6072  6194 I python  : ======================================================================
05-18 09:16:59.727  6072  6194 I python  : ERROR: test_run_module (tests.test_requirements.PyjniusTestCase)
05-18 09:16:59.727  6072  6194 I python  : ----------------------------------------------------------------------
05-18 09:16:59.727  6072  6194 I python  : Traceback (most recent call last):
05-18 09:16:59.727  6072  6194 I python  :   File "/home/user/app/testapps/on_device_unit_tests/build/bdist.android-x86/test_app/tests/test_requirements.py", line 51, in test_run_module
05-18 09:16:59.727  6072  6194 I python  :   File "/home/user/.local/share/python-for-android/build/python-installs/bdist_unit_tests_app/x86_64/jnius/reflect.py", line 229, in autoclass
05-18 09:16:59.727  6072  6194 I python  :   File "jnius/jnius_export_func.pxi", line 26, in jnius.jnius.find_javaclass
05-18 09:16:59.727  6072  6194 I python  :   File "jnius/jnius_utils.pxi", line 91, in jnius.jnius.check_exception
05-18 09:16:59.727  6072  6194 I python  : jnius.jnius.JavaException: JVM exception occurred: Didn't find class "org.kivy.android.PythonActivity" on path: DexPathList[[directory "."],nativeLibraryDirectories=[/system/lib64, /system_ext/lib64, /system/lib64, /system_ext/lib64]] java.lang.ClassNotFoundException

I don't understand that since the app is already loading the PythonActivity class earlier. I don't think that error is related to this change, though.

The issue seems to be tracked here: #2533, so it's definitely something that appeared before your changes.

misl6
misl6 previously approved these changes May 18, 2022
View reviewed changes
Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

LGTM, nice!
Left a minor comment, feel free to address it or not :)

AndreMiras
AndreMiras previously approved these changes May 18, 2022
View reviewed changes
@dbnicholson dbnicholson dismissed stale reviews from AndreMiras and misl6 via 2339d95 May 20, 2022 17:47
@dbnicholson dbnicholson force-pushed the webview-multi-arch branch from 2339d95 to 831e275 Compare May 20, 2022 20:13
@dbnicholson
Support multiarch in webview bootstrap
eb33352 
This is essentially exactly the same as what the sdl2 bootstrap is
doing.
@dbnicholson dbnicholson force-pushed the webview-multi-arch branch from 831e275 to 598cc79 Compare May 20, 2022 21:50
@dbnicholson
Copy link
Contributor Author

I think this is working as suggested. I actually think this will increase build time since each runner is going to have to rebuild the docker image and the entire p4a distribution.

misl6
misl6 requested changes May 21, 2022
View reviewed changes
Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

Nice! Not your fault, but we need to update the matrix in order to have a run on macos-latest, left a couple of suggestions.

Regarding the docker image build time: Doing some caching will save us a lot of CI time, and is certainly something we should improve in the future 😄

@dbnicholson
Add webview app make targets and artifact builds
e37311d 
This should help exercise the webview bootstrap in CI and allow testing
a webview app using the built artifacts.
@dbnicholson dbnicholson force-pushed the webview-multi-arch branch from af14e87 to e37311d Compare May 21, 2022 19:43
misl6
misl6 approved these changes May 22, 2022
View reviewed changes
Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@misl6 misl6 merged commit b7146e8 into kivy:develop May 22, 2022
@dbnicholson dbnicholson deleted the webview-multi-arch branch May 23, 2022 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@misl6 misl6 misl6 approved these changes

@AndreMiras AndreMiras AndreMiras left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@dbnicholson @misl6 @AndreMiras