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

[TIMOB-24967] Android: Update to support library 26.0.1 #9274

Merged
merged 6 commits into from Aug 18, 2017

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Aug 3, 2017

  • Update to support library 26.0.0 26.0.1
  • Target android-26 by default
  • Build using android-26
  • Include IntelliJ and Android Studio project files

JIRA Ticket

@garymathews
Copy link
Contributor Author

Wait for Jenkins to update to android-26

@jquick-axway
Copy link
Contributor

@garymathews, let's not target API Level 26 (aka: Android O) yet.

Before we can target API Level 26, we need to resolve any breaking changes it'll cause first. I'm especially concerned about the changes mentioned under Networking, Security, and Privacy.
https://developer.android.com/preview/behavior-changes.html

However, we should allow the developer to target API Level 26 if they want to via the "tiapp.xml" (they can do so at their own risk). I'm just saying we shouldn't default to it yet.

@garymathews
Copy link
Contributor Author

@jquick-axway But we need to target it to access Android O specific features that we are aiming to support, and targeting it will also let us verify what is broke. We can see what tests fail on Jenkins and also give QE a build for testing too.

@jquick-axway
Copy link
Contributor

@garymathews, I understand that. I am okay with us "compiling" with the API Level 26 SDK. My point is that we should not "target" API Level 26 by default yet, because targeting it is what triggers the breaking changes. And we don't officially support API Level 26 until the breaking changes have been resolved.

However, I am okay with allowing developers (us and customers) to opt-in to targeting API Level 26 at their own risk by doing the below. This is what we should do for the moment for internal testing.

<ti:app xmlns:ti="http://ti.appcelerator.org">
	<android xmlns:android="http://schemas.android.com/apk/res/android">
		<manifest>
			<uses-sdk android:minSdkVersion="16" android:targetSdkVersion="26"/>
		</manifest>
	</android>
</ti:app>

@garymathews
Copy link
Contributor Author

garymathews commented Aug 11, 2017

@jquick-axway Gotcha, I've amended the android/package.json to reflect that. Target Android SDK 25, max supported Android SDK 26.

"android sdk": ">=23 <=25.x",
"android build tools": ">=23 <=25.x",
"android platform tools": ">=17 <=25.x",
"android sdk": ">=25 <=26.x",
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting the "android sdk" max to 26.x will still make Titanium target API Level 26 by default if it's installed. This is what needs to be 25.x for the time being.

I remember talking to Chris Barber about this for the following change to ensure Titanium 6.x wouldn't target API Level 25 by default, but still allow users to target it at their own risk.
jquick-axway@a80cb33

Question: Is there a reason we need to bump up the min target API Level we support from 23 to 25? I'm not opposed to it. Just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I'll update the PR.

I guess we could leave it at 23, I wanted to keep it inline with our support library.

"android platform tools": ">=17 <=25.x",
"android sdk": ">=25 <=26.x",
"android build tools": ">=25 <=26.x",
"android platform tools": ">=25 <=26.x",
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're including v26 of the Google supports libraries, I believe the min API level needs to be 26 for "android build tools" and "android platform tools"... or else a compiler error will occur... right? A compiler error might happen because the v26 libraries may reference new API Level 26 resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, I've used 25.0.3 build tools before without issues. Not sure about the platform tools version though. I'll check that

"node": ">=4.0 <=7.x",
"java": "<=1.8.x"
"java": ">=1.8.x"
Copy link
Contributor

Choose a reason for hiding this comment

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

Question:
Our "node scons.js build" command currently compiles our core Java code using Java version 1.6, regardless of what's installed. It would be nice if we can bump it up to Java 1.7 so that we can support new Java features such as the try-with-resource blocks and Closeable interfaces...
https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

Do you happen to know where in our CLI or build scripts this is controlled. We can make this a separate PR I suppose, but since we're now requiring a higher JDK version here, this might be a good opportunity to use newer Java features. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should be here if you want to test 1.7

"android tools": "<=26.x",
"android ndk": ">=r8e <=r9",
"android ndk": ">=r11c <=r15c",
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding Android NDK version, we have to watch out for C++ ABI compatibility issues with our precompiled V8 library. This is to avoid compiler errors and might impact hyper loop. We can figure this out later. (This is more of a warning.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the min should be r11c, our master branch should be compatible with r15c with the new V8 update PR. So lets keep this in mind when we backport to 6_2_X

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

@garymathews garymathews force-pushed the TIMOB-24967 branch 2 times, most recently from 013b509 to 26389b6 Compare August 16, 2017 15:42
@garymathews garymathews changed the title [TIMOB-24967] Android: Update to support library 26.0.0 [TIMOB-24967] Android: Update to support library 26.0.1 Aug 16, 2017
@lokeshchdhry
Copy link
Contributor

lokeshchdhry commented Aug 16, 2017

@garymathews , when I try to build an app with a sdk built for this PR I get errors:

[INFO] :   Running AAPT: /Users/lchoudhary/Desktop/android-sdk-macosx/build-tools/26.0.1/aapt "package" "-f" "-m" "-J" "/Users/lchoudhary/Desktop/workspaces/workspace_2016/ZXYZ/build/android/gen" "-M" "/Users/lchoudhary/Desktop/workspaces/workspace_2016/ZXYZ/build/android/AndroidManifest.xml" "-A" "/Users/lchoudhary/Desktop/workspaces/workspace_2016/ZXYZ/build/android/bin/assets" "-S" "/Users/lchoudhary/Desktop/workspaces/workspace_2016/ZXYZ/build/android/res" "-I" "/Users/lchoudhary/Desktop/android-sdk-macosx/platforms/android-25/android.jar" "-F" "/Users/lchoudhary/Desktop/workspaces/workspace_2016/ZXYZ/build/android/bin/app.ap_" "--output-text-symbols" "/Users/lchoudhary/Desktop/workspaces/workspace_2016/ZXYZ/build/android/intermediates/bundles" "--no-version-vectors" "--auto-add-overlay" "--extra-packages" "ti.modules.titanium.ui:android.support.v7.appcompat:android.support.v7.cardview:android.support.design:android.support.compat"
[ERROR] :  Failed to package application:
[ERROR] :  
[ERROR] :  /Users/lchoudhary/Desktop/workspaces/workspace_2016/ZXYZ/build/android/res/values-v26/values-v26.xml:15: error: Error: No resource found that matches the given name: attr 'android:keyboardNavigationCluster'.
[ERROR] :  
[ERROR] :  /Users/lchoudhary/Desktop/workspaces/workspace_2016/ZXYZ/build/android/res/values-v26/values-v26.xml:19: error: Error: No resource found that matches the given name: attr 'android:keyboardNavigationCluster'.

If I target android 26 then it build fine.

@garymathews
Copy link
Contributor Author

@lokeshchdhry Updated PR, try now? Also make sure the default generated Android manifest has android:targetSdkVersion="25"

@lokeshchdhry
Copy link
Contributor

@garymathews, In the androidManifest I see <uses-sdk android:minSdkVersion="16" android:targetSdkVersion="26"/>

@garymathews
Copy link
Contributor Author

garymathews commented Aug 18, 2017

@lokeshchdhry Remove

<uses-sdk android:minSdkVersion="16" android:targetSdkVersion="26"/>

from your tiapp.xml, it should default to

<uses-sdk android:minSdkVersion="16" android:targetSdkVersion="25"/>

in the generated AndroidManifest.xml

@lokeshchdhry
Copy link
Contributor

@garymathews , My bad the app had targetSDK as 26.

@lokeshchdhry
Copy link
Contributor

FR Passed.

App builds fine with support library 26.0.1. No crash etc is seen.
The real verification will be done in this PR #9278 but this PR needs to be merged first.

Studio Ver: 4.9.1.201707200100
SDK Ver: 7.0.0 local build
OS Ver: 10.12.3
Xcode Ver: Xcode 8.3.3
Appc NPM: 4.2.9
Appc CLI: 6.2.3
Ti CLI Ver: 5.0.14
Alloy Ver: 1.9.13
Node Ver: 6.10.1
Java Ver: 1.8.0_101
Devices: ⇨ google Nexus 5 --- Android 6.0.1
⇨ google Pixel --- Android 7.1.1
Emulator: Android 4.4.4

@lokeshchdhry lokeshchdhry merged commit af4da32 into tidev:master Aug 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants