Skip to content

Conversation

@gnprice
Copy link
Contributor

@gnprice gnprice commented Nov 4, 2022

Summary

This fixes #35204. It ensures that for getting the Android binary dependencies provided by React Native, the build always looks only in the local Maven repo inside node_modules/react-native/.

Without this, the build also looks in the other configured repositories like Maven Central. Those sometimes have RN binaries in them, by mistake or otherwise, which may be the wrong version.

For how this snippet works, see Gradle documentation:
https://docs.gradle.org/current/userguide/declaring_repositories.html#sec:repository-content-filtering

Thanks to inckie for pointing out this feature:
#35204 (comment)

Changelog

[Android] [Fixed] - Always get React Native Android libraries from node_modules

Test Plan

I applied this change to my own project where #35204 was reproducing, and the build started succeeding again.

This fixes facebook#35204.  It ensures that for getting the Android binary
dependencies provided by React Native, the build always looks only
in the local Maven repo inside node_modules/react-native/.

Without this, the build also looks in the other configured
repositories like Maven Central.  Those sometimes have RN binaries
in them, by mistake or otherwise, which may be the wrong version.

For how this snippet works, see Gradle documentation:
  https://docs.gradle.org/current/userguide/declaring_repositories.html#sec:repository-content-filtering

Thanks to inckie for pointing out this feature:
  facebook#35204 (comment)
@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 Nov 4, 2022
@react-native-bot react-native-bot added Bug Platform: Android Android applications. labels Nov 4, 2022
gnprice added a commit to gnprice/zulip-mobile that referenced this pull request Nov 4, 2022
This fixes a build failure which started happening today due to
an operational mistake in React Native release management:
  facebook/react-native#35204

Happily Gradle gives us a way to more precisely pin down what we
want it to do when finding dependencies, which makes the build
robust to this and any similar issue.  I've sent the same fix
upstream for the template app:
  facebook/react-native#35208

See also where we spotted the issue in CI:
  zulip#5524 (comment)
and discussion in chat:
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/build.20failure.3A.20libfbjni.2Eso.20duplicated/near/1459787
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 6,995,344 +0
android hermes armeabi-v7a 6,371,882 +0
android hermes x86 7,408,031 +0
android hermes x86_64 7,272,040 +0
android jsc arm64-v8a 8,861,159 +0
android jsc armeabi-v7a 7,599,671 +0
android jsc x86 8,918,904 +0
android jsc x86_64 9,402,268 +0

Base commit: f0b7cbe
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: f0b7cbe
Branch: main

@pull-bot
Copy link

pull-bot commented Nov 5, 2022

PR build artifact for c5c32cb is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@mikehardy
Copy link
Contributor

@cortinico + @brentvatne you may both be interested in this - it came from the collaboration on the github issue related to the android build break today

@gnprice
Copy link
Contributor Author

gnprice commented Nov 5, 2022

CI is failing. It looks like the issue is that the CI job relies on passing an extra Gradle property REACT_NATIVE_MAVEN_LOCAL_REPO to specify where to look for the local Maven repo:

# .circleci/config.yml
            ./gradlew assemble<< parameters.flavor >> -PREACT_NATIVE_MAVEN_LOCAL_REPO=/root/react-native/maven-local

Then the way that property is given meaning is this bit in the RN Gradle plugin, in packages/react-native-gradle-plugin/src/main/kotlin/com/facebook/react/utils/DependencyUtils.kt:

  fun configureRepositories(project: Project, reactNativeDir: File) {
    with(project) {
      if (hasProperty("REACT_NATIVE_MAVEN_LOCAL_REPO")) {
        mavenRepoFromUrl("file://${property("REACT_NATIVE_MAVEN_LOCAL_REPO")}")
      }

That is, when the property is present, the plugin configures an additional Maven repo based on it. That gets defeated when there's already an exclusiveContent pointing elsewhere for the desired content, as there is with this PR.

I think it should be straightforward to fix things so that this fix for #35204 works without interfering with the use of the REACT_NATIVE_MAVEN_LOCAL_REPO property -- the simplest way is probably to adjust the template app/build.gradle code so that it too respects that property. I'll take a look at that sometime in the next few days.

Perhaps better: it seems like that code in the RN Gradle plugin is already doing a job that overlaps with that code in the template app/build.gradle script. Perhaps that logic can be deduplicated, simplifying the script? That'd be neat but possibly out of scope for fixing the present issue. I might take a look at that after I get a working fix that passes CI.


To be explicit for anyone seeing this thread while looking for a workaround to #35204: the fix in this PR is still a good one to apply to individual apps (even before the further wrinkles I intend to add to it.)

The case where it doesn't work is if you're passing a Gradle property REACT_NATIVE_MAVEN_LOCAL_REPO, as in ./gradlew -PREACT_NATIVE_MAVEN_LOCAL_REPO=…, to specify an alternate location for the Android React Native libraries. That's an undocumented feature of the React Native build system, so most people are unlikely to be using it.

@cortinico
Copy link
Contributor

Hey @gnprice
Thanks for sending this PR.

This change as it is, it's actually unnecessary as it will land on main (so wilk be shipped on 0.72).

You should send this same PR agains the 0.68-stable, 0.69-stable and 0.70-stable.

0.71 is instead unaffected as it will consume artifacts from Maven Central. That's also the reason why the CI is red.

@cortinico
Copy link
Contributor

Closing for the aforemntioned reasons + we provided hotfixes for the last 8 stable releases

@cortinico cortinico closed this Nov 7, 2022
@gnprice
Copy link
Contributor Author

gnprice commented Nov 8, 2022

0.71 is instead unaffected as it will consume artifacts from Maven Central

Ah, and I see the explanation that's now on #35210:

In 0.71 we cleaned up the new app template and removed all the + dependencies (see here) and we moved the template to use the React Native Gradle Plugin, which will allow us to prevent scenarios like this from happening in the future.

That's great news! Glad this is already fixed thoroughly in main, and thanks for providing those stable-version fixes.

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. Platform: Android Android applications.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[HAS WORKAROUND] Android build fails since 0.71.0-rc.0 tag added

7 participants