Lint jar for IID Version incompatabiltiy #1347
Conversation
Codecov Report
Continue to review full report at Codecov.
|
Binary Size ReportAffected SDKs
Test Logs |
| // firebase-iid v20.1.0 already depended on FirebaseInstallations for identifier creation, | ||
| // but had an issue of possible silent failures. Thus, v20.1.1 or later is recommended. | ||
| val validIidVersionWithFis = "20.1.1" |
There was a problem hiding this comment.
I don't think developers need this kind of context.
Just tell them which version to use.
If this comment is meant for us, then I would recommend to rephrase:
"firebase-iid v20.1.0 is compatible with FirebaseInstallations, but contains a bug that was fixed in v20.1.1"
| | ||
| explanation = """ | ||
| These versions of FirebaseInstanceId and FirebaseInstallations can not be used in parallel. | ||
| Please update your Firebase Instance ID to a version 20.1.0 or later. |
There was a problem hiding this comment.
There seems to be an inconsistent Inconsistent use of versions!
Sometimes it's 20.1.0, sometimes 20.1.1 (see below).
I assume that you want to prevent usage of <20.1.0 and recommend usage of v20.1.1...
I think, you would make your life easier by just marking everything before v20.1.1 as incompatible and keep it consistent.
There was a problem hiding this comment.
Agreed. Thanks for catching this.
There was a problem hiding this comment.
This explanation is not the part that developers are most likely to see, instead this is the error they will see in their build:
/Users/vkryachko/Code/firebase/firebase-android-sdk/firebase-inappmessaging-display: Error: Incompatible IID version found in variant debug: com.google.firebase:firebase-iid:20.0.1@aar [IncompatibleIidVersion] With that in mind I think it could be improved in a few ways:
- Most likely developers don't have a direct dependency on iid and don't even know what iid is, it might make sense mention that the dependency may be transitive and how it most likely ended up in their build
- "@aar" part may confuse developers, consider dropping it
- Consider adding a call for action here like how exactly the problem can be solved with a code example to copy paste into their build file.
| private fun compareVersions(versionA: String, versionB: String): Int { | ||
| val versionAComponents = versionA.split('.').toTypedArray() | ||
| val versionBComponents = versionB.split('.').toTypedArray() | ||
| val k = Math.min(versionAComponents.size, versionBComponents.size) | ||
| for (i in 0..k - 1) { | ||
| if (versionAComponents.get(i) > versionBComponents.get(i)) { | ||
| return 1 | ||
| } | ||
| if (versionAComponents.get(i) < versionBComponents.get(i)) { | ||
| return -1 | ||
| } | ||
| } | ||
| | ||
| if (versionAComponents.size == versionBComponents.size) { | ||
| return 0 | ||
| } | ||
| if (versionAComponents.size < versionBComponents.size) { | ||
| return -1 | ||
| } else { | ||
| return 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
This method seems unnecessarily complicated.
Can't we just check for the version that we are interested in?
kind of like this (pseudo code):
versionComponents = version.split('.')
// check if correct SemVer format
return FALSE if 3 != versionComponents.size
// major version before v20? => incompatible
return FALSE if 20 > versionComponants[0].asInt
// major version after v21? => compatible
return TRUE if 21 <= versionComponants[0].asInt
// major version is v20 and minor version major version after v20.1? => compatible
return (1 <= versionComponants[1].asInt)
| @ankitaj224 ptal at #1346 |
| @ankitaj224 a few points:
|
| import com.android.tools.lint.detector.api.Location | ||
| import com.android.tools.lint.detector.api.Scope | ||
| import com.android.tools.lint.detector.api.Severity | ||
| import com.android.tools.lint.detector.api.* |
There was a problem hiding this comment.
This is against Java readability rules in Google I think. Not sure how this is handled here?
| return true | ||
| } | ||
| // Incompatible if major version is before v20 | ||
| if (20 > versionComponents.get(0).toInt()) { |
There was a problem hiding this comment.
Can #toInt throw an exception?
Do we care?
| private fun isIncompatibleVersion(coordinates: MavenCoordinates): Boolean { | ||
| // TODO: implement version check | ||
| return coordinates.version == "0.0.0" | ||
| val versionComponents = coordinates.version.split('.').toTypedArray() |
There was a problem hiding this comment.
Can you tell split to split this into maximum 3 components?
If yes, please do so.
...hecks/src/main/java/com/google/firebase/installations/lint/IncompatibleIidVersionDetector.kt Outdated Show resolved Hide resolved
5 participants
Patched and improved #1346
@vkryachko please merge your PR and I ll update the my branch accordingly.