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

Remove unused variable NODE_MODULES_DIR from build.gradle in app template #34459

Closed
wants to merge 1 commit into from
Closed

Conversation

tomekzaw
Copy link
Contributor

Summary

This PR removes unused variable NODE_MODULES_DIR passed from build.gradle to CMakeLists.txt which causes the following CMake warnings to appear in the logs:

> Task :app:configureCMakeDebug[arm64-v8a]
C/C++: debug|arm64-v8a :CMake Warning:
C/C++: debug|arm64-v8a :  Manually-specified variables were not used by the project:
C/C++: debug|arm64-v8a :    NODE_MODULES_DIR

First I changed the value of NODE_MODULES_DIR to some non-existent path (i.e. -DNODE_MODULES_DIR=/foo/bar) to confirm that the variable is indeed unused. Then I completely removed it from arguments and the CMake warning disappeared.

Changelog

[Android] [Fixed] - Removed unused variable NODE_MODULES_DIR from build.gradle in app template

Test Plan

  1. Create a new RN 0.70.0-rc.3 app from template with npx react-native@next init RN070RC3 --version 0.70.0-rc.3
  2. Set newArchEnabled=true in settings.gradle
  3. Open android directory in Android Studio
  4. Run Gradle Sync
  5. Build the app
  6. Search for NODE_MODULES_DIR in the logs
  7. Notice the CMake warning
  8. Remove the line from this PR
  9. Build the app again
  10. Search for NODE_MODULES_DIR in the logs
  11. Confirm there are no occurrences

@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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 19, 2022
@react-native-bot react-native-bot added Bug Platform: Android Android applications. labels Aug 19, 2022
Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Yup that's not needed, it was a leftover from Android.mk.
I've also checked both in the CLI/Autolinking and there is no need for this variable anymore

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,615,166 +0
android hermes armeabi-v7a 7,030,150 +0
android hermes x86 7,915,638 +0
android hermes x86_64 7,889,604 +0
android jsc arm64-v8a 9,494,197 +0
android jsc armeabi-v7a 8,272,050 +0
android jsc x86 9,432,261 +0
android jsc x86_64 10,025,334 +0

Base commit: e142de0
Branch: main

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @tomekzaw in 50b1270.

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 Aug 19, 2022
@cortinico
Copy link
Contributor

This could be cherry picked in 0.70 RC4. How do you feel @tomekzaw @kelset @dmitryrykun

@dmytrorykun
Copy link
Contributor

@cortinico looks pick worthy to me.

@tomekzaw tomekzaw deleted the patch-1 branch August 19, 2022 15:40
@analysis-bot
Copy link

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

Base commit: 50b1270
Branch: main

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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants