feat: android gradle wrapper update #2131
Conversation
| import execa from 'execa'; | ||
|
|
||
| // TODO: Fetch it from somewhere? | ||
| const gradleWrapperVersionsMap: Record<number, string> = { |
There was a problem hiding this comment.
Don't really like this hardcoded array. Maybe we could fetch data about gradle versions from core somewhere?
(But that would be time consuming and then we definitely shouldn't be running checkGradleWrapper on each run of command... ) 🤔
There was a problem hiding this comment.
In ideal world flow should look like this IMO:
- We cache
package.json, - While running
run/build-androidwe check is it changes, if yes we do a request to GitHub and look for branch related toreact-nativeversion that is presented inpackage.json, then we do the logic and we run./gradlew wrapper ...
The drawback here is that we need to be sure the source of informations, if something change in future in response we potentially will need to release X patch releases.
There was a problem hiding this comment.
Can we just get it from the react-native package directly?
% cat node_modules/react-native/template/android/gradle/wrapper/gradle-wrapper.properties
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-8.0.1-all.zip
networkTimeout=10000
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
There was a problem hiding this comment.
Also, I'm not sure this if this isn't overly aggressive. For instance, Gradle 7.5 works fine with 0.72, but with this change, you're forcing people to upgrade even though they might not be ready for that yet. We have a similar check in RNTA, but using hard-coded values because the template isn't always right: https://github.com/microsoft/react-native-test-app/blob/1289c283c034472be5adbdc33a2238fb08b27875/android/test-app-util.gradle#L9-L45
There was a problem hiding this comment.
Also, I'm not sure this if this isn't overly aggressive. [...]
Maybe we can extend prompt question with "No, don't ask again" option and store that value in cache so we won't be asking repeatedly?
| } | ||
| // TODO: Do we care about minor and patch or only major version? | ||
| if ( | ||
| gradleVersionForCurrent.slice(0, 2) !== gradlewWrapperVersion.slice(0, 2) |
There was a problem hiding this comment.
Do we care about patch versions of gradle or major.minor is enough here?
cc @cortinico ?
There was a problem hiding this comment.
We care about every version as they might change the wrapper script at any time
adamTrz
force-pushed
the
feat/android-gradle-wrapper
branch
3 times, most recently
from
db5025a
to
f5b6103
Compare
| }; | ||
|
|
||
| export const checkGradleWrapper = async (projectRoot: string) => { | ||
| if (process.env.CI) { |
There was a problem hiding this comment.
I don't think this check is enough. You also need to check whether you're in an interactive shell. Otherwise this will just stall.
| import execa from 'execa'; | ||
|
|
||
| // TODO: Fetch it from somewhere? | ||
| const gradleWrapperVersionsMap: Record<number, string> = { |
There was a problem hiding this comment.
Also, I'm not sure this if this isn't overly aggressive. For instance, Gradle 7.5 works fine with 0.72, but with this change, you're forcing people to upgrade even though they might not be ready for that yet. We have a similar check in RNTA, but using hard-coded values because the template isn't always right: https://github.com/microsoft/react-native-test-app/blob/1289c283c034472be5adbdc33a2238fb08b27875/android/test-app-util.gradle#L9-L45
| return await prompt({ | ||
| name: 'update', | ||
| type: 'select', | ||
| message: `Upgrade Gradle Wrapper to ${gradleVersionForCurrent}?`, | ||
| choices: [ | ||
| {title: 'Yes', value: true}, | ||
| {title: 'No', value: false}, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
I wonder if run-android is doing too much here. Shouldn't we instead redirect people to run the doctor command instead?
adamTrz
force-pushed
the
feat/android-gradle-wrapper
branch
from
f5b6103
to
139d6bf
Compare
|
There hasn't been any activity on this pull request in the past 3 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. |
Summary:
Check version of Android Gradle and update it when necessary.
Fixes #1837
Test Plan:
build-androidorrun-androidagainst test projectChecklist