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

chore(cordova/android): outline android lib #1571

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HankLi0130
Copy link

Modification for developing outline Android SDK by nthlink.

  • Upgrade gradle version
  • Support kotlin 1.8
  • Upgrade compileSdk and targetSdk to 33
  • Broadcast vpn status disconnected while tear down active tunnel.

@HankLi0130 HankLi0130 requested a review from a team as a code owner February 9, 2023 10:15
@fortuna fortuna changed the title Outline android lib chore(cordova/android): outline android lib Feb 9, 2023
Copy link
Collaborator

@fortuna fortuna left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. It's exciting to be able to use Kotlin!
I do have a few concerns, since this breaks the build and I'm not sure how the new API level affects our app.

/cc @daniellacosse @jyyi1

@@ -1,5 +1,6 @@
// Top-level build file where you can add configuration options common to all sub-projects/modules.
plugins {
// This version number must be kept in sync with AGP_VERSION in cordova-android.
id 'com.android.library' version '7.2.1' apply false
id 'com.android.library' version '7.4.1' apply false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

I compiled the code with android studio successfully.

Screenshot 2023-02-10 at 10 54 51 AM

Is there any documentation that introduce how to compile outline-client with OutlineAndroidLib? I'll try it again.

@@ -1,6 +1,6 @@
#Fri Jan 06 14:17:15 EST 2023
distributionBase=GRADLE_USER_HOME
distributionUrl=https\://services.gradle.org/distributions/gradle-7.4-bin.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-7.5-bin.zip
Copy link
Collaborator

Choose a reason for hiding this comment

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

Or perhaps this is breaking the build?

Copy link
Author

Choose a reason for hiding this comment

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

I updated the gradle version to 7.5. The plugin org.jetbrains.kotlin.android should be set to 7.4.1.

Here is the reference: https://developer.android.com/studio/releases/gradle-plugin#updating-gradle


defaultConfig {
minSdk 22
targetSdk 32
targetSdk 33
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we can accept this change without ensuring our app works well with API 33.
Do you need this change? I believe you can set a different target SDK on your app.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I can set the SDK to 32.

I made this change is because since Android Studio Electric Eel ( 2022.1.1 Patch 1), the initial compileSdk and targetSdk are 33 while starting a new project. I want developers who want to use outline SDK no need to downgrade the SDK version after they create a new project while developing their VPN.

So we keep the SDK version to 32?

@@ -2,9 +2,13 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android">

<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
<uses-permission android:name="android.permission.POST_NOTIFICATIONS" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Author

Choose a reason for hiding this comment

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

Foreground Service, which is VpnTunnelService in OutlineAndroidLib, needs this permission with targetSDK 33.

Here is the reference: https://developer.android.com/develop/ui/views/notifications/notification-permission

@@ -30,627 +30,634 @@
import android.net.VpnService;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please give an overview of what's changing in this file?

Also, please restore the 2-space indentation, so we can see the changes and for consistency.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, my bad.

Here is what I've changed,

private void tearDownActiveTunnel() {
    stopVpnTunnel();
    stopForeground();
    // add this line, line 361
    broadcastVpnConnectivityChange(TunnelStatus.DISCONNECTED);
    tunnelConfig = null;
    stopNetworkConnectivityMonitor();
    tunnelStore.setTunnelStatus(TunnelStatus.DISCONNECTED);
  }

@HankLi0130
Copy link
Author

HankLi0130 commented Feb 10, 2023

I found that we're using cordova-android 11 in package.json. The cordova-android 11 only supports targetSDK 32 and Gradle: 7.4.2. I removed the code that upgrade gradle version, targetSDK and compileSDK. It should be compiled now.

references: https://cordova.apache.org/announcements/2022/07/12/cordova-android-release-11.0.0.html

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.

None yet

2 participants