-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
fix: jvm 11 error message from ReactPlugin.kt and react.gradle #33048
Conversation
Base commit: cd79317 |
Base commit: cd79317 |
3c27c7f
to
515f8be
Compare
b92755c
to
eb32655
Compare
I like this as a guard rail, the only thing I don't like is that it will require maintenance going forward, now there will be 3 spots to bump the JDK version in if the requirement moves. Isn't the real requirement that JDK is > 8? So perhaps there could be an integer parse and check for > 8 vs a fixed "11. That way JDK14 and 16 etc will work with no further work? |
Agree. That's why having this inside |
eb32655
to
691c4f3
Compare
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 like this a lot more! it won't just irritatingly break in the future now, but does protect against the JDK8 past.
The only thing I can think of now is that the error message is not really consistent with the test(s) anymore. Should probably say "... requires JDK11 or higher" ?
I do think this error message will help a lot of people though, even in the much much smaller developer community that is the AnkiDroid project (maybe 20 developers?) this helped a few people. Here in react-native for 0.68 I bet this helps hundreds, most of whom probably do not know about doctor
yet
@cortinico Agree, I will also make another PR for doctor CLI. @mikehardy you are right, I am done with the changes that you told |
691c4f3
to
09afa5e
Compare
@mikehardy Thanks, I have changed the error message with |
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.
Near as I can tell, it's a thing of beauty! 👍
@@ -50,6 +52,19 @@ java { | |||
targetCompatibility = JavaVersion.VERSION_11 |
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.
Please change those two to JavaVersion.VERSION_8
otherwise the build won't even start on JDK 8
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.
as far as I know. users of old releases will use older versions of react-native-gradle-plugin
. let me change it to JavaVersion.VERSION_1_8
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.
@cortinico with JavaVersion.VERSION_1_8
i am getting this error
Build file '/Users/apple/Data/react-native/ReactAndroid/build.gradle' line: 9
* What went wrong:
An exception occurred applying plugin request [id: 'com.android.library']
> Failed to apply plugin 'com.android.internal.library'.
> Android Gradle plugin requires Java 11 to run. You are currently using Java 1.8.
You can try some of the following options:
- changing the IDE settings.
- changing the JAVA_HOME environment variable.
- changing `org.gradle.java.home` in `gradle.properties`.
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.
Are you building with JDK 11 right?
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.
@cortinico I am getting this error for java 1.8. with JDK 11 it is working well. should and replaceJavaVersion.VERSION_11
withJavaVersion.VERSION_1_8
, I think, we should not replace it because there is no benefit according to me what do you say?
packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/ReactPlugin.kt
Outdated
Show resolved
Hide resolved
09afa5e
to
f140b44
Compare
f140b44
to
228ae8e
Compare
@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This pull request was successfully merged by @nomi9995 in 4e947ec. When will my fix make it into a release? | Upcoming Releases |
Summary: you can see discussion here: reactwg/react-native-releases#13 (comment) we were getting this error message when we build Gradle with other than 11 JVM ``` > Task :react-native-gradle-plugin:compileJava FAILED 2 actionable tasks: 2 executed FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':react-native-gradle-plugin:compileJava'. > invalid source release: 11 ``` this solution is suggested by mikehardy after this PR, now the error is like this ``` ************************************************************************************************************** ERROR: requires JDK11 or higher. Incompatible major version detected: '8' ************************************************************************************************************** ``` ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://github.com/facebook/react-native/wiki/Changelog --> [Android] [Fixed] - jvm 11 error message Pull Request resolved: #33048 Test Plan: install other than 11 java version and just run `./scripts/test-manual-e2e.sh` this command at the root of RN repo than this error will appair `invalid source release: 11` Reviewed By: ShikaSD Differential Revision: D34110990 Pulled By: cortinico fbshipit-source-id: c142a363c7cec0db65d5ab9da858fd25866c7c49
Summary
you can see discussion here: reactwg/react-native-releases#13 (comment)
we were getting this error message when we build Gradle with other than 11 JVM
this solution is suggested by @mikehardy
after this PR, now the error is like this
Changelog
[Android] [Fixed] - jvm 11 error message
Test Plan
install other than 11 java version and just run
./scripts/test-manual-e2e.sh
this command at the root of RN repo than this error will appairinvalid source release: 11