Skip to content
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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import adb from '../runAndroid/adb';
import getAdbPath from '../runAndroid/getAdbPath';
import {getTaskNames} from '../runAndroid/getTaskNames';
import {promptForTaskSelection} from '../runAndroid/listAndroidTasks';
import {checkGradleWrapper} from '../runAndroid/checkGradleWrapper';

export interface BuildFlags {
mode?: string;
Expand All @@ -26,6 +27,8 @@ async function buildAndroid(
) {
const androidProject = getAndroidProject(config);

await checkGradleWrapper(config.root);

if (args.tasks && args.mode) {
logger.warn(
'Both "tasks" and "mode" parameters were passed to "build" command. Using "tasks" for building the app.',
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import path from 'path';
import fs from 'fs';
import {logger, prompt, version} from '@react-native-community/cli-tools';
import chalk from 'chalk';
import execa from 'execa';

// TODO: Fetch it from somewhere?
const gradleWrapperVersionsMap: Record<number, string> = {
Copy link
Collaborator Author

@adamTrz adamTrz Oct 24, 2023

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... ) 🤔

Copy link
Collaborator

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:

  1. We cache package.json,
  2. While running run/build-android we check is it changes, if yes we do a request to GitHub and look for branch related to react-native version that is presented in package.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.

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Collaborator Author

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?

68: '7.3',
69: '7.3',
70: '7.5',
71: '7.5',
72: '8.0',
};

const findGradlewWrapperVersion = (root: string) => {
// TODO: In upgrade helper we should have ifno warning users not to upgrade this file manually or this would fail...
const gradlePropertiesFilePath = path.join(
root,
'android/gradle/wrapper/gradle-wrapper.properties',
);
let propertiesContent = '';
if (fs.existsSync(gradlePropertiesFilePath)) {
propertiesContent = fs.readFileSync(gradlePropertiesFilePath, 'utf-8');
}
const match = propertiesContent.match(/\d+(\.\d+)+/);
if (match) {
return match[0];
}
return undefined;
};

const askForGradleUpdate = async (
gradleVersionForCurrent: string,
gradlewWrapperVersion: string,
) => {
logger.info(`We have detected that you have outdated Gradle Wrapper in your Android project.
Current version is ${chalk.bold(
gradlewWrapperVersion,
)} while recommended version is ${chalk.bold(
gradleVersionForCurrent,
)}. Would you like to run automatic update?`);
return await prompt({
name: 'update',
type: 'select',
message: `Upgrade Gradle Wrapper to ${gradleVersionForCurrent}?`,
choices: [
{title: 'Yes', value: true},
{title: 'No', value: false},
],
});
Comment on lines +43 to +51
Copy link
Contributor

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good call!

};

const runGradleWrapperUpdateTwice = (gradleVersionForCurrent: string) => {
logger.info('Upgrading gradle wrapper files');
try {
const cmd = process.platform.startsWith('win')
? 'gradlew.bat'
: './gradlew';
const gradleArgs = [
'wrapper',
`--gradle-version=${gradleVersionForCurrent}`,
'--distribution-type=all',
];
logger.debug(
`Running command "cd android && ${cmd} ${gradleArgs.join(' ')}"`,
);
execa.sync(cmd, gradleArgs, {stdio: 'inherit', cwd: 'android'});
execa.sync(cmd, gradleArgs, {stdio: 'inherit', cwd: 'android'});
} catch (error) {
return;
}
};

export const checkGradleWrapper = async (projectRoot: string) => {
if (process.env.CI) {
Copy link
Contributor

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.

return;
}
const semver = version.current(projectRoot);
const minor = semver?.minor;
if (!minor) {
return;
}
const gradleVersionForCurrent = gradleWrapperVersionsMap[minor];
const gradlewWrapperVersion = findGradlewWrapperVersion(projectRoot);
if (!gradlewWrapperVersion) {
return;
}
// TODO: Do we care about minor and patch or only major version?
if (
gradleVersionForCurrent.slice(0, 2) !== gradlewWrapperVersion.slice(0, 2)
Copy link
Collaborator Author

@adamTrz adamTrz Oct 24, 2023

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 ?

Copy link
Member

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

) {
const {update} = await askForGradleUpdate(
gradleVersionForCurrent,
gradlewWrapperVersion,
);
if (update) {
runGradleWrapperUpdateTwice(gradleVersionForCurrent);
}
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {build, BuildFlags, options} from '../buildAndroid';
import {promptForTaskSelection} from './listAndroidTasks';
import {getTaskNames} from './getTaskNames';
import {checkUsers, promptForUser} from './listAndroidUsers';
import {checkGradleWrapper} from './checkGradleWrapper';

export interface Flags extends BuildFlags {
appId: string;
Expand Down Expand Up @@ -74,6 +75,8 @@ async function runAndroid(_argv: Array<string>, config: Config, args: Flags) {
link.setVersion(config.reactNativeVersion);
}

await checkGradleWrapper(config.root);

if (args.binaryPath) {
if (args.tasks) {
throw new CLIError(
Expand Down
Loading