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

[LOCAL] Make sure Java Toolchain and source/target level is applied to all projects #37576

Merged
merged 2 commits into from
May 26, 2023

Conversation

cortinico
Copy link
Contributor

Summary:

This should fix reactwg/react-native-releases#54 (comment) by making sure we set JDK source/target level to 11 for all the modules.

Changelog:

[INTERNAL] - Make sure Java Toolchain and source/target level is applied to all projects

Test Plan:

cc @Kudo

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels May 25, 2023
@Kudo
Copy link
Contributor

Kudo commented May 26, 2023

the solution works for the me. however i feel the way to apply java version for all projects is intrusive and afraid if that may break some third party libraries. especially this is done in RNGP where people don't have a way to change it (expect patch-package), would recommend to have a special property to disable this feature. so if there are some errors, people could add something like reactNativeGradlePlugin.forceJavaVersion=false into gradle.properties.

@cortinico
Copy link
Contributor Author

so if there are some errors, people could add something like reactNativeGradlePlugin.forceJavaVersion=false into gradle.properties.

Yup I could and it probably makes sense. However there is no way to selectively disable this for some of the libraries

@Kudo
Copy link
Contributor

Kudo commented May 26, 2023

However there is no way to selectively disable this for some of the libraries

so we may have comma splitting string for the property like reactNativeGradlePlugin.forceJavaVersionExcludedProjects
not sure whether that's too complicated. anyway, either way (disable all or disable selectively) looks good to me.

  fun configureJavaToolChains(input: Project) {
    val excludeProjects = (input.rootProject.findProperty("reactNativeGradlePlugin.forceJavaVersionExcludedProjects") as String?)?.split(",") ?: emptyList()
    input.rootProject.allprojects { project ->
      if (excludeProjects.contains(project.name)) {
        return@allprojects
      }
      // ...
    }
  }

@cortinico
Copy link
Contributor Author

@Kudo I've added a killswitch. I don't think there will be scenarios where we need to selectively turn this off for a library as Java versions are generally forward compatible (i.e. Java 8 code is valid Java 11 code).

If this is not the case, we can add specific handling in a point release

@cortinico cortinico requested review from jakeblakeley, cipolleschi and kelset and removed request for jakeblakeley May 26, 2023 10:46
* configure the toolchain to 11.
*/
fun configureJavaToolChains(input: Project) {
if (input.hasProperty(INTERNAL_DISABLE_JAVA_VERSION_ALIGNMENT)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it's hasProperty so react.internal.disableJavaVersionAlignment=false will also disable the java version enforcement. but it doesn't really matter though 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't have permission to approve the pr, but it looks good to me. thanks @cortinico.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's hasProperty so react.internal.disableJavaVersionAlignment=false will also disable the java version enforcement. but it doesn't really matter though 😅

Yeah I prefer this approach as the property is called disable... so we won't have to handle all the boolean cases (true/True/1/etc). Plus it's an .internal. property so no need to be super strict here

@kelset
Copy link
Contributor

kelset commented May 26, 2023

(CI is failing 'cause it's not rebased on top of the branch, go ahead and merge anyway, we're still addressing all the various ❌)

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,545,805 -194,752
android hermes armeabi-v7a 7,860,130 -191,491
android hermes x86 9,024,833 -206,050
android hermes x86_64 8,879,939 -202,291
android jsc arm64-v8a 9,145,587 -157,578
android jsc armeabi-v7a 8,335,670 -156,164
android jsc x86 9,198,939 -165,631
android jsc x86_64 9,457,119 -162,906

Base commit: a244209
Branch: main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner Pick Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants