-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Android: Clean up lint warnings and other warnings to get build success #12143
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
base: master
Are you sure you want to change the base?
Conversation
@ejona86 @shivaspeaks Can you please review. |
@Sangamesh1997 Can you provide the build error log that you are fixing with this PR? In the failing build.log of PR #12040 I don't see any errors about @TargetApi for example. |
@kannanjgithub In the build log of PR #12040 , I observed that the only visible lint error was OldTargetApi. However, when I ran the lint check locally in (lint report.html), it also flagged other warnings like InlineApi, ObseleteSdkInt and NewApi, as mentioned by Eric earlier. So in this PR, I've fixed all these together to ensure build passes cleanly and is future-proof. |
@Sangamesh1997 can you share that lint report.html? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we working on silencing? I thought the discussion happened to upgrade and fix in the #12040 (comment)
@shivaspeaks I have addressed and cleared all the lint warnings in #12143, which resolves the issue tracked in #6868. |
Does it cause any problem if you set targetSdkVersion to the latest available even while the AGP is not upgraded yet? @Sangamesh1997 |
@@ -165,6 +166,7 @@ public Intent asBindIntent() { | |||
* | |||
* <p>See {@link Intent#URI_ANDROID_APP_SCHEME} for details. | |||
*/ | |||
@SuppressLint("InlinedApi") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning inlinedApi is about using the constant URI_ANDROID_APP_SCHEME that was introduced in API level 22. Instead do intentForUri.toUri(android.os.Build.VERSION.SDK_INT < 22? URL_INTENT_SCHEME : URI_ANDROID_APP_SCHEME);
and remove the suppression. If the linter still complains, then add @TargetApi(22) to indicate that we are only using URI_ANDROID_APP_SCHEME at and above API level 22.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kannanjgithub yes fixed with conditional version check only - lint warnings resolved without @TargetApi working.
@@ -183,18 +184,22 @@ private static Status bindInternal( | |||
bindResult = context.bindService(bindIntent, conn, flags); | |||
break; | |||
case BIND_SERVICE_AS_USER: | |||
bindResult = context.bindServiceAsUser(bindIntent, conn, flags, targetUserHandle); | |||
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is going to fail the connection with Unimplemented for Android API levels 21 - 29. It would already be crashing for these API levels? @ejona86 . Also for similar changes below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdcormie probably knows what's going on better. But I'd expect we need proper error handling here, and not claiming things are UNIMPLEMENTED. INTERNAL with a message that the API is not available might be fair.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding bindServiceAsUser()
, I agree that we should not fail with UNIMPLEMENTED
in this case because that already means something else that applications do want to handle very specifically.
If we reach this line it's because targetUserHandle != null
which means the application is asking for a cross-user Channel. The public entry point to this feature already documents an SDK >= 30 requirement which is why the old behavior of crashing with an NoSuchMethodError was actually totally fine IMHO. IF we just do checkState(SDK_INT >= 30, "Cross user Channel requires Android R+")
would that make the linter happy and yield the desired INTERNAL
error?
@@ -0,0 +1,4 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> | |||
<lint> | |||
<issue id="OldTargetApi" severity="ignore" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it cause any problem if you set targetSdkVersion to the latest available even while the AGP is not upgraded yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it causes AAPT2 linking failures due to resource format changes. AGP 7.x is incompatible with SDK 35/36, so AGP must be upgraded first. keeping the lint.xml
suppression after upgrading AGP/SDK won’t break anything, and have to remove it after the AGP/SDK upgrade to keep our configuration clean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include that relevant information here as a comment, so it is clear when it can be removed.
37f84d1
to
94532a6
Compare
…xed_12142 # Conflicts: # binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java
@kannanjgithub I tried and got a build failure with 'AAPT2 error: Android resourse linking failed'. SDK 35/36 introduced internal changes in resource formats. The current AGP versions cant handle these changes during resource linking. that's why I suspect that using compileSdk>34 is unsafe unless AGP is upgraded to 8.x.+. |
@@ -0,0 +1,4 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> | |||
<lint> | |||
<issue id="OldTargetApi" severity="ignore" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include that relevant information here as a comment, so it is clear when it can be removed.
@@ -183,18 +184,22 @@ private static Status bindInternal( | |||
bindResult = context.bindService(bindIntent, conn, flags); | |||
break; | |||
case BIND_SERVICE_AS_USER: | |||
bindResult = context.bindServiceAsUser(bindIntent, conn, flags, targetUserHandle); | |||
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdcormie probably knows what's going on better. But I'd expect we need proper error handling here, and not claiming things are UNIMPLEMENTED. INTERNAL with a message that the API is not available might be fair.
@@ -172,7 +175,10 @@ public String asAndroidAppUri() { | |||
// factory methods. Oddly, a ComponentName is not enough. | |||
intentForUri = intentForUri.cloneFilter().setPackage(getComponent().getPackageName()); | |||
} | |||
return intentForUri.toUri(URI_ANDROID_APP_SCHEME); | |||
return intentForUri.toUri( | |||
Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP_MR1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you bump the minimum SDK version to 22 instead of this change? We should already be on 23, and in a few days, maybe could even do 24. Bumping to 23 might introduce more warnings, but 21->22 seems likely to be trivial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Falling back to an intent:
URI isn't OK. The method name and javadoc explicitly say we'll return an android-app://
URI.
min SDK bump SGTM.
Fixes : #6868 #12142
Worked on clearing the lint warnings (OldTargetApi, ObsoleteSdkInt, InlinedApi, NewApi)