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.

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

Native code updates for early 2023 #952

Merged
merged 34 commits into from
Apr 18, 2023
Merged

Native code updates for early 2023 #952

merged 34 commits into from
Apr 18, 2023

Conversation

shankari
Copy link
Contributor

Bump up the versions of all dependencies
So that we can check in our updates in bite-size chunks without having a giant
PR that is hard to look into later
👷 Enable the CI for `maint_upgrade_*` branches as…
Per e-mission/e-mission-docs#838 (comment)
This is the first step in which I remove all the controls related to it

```
$ grep -ri notify www/js/control | wc -l
       0
$ grep -ri notify www/templates/control/ | wc -l
       0
```

Testing done:
Profile screen loads correctly
This includes both copies (incident and survey)

Testing done:
- No instances left

```
$ grep -ri posttrip.prompt www/js
$ grep -ri PostTripAutoPrompt www/js
$
```

- UI loaded without errors
After removing all the uses of the package, remove the package itself
🔥 Remove the transition notify plugin
- Use opcode instead of email throughout
- Use the correct plugin name
- Do not prepopulate the "to" email in the "download JSON dump"

This makes the UI consistent
e-mission/cordova-jwt-auth@b0d29c1
Remove all `console.log` and `ionicPopup.alert`, which didn't actually work properly
and replace them with the recommended `Logger.displayError`

This should allow us to see the error preventing the token from showing up on iOS

```
$ grep --after-context=1 "function(error)" www/js/control/general-settings.js
        }, function(error) {
            Logger.displayError("While getting connect url", error);
--
        }, function(error) {
            Logger.displayError("while getting opcode, ",error);
--
        }, function(error) {
            Logger.displayError("while getting current state", error);
--
                }, function(error) {
                    Logger.displayError("while clearing user cache, error ->", error);
--
        }, function(error) {
            Logger.displayError("while invalidating cache, error->", error);
--
        }).catch(function(error) {
            Logger.displayError("Error while forcing sync", error);
--
        }, function(error) {
            Logger.displayError("Error reading consent document from cache", error)
```
Rename and simplify the auth plugin
As outlined in e-mission/e-mission-docs#838 (comment)
- we are not using the ionic-deploy functionality any more. It was always unclear whether that constituted "code download"
- it is not currently authorized by NREL cyber
- having separate branches without a lot of oversight just led to a lot of divergent, throw-away code
- NREL cyber will not allow us to deploy code written by others without review and merge anyway
- so we are going to switch to a small number of features, checked into master as modules, and enabled/disabled via a simple static config file

This is the last of the three cleanups in the issue.
The other two were fixed in
#925
and
e-mission/cordova-jwt-auth#46

We can now start the real API upgrade
🔥 Remove the cordova ionic plugin and the ionic deploy update check
- android CLI version to the latest
- android sdk packages to the latest
- cordova, ionic and bower to the latest
- all non-OpenPATH plugins to the latest versions

Some of these (`cordova-plugin-badge`/`phonegap-plugin-barcodescanner`) ran into an issue with using `compile` when they actually needed `implementation` for the new gradle.

The badge is a dependency of the local notification plugin, so I migrated from `timkellypa` to `levanax` who has some commits that have fixed it and some other issues. This fixes it for the local notification plugin but not for the `cordova-plugin-badge` which is pulled in as a dependency from elsewere

I fixed that and the barcodescanner by adding a new hook that renames `compile` to `implementation` before build.

Testing done: builds
If not, even the install fails with

```
BEGIN: About to start android SDK download
Error: A JNI error has occurred, please check your installation and try again
Exception in thread "main" java.lang.UnsupportedClassVersionError: com/android/prefs/AndroidLocationsProvider has been compiled by a more recent version of the Java Runtime (class file version 55.0), this version of the Java Runtime only recognizes class file versions up to 52.0
	at java.lang.ClassLoader.defineClass1(Native Method)
	at java.lang.ClassLoader.defineClass(ClassLoader.java:756)
	at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
	at java.net.URLClassLoader.defineClass(URLClassLoader.java:473)
	at java.net.URLClassLoader.access$100(URLClassLoader.java:74)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:369)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:363)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.net.URLClassLoader.findClass(URLClassLoader.java:362)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
	at java.lang.Class.getDeclaredMethods0(Native Method)
	at java.lang.Class.privateGetDeclaredMethods(Class.java:2701)
```
Bump up versions of the dev chain and all other plugins
And change the existing instructions to only apply to android 11
Bump up the minSDK to API 23 (Marshmallow, Android 6)
    - I am not sure why this was API 22; we are certainly not testing on that version

Bump the targetSDK to API 32, android 12 (required for Google Play)
Switch the push and local notification plugins to ones that support the new trampoline functionality
https://developer.android.com/about/versions/12/behavior-changes-12#notification-trampolines

The push plugin is a "dev" version but seems to have all android fixes
It also bumps up the iOS version so that we can avoid the "non-public selectors" error
havesource/cordova-plugin-push#106
e-mission/e-mission-docs#838 (comment)

The push plugin had kotlin build errors that were fixed by adding the gradle preferences
havesource/cordova-plugin-push#218 (comment)

The local notification is a new fork maintained by a new author
which requires us to add the new `AndroidLaunchMode` preference.

The new plugins already have the compile -> implementation fix
So removing the hook added in 6de1f4c

Testing done:
- builds
- generated a local notification by turning off location permissions
- clicking on the notification opened the status screen
To avoid reintroducing the hook after the following failure

```
* Where:
See https://docs.gradle.org/7.4.2/userguide/command_line_interface.html#sec:command_line_warnings
Script '/Users/runner/work/e-mission-phone/e-mission-phone/platforms/android/phonegap-plugin-barcodescanner/emission-barcodescanner.gradle' line: 9

* What went wrong:
A problem occurred evaluating script.
> Could not find method compile() for arguments [{name=barcodescanner-release-2.1.5, ext=aar}] on object of type org.gradle.api.internal.artifacts.dsl.dependencies.DefaultDependencyHandler.

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
```
The new QR code scanning plugin that we added in
a8ca751
had a dependency on `cordova-plugin-add-swift-support`
This is a companion commit to
e-mission/e-mission-data-collection@60f6fa4
from
e-mission/e-mission-data-collection#202

This allows us to start foreground services from the background
Which allows us to keep our original model and kick the can down the road
e-mission/e-mission-docs#838 (comment)

I have explored options to do it the right way, but am concerned about testing
overhead given our short time frame.

This change:
- adds interface functions to check and fix battery optimizations
- implements the check with the powermanager call
- implements the fix by launching the optimization window

Testing done:
- Turn off the data optimizations
- Kill the app (including the foreground service) using adb
e-mission/e-mission-docs#838 (comment)

- Send the initialize transition using adb
    - Foreground service is restarted
e-mission/e-mission-docs#838 (comment)
The `google_apis_playstore` images are signed and cannot be rooted
If we want to send broadcast messages using adb for testing, we need root support
e-mission/e-mission-docs#838 (comment)

Add a couple of rootable system images as well so we can test appropriately
Since this is going to be a testable version
Last few changes + our plugin bump ups
Bump up version number for the auth plugin
We tried to migrate from `phonegap-plugin-barcodescanner` to the better
supported `cordova-plugin-barcode-qrscanner`. However, I was not able to get
the new plugin to work. The OS indicated that the app was accessing the camera,
but we were not able to see the preview, even after I added a `show` to the code.

e-mission/e-mission-docs#838 (comment)
e-mission/e-mission-docs#838 (comment)

There doesn't seem to be an option to file issues or to communicate with the creator.
So let's fall back to the older `phonegap-plugin-barcodescanner`
We restore the `android_change_compile_implementation` hook so that we can
change the `compile` to `implementation` and get the plugin to compile.

Bonus fix: Add the `NSCameraUsageDescription` via the config.xml so that we
don't get a permission issue while running on iOS.

Testing done:
- Builds on both android and iOS
- Scanning launches on both android and iOS
- QR codes are scanned on both android and iOS
Revert the QR code plugin to the one that we used before
Since it is no longer experimental
- Native code change to stop checking for it
- UI changes to remove the button from the control screen
- Remove the toggle function from the javascript
- Remove the translation from i18n
Final set of minor fixes for the new release
@shankari shankari merged commit b409bc0 into master Apr 18, 2023
@shankari shankari deleted the maint_upgrade_2023 branch April 18, 2023 06:05
shankari added a commit that referenced this pull request Apr 19, 2023
This file is not present in master, so we didn't remove the update check and
posttrip prompt from it while implementing #952

Notably
c13f663
cd5cebc

So let's remove the dependencies from it as well
shankari added a commit to e-mission/e-mission-devapp that referenced this pull request Apr 23, 2023
To be consistent with 2023 phone upgrades
Related issue: e-mission/e-mission-docs#838
Related PR: e-mission/e-mission-phone#952

Since the rest of the app is not the same, I applied the changes by diffing the
config.xml and package.json and the setup directory with the corresponding
locations in the e-mission-phone repo

Additional minor changes:
+ comment out the `configure_xml_and_json` step, since we don't have the
cordovabuild versions in this release
+ change the package name to `edu.berkeley.eecs.emission.devapp`
+ bump up the versions

Testing done:
- devapp builds successfully
- after making phone app changes, devapp displays phone app successfully
shankari added a commit to shankari/e-mission-phone that referenced this pull request Apr 23, 2023
Related issue: e-mission/e-mission-docs#838
Related PR for cordovabuild: e-mission#952
Related PR for devapp: e-mission/e-mission-devapp#18

Main changes:
- need to add "Ventura" to the list of releases to fix:

    ```
    /Users/kshankar/e-mission/phone-rciti-branch/node_modules/macos-release/index.js:27
        const [name, version] = nameMap.get(release);
                                ^

    TypeError: undefined is not iterable (cannot read property Symbol(Symbol.iterator))
        at macosRelease (/Users/kshankar/e-mission/phone-rciti-branch/node_modules/macos-release/index.js:27:26)
        at osName (/Users/kshankar/e-mission/phone-rciti-branch/node_modules/os-name/index.js:21:18)
        at new Insight (/Users/kshankar/e-mission/phone-rciti-branch/node_modules/insight/lib/index.js:37:13)
        at Object.<anonymous> (/Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova/src/telemetry.js:26:15)
        at Module._compile (node:internal/modules/cjs/loader:1246:14)
        at Module._extensions..js (node:internal/modules/cjs/loader:1300:10)
        at Module.load (node:internal/modules/cjs/loader:1103:32)
        at Module._load (node:internal/modules/cjs/loader:942:12)
        at Module.require (node:internal/modules/cjs/loader:1127:19)
        at require (node:internal/modules/helpers:112:18)
    ```
    - we do this by copying over a file that includes Ventura
        per https://stackoverflow.com/questions/68318289/ionic-fails-building-in-macos-12-monterey
    - I tried to do this in a more principled fashion by upgrading to the most recent version of cordova
        - but we are already at the most recent version of cordova, and it still doesn't work
    - I then tried to compare it to the same file in the cordovabuild version and it was the same

        ```
           $ grep Ventura ~/e-mission/native_code_upgrade/node_modules/macos-release/index.js | wc -l
           0
        ```
    - on further invesigation, it turns out that the issue is not that the more
      recent version of cordova has a newer version of the macos-release
      library, but that it doesn't use `Insight` (at least by default), which
      is the module that calls it
    - phonegap, which we need for `phonegap serve`, does
        ```
        $ grep -r "new Insight" node_modules/
        node_modules//cordova/node_modules/insight/readme.md:const insight = new Insight({
        node_modules//cordova/node_modules/insight/readme.md:const insight = new Insight({
        node_modules//cordova/node_modules/insight/lib/push.js: const insight = new Insight(message);
        node_modules//insight/readme.md:const insight = new Insight({
        node_modules//insight/readme.md:const insight = new Insight({
        node_modules//insight/lib/push.js:  const insight = new Insight(msg);
        node_modules//phonegap/node_modules/cordova/src/telemetry.js:var insight = new Insight({
        node_modules//phonegap/lib/cli/analytics.js:var insight = new Insight({
        ```
    - so there is really no alternative to hot-patching the file

- add the cordova `browser` platform to fix:

    ```
    CordovaError: No platforms added to this project. Please use `phonegap platform add <platform>`.
        at Object.preProcessOptions (/Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova-lib/src/cordova/util.js:313:15)
        at /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova-lib/src/cordova/prepare.js:49:40
        at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    ```
    - incidentally, if we don't add the platform, cordova tries to add it itself, and that completely messes up the `node_modules`

    There are 720 packages

    ```
    $ ls -al node_modules/ | wc -l
         720
    ```

    we try to add the browser, which fails

    ```
    Using cordova-fetch for cordova-browser@^6.0.0

    (node:24473) Warning: Accessing non-existent property 'browser' of module exports inside circular dependency
    (Use `node --trace-warnings ...` to show where the warning was created)

    Failed to fetch platform cordova-browser@^6.0.0
    Probably this is either a connection problem, or platform spec is incorrect.
    Check your connection and platform name/version/URL.
    Could not determine package name from output:
    added 24 packages, changed 1 package, and audited 131 packages in 10s
    ```

    we end up with 111 packages, and `phonegap` is uninstalled as well

    ```
    $ ls -al node_modules | wc -l
         111
    ```

    - and the `phonegap` command is not found either

- add shelljs to the dependencies to fix

    ```
     [warning] Unable to load PlatformApi from platform. Error: Cannot find module 'shelljs'
     [warning] Require stack:
     [warning] - /Users/kshankar/e-mission/phone-rciti-branch/platforms/browser/cordova/Api.js
     [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova-lib/src/cordova/util.js
     [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova-lib/src/platforms/platforms.js
     [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova-lib/src/plugman/install.js
     [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova-lib/src/plugman/plugman.js
     [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova-lib/cordova-lib.js
     [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/node_modules/cordova/cordova.js
     [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/lib/cordova/index.js
     [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/lib/phonegap/cordova.js
     [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/lib/phonegap.js
     [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/lib/main.js
     [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/lib/cli.js
     [warning] - /Users/kshankar/e-mission/phone-rciti-branch/node_modules/phonegap/bin/phonegap.js
    ```

Testing done:

```
$ rm -rf node_modules
$ bash setup/setup_serve.sh
$ npm run serve
```

the serve process starts up and the devapp is able to connect to it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

⬆️ [phone] Upgrade to the latest everything + some RFC 🎉
1 participant