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

Applied missing changes from bumping Gradle wrapper to 6.0.1 #27639

Closed
wants to merge 1 commit into from
Closed

Applied missing changes from bumping Gradle wrapper to 6.0.1 #27639

wants to merge 1 commit into from

Conversation

SaeedZhiany
Copy link
Contributor

@SaeedZhiany SaeedZhiany commented Dec 30, 2019

Summary

This PR is related to #27290.
I just upgraded my project's Gradle wrapper version to 6.0.1 and I realized some files have some differences with the files in react-native template folder. so I create this PR to apply differences.
the main difference is in the gradlew file. I'm not familiar with Linux shell scripts but it seems there was a syntax error in case items syntax. ( should not be used in declaring case's items. it may has building error in Linux OS.

Changelog

[Android] [Fixed] - Applied missing changes from bumping Gradle wrapper to 6.0.1

Test Plan

I have no Linux OS right now, so I can't directly test these changes, but because the changes have made by running gradlew wrapper command, it should not break CI. (I hope :) )

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 30, 2019
@react-native-bot react-native-bot added Bug Platform: Android Android applications. labels Dec 30, 2019
@SaeedZhiany
Copy link
Contributor Author

In ci/circleci: test_android's log, it seems it can build Android RNTester App successfully and it failed in Collect Test Results phase because some directory has not been found.

@kelset kelset requested a review from dulmandakh January 7, 2020 10:02
Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

This is standard - any time you bump gradle you have to commit the full set of changes - the dependency in the gradle settings file, as well as the shell wrappers and the jar. Looks like some was just missed in moving gradle to the new version and this cleans that up? Which is easy to agree with

Copy link
Contributor

@friederbluemle friederbluemle left a comment

Choose a reason for hiding this comment

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

👍 Yes, this should be merged. Gradle 6.0+ comes with these updated wrapper scripts. Most likely what happened is that a previous version (Gradle 5.0+) was used to upgrade Gradle wrapper to 6.0 (which of course did not contain the new scripts yet). Running the wrapper task using Gradle 6.0.1 locally yields zero changed lines against the PR head branch.

Btw, the case items are not a syntax error, but it is more conventional to omit the (.

@friederbluemle
Copy link
Contributor

In ci/circleci: test_android's log, it seems it can build Android RNTester App successfully and it failed in Collect Test Results phase because some directory has not been found.

Please check again after this PR has been merged. The error is most likely caused by something else.

Copy link
Contributor

@dulmandakh dulmandakh left a comment

Choose a reason for hiding this comment

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

LGTM. I use gradle wrapper to upgrade versions, like ./gradlew wrapper --gradle-version=6.0.1 --distribution-type=all, and it's strange that it didn't produce this change. Or I made a mistake. Sorry.

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@friederbluemle
Copy link
Contributor

LGTM. I use gradle wrapper to upgrade versions, like ./gradlew wrapper --gradle-version=6.0.1 --distribution-type=all, and it's strange that it didn't produce this change. Or I made a mistake. Sorry.

No worries :)

Tip for the future: When updating between major versions (or when updates to the wrapper scripts are expected), run the command twice: The first time ./gradlew it still the old version (unaware of the new scripts).

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @SaeedZhiany in aa0ef15.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jan 13, 2020
@SaeedZhiany SaeedZhiany deleted the FixGradleWrapperFiles branch January 15, 2020 05:29
osdnk pushed a commit to osdnk/react-native that referenced this pull request Mar 9, 2020
…k#27639)

Summary:
This PR is related to facebook#27290.
I just upgraded my project's Gradle wrapper version to 6.0.1 and I realized some files have some differences with the files in react-native `template` folder. so I create this PR to apply differences.
the main difference is in the `gradlew` file. I'm not familiar with Linux shell scripts but it seems there was a syntax error in `case` items syntax. `(` should not be used in declaring case's items. it may has building error in Linux OS.

## Changelog

[Android] [Fixed] - Applied missing changes from bumping Gradle wrapper to 6.0.1
Pull Request resolved: facebook#27639

Test Plan: I have no Linux OS right now, so I can't directly test these changes, but because the changes have made by running `gradlew wrapper` command, it should not break CI. (I hope :) )

Differential Revision: D19341671

Pulled By: cpojer

fbshipit-source-id: ccfc3c12af3f5468671737e5ba0b1674b4491590
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants