Skip to content

Conversation

@rtibbles
Copy link
Member

@rtibbles rtibbles commented Mar 18, 2022

  • Removes PyEverywhere for building the app
  • Remove PyEverywhere for controlling the runtime
  • Updates Python for Android to the latest released version, with minimal patches on top to ensure continued function
  • Simplifies the P4A bootstrap to control URL loading from Python
  • Lets Kolibri find unused ports rather than hardcoding
  • Lints the codebase
  • Adds a Github Action for creating debug builds of the APK

Fixes #63
Fixes #90
Fixes #93
Fixes #94
Fixes #96
Fixes #97


@rtibbles rtibbles requested review from DXCanas and jamalex March 18, 2022 03:31
@rtibbles rtibbles marked this pull request as draft March 18, 2022 15:45
@rtibbles rtibbles marked this pull request as ready for review March 18, 2022 15:57
@MBKayro
Copy link

MBKayro commented Mar 21, 2022

Thank you @rtibbles works perfectly :)

Copy link
Member

@DXCanas DXCanas left a comment

Choose a reason for hiding this comment

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

I think I've commented on the bits I can be useful on. No functional changes to request, but do have some questions and non-blocking requests for comments here and there 🙃

on:
workflow_dispatch:
# Inputs the workflow accepts.
inputs:
Copy link
Member

Choose a reason for hiding this comment

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

Fancy

options:
- 32bit
- 64bit
workflow_call:
Copy link
Member

Choose a reason for hiding this comment

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

Interesting distinction between call and dispatch

Copy link
Member Author

Choose a reason for hiding this comment

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

Dispatch is for manually triggering, call is for triggering by a different workflow.

Comment on lines +42 to +45
- uses: actions/checkout@v2
if: ${{ !inputs.ref }}
- uses: actions/checkout@v2
if: ${{ inputs.ref }}
Copy link
Member

Choose a reason for hiding this comment

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

Hwoaw

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an irritating wrinkle that we have to do for workflow_call as it gets run in the context of the calling repository, not this repository.

@@ -0,0 +1,28 @@
name: Linting
Copy link
Member

Choose a reason for hiding this comment

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

this'll be fun

Comment on lines +1 to +9
# Ensure that Django's SQLite backend module is included
sqlite3/*
lib-dynload/_sqlite3.so
unittest/*
wsgiref/*
lib-dynload/_csv.so
lib-dynload/_json.so
# Django REST framework has a dependency on the Django test module
django/test/*
Copy link
Member

Choose a reason for hiding this comment

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

I take it this is P4A's allowlist, for use during copy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - this allows things to be copied even if they are in the blocklist.

Comment on lines +1 to +49
# remove some assorted additional plugins
kolibri/plugins/demo_server/*

# remove python2-only stuff
kolibri/dist/py2only/*

# Remove cextensions
kolibri/dist/cext*

# remove source maps
*.js.map

# remove unused translation files from django and other apps
kolibri/dist/rest_framework/locale/*
kolibri/dist/django_filters/locale/*
kolibri/dist/mptt/locale/*

kolibri/dist/django/contrib/admindocs/locale/*
kolibri/dist/django/contrib/auth/locale/*
kolibri/dist/django/contrib/sites/locale/*
kolibri/dist/django/contrib/contenttypes/locale/*
kolibri/dist/django/contrib/flatpages/locale/*
kolibri/dist/django/contrib/sessions/locale/*
kolibri/dist/django/contrib/humanize/locale/*
kolibri/dist/django/contrib/admin/locale/*

# remove some django components entirely
kolibri/dist/django/contrib/gis/*
kolibri/dist/django/contrib/redirects/*
kolibri/dist/django/conf/app_template/*
kolibri/dist/django/conf/project_template/*
kolibri/dist/django/db/backends/postgresql_psycopg2/*
kolibri/dist/django/db/backends/postgresql/*
kolibri/dist/django/db/backends/mysql/*
kolibri/dist/django/db/backends/oracle/*
kolibri/dist/django/contrib/postgres/*

# remove bigger chunks of django admin (may not want to do this)
kolibri/dist/django/contrib/admin/static/*
kolibri/dist/django/contrib/admin/templates/*

# other assorted testing stuff
*/test/*
*/tests/*
kolibri/dist/tzlocal/test_data/*

# remove some unnecessary apps
kolibri/dist/redis_cache/*
kolibri/dist/redis/*
Copy link
Member

Choose a reason for hiding this comment

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

niice. How's the .apk size compare to the old process?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a few MB smaller, it seems. I think that's probably mostly from dropping all the PyEverywhere runtime code.

Comment on lines +1 to +3
cython
virtualenv
git+https://github.com/learningequality/python-for-android@350d7158a0a35f578a95d50de969101579bbdc4f#egg=python-for-android
Copy link
Member

Choose a reason for hiding this comment

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

👌

--env P4A_RELEASE_KEYSTORE_PASSWD \
--env P4A_RELEASE_KEYALIAS_PASSWD \
--env ARCH \
--env ANDROID_HOME=${CONTAINER_HOME}/.local/android \
Copy link
Member

Choose a reason for hiding this comment

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

👌

build_number = build_base_number + 2 * int(buildkite_build_number) + increment_for_64bit
print(build_number)
build_number = (
build_base_number + 2 * int(buildkite_build_number) + increment_for_64bit
Copy link
Member

Choose a reason for hiding this comment

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

oof.

I recall some recent discussions around changing this up. We holding off on that?

Also, this still depends on BUILDKITE_BUILD_NUMBER - are we setting that manually via GHA? Or are we skipping this bit altogether? Noting that this is how Android determines whether or not to "upgrade" and installed app.

Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment I'm only building the debug build in Github Actions - resolving this seemed like a blocker to being able to build the non-debug but develop builds for PRs.

@rtibbles rtibbles merged commit 947fa84 into develop Apr 4, 2022
@rtibbles rtibbles deleted the vendor_build branch April 4, 2022 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants