Support SDK versions with pre-release identifiers #709
Conversation
Codecov Report
@@ Coverage Diff @@
## master #709 +/- ##
=======================================
Coverage 64.38% 64.38%
=======================================
Files 18 18
Lines 1828 1828
=======================================
Hits 1177 1177
Misses 651 651Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #709 +/- ##
=========================================
+ Coverage 63.98% 64.38% +0.4%
=========================================
Files 18 18
Lines 1824 1828 +4
=========================================
+ Hits 1167 1177 +10
+ Misses 657 651 -6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Firstly, thanks for taking the time to submit the issue and this PR.
Secondly, I pointed out a few potentially problematic places in your code. IMO, we really should ensure that 20.0.0-rc1 < 20.0.0 and for that, you probably have to add the pre-release identifiers to the token lists.
Lastly, it would be great if we could use an external library to do the version comparison. Version Compare looks promising. We should be able to include that using the following code:
buildscript {
dependencies {
classpath 'com.g00fy2:versioncompare:1.3.1@jar'
}
}
framework/cordova.gradle
Outdated
| // "19.0.0" > "19" | ||
| // "19.0.1" > "19.0.0" | ||
| // "19.1.0" > "19.0.1" | ||
| // "19" > "18.999.999" | ||
| int compareVersions(String a, String b) { | ||
| a = removeTagOfReleaseLifeCycle(a) | ||
| b = removeTagOfReleaseLifeCycle(b) |
There was a problem hiding this comment.
AFAICT, with this code 20.0.0-rc1 == 20.0.0 would hold. Whereas I would expect 20.0.0-rc1 < 20.0.0.
There was a problem hiding this comment.
Thanks for the review.
I thought that Android SDK Manager will change the version string by removing the pre-release identifier after the release so we don't need to compare with pre-release identifiers. But some users may use all versions of build-tools for debugging.
I don't know that the 3rd party libraries can be used for the solution. Is it acceptable for the project? It may cause vulnerability and reduce consistency.
There was a problem hiding this comment.
We use tons of 3rd party libraries on the Node.js side of our tools, so I don't see why we should not use Java/Groovy libraries. Especially when it's only a compile time dependency. I'm always happy when we can reduce the amount of code we have to maintain.
There was a problem hiding this comment.
Ok, thanks a lot. I am going to use the library.
There was a problem hiding this comment.
Nice! Thanks for giving this another go.
raphinesse
changed the title
|
@wmramazan Thanks for the update. But why did you include all the changes outside of |
|
I just wanted to update the project for the new version of Android Studio. |
|
Please undo any changes that are not directly related to the problem we try to solve here. Please mind the commit I added when doing that. |
Closes apache#708. Do not rely on our own version regex anymore Revert "Do not rely on our own version regex anymore" This reverts commit 6b3345e. Revert "Closes apache#708." This reverts commit 8d9a1f5. Add version-compare library to compare build-tools versions properly.
|
Thanks for the update and for fixing my commit @wmramazan. The modified file looks good to me. Do you have any remarks on the bigger picture @erisu? |
Platforms affected
Android
Motivation and Context
Fixes #708
Description
I changed the regular expression for versions of build tools in cordova.gradle file in order to accept all possible versions. And I removed the tag of release life cycle in version string when comparing them.
Testing
I built an example project with both Android SDK Build-Tools 28.0.0 and 29.0.0-rc1.
Checklist
(platform)if this change only applies to one platform (e.g.(android))