-
Notifications
You must be signed in to change notification settings - Fork 901
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
feat: android gradle wrapper update #2131
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ideal world flow should look like this IMO:
- We cache
package.json
, - While running
run/build-android
we check is it changes, if yes we do a request to GitHub and look for branch related toreact-native
version 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we care about patch versions of gradle or major.minor is enough here?
cc @cortinico ?
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.
We care about every version as they might change the wrapper script at any time
db5025a
to
f5b6103
Compare
}; | ||
|
||
export const checkGradleWrapper = async (projectRoot: string) => { | ||
if (process.env.CI) { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if run-android
is doing too much here. Shouldn't we instead redirect people to run the doctor
command instead?
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.
Hmm, good call!
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-android
orrun-android
against test projectChecklist