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.

Sign up for GitHub

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

Jump to bottom

fix build failure on windows in android #47641

Closed

FouadMagdy01 wants to merge 3 commits into facebook:main from FouadMagdy01:fix-cmake-invalid-escape-character

Contributor

FouadMagdy01 commented Nov 15, 2024 •

edited

Loading

Summary:

This pull request addresses a CMake configuration issue where an invalid escape character in file paths caused the build process to fail. Specifically, it resolves the issue in the React Native CMake configuration file where the path separator was incorrectly handled, leading to an error in the build system.

the issue is in This Issue and This

Changelog:

[INTERNAL] [FIXED] - Corrected invalid escape character in CMake path handling

Test Plan:

To test the changes, I performed the following steps:

Cloned the repository and checked out the fix-cmake-invalid-escape-character branch.
Ran the CMake build on a Windows environment where the issue was previously occurring.
Verified that the build process completed successfully without the "invalid character escape" error.
Ensured that the path handling now works correctly in CMake on Windows platforms.


          fix build failure on windows in android

7f22f7b

facebook-github-bot added CLA Signed Shared with Meta labels

brentvatne mentioned this pull request

Windows Build Failed for Android expo/expo#32957

Closed

cipolleschi requested changes

View reviewed changes

Contributor

cipolleschi left a comment

Hi @FouadMagdy01, thank you so much to working on this problem and for trying to fix it.

The code looks good, but as a general rule, let's try to minimize the number of changes we are introducing.
In this PR there are some formatting and some comment changes that are not really needed, so it's better to revert those small changes and only keep the changes that are actually needed.

I also left a couple of questions because there are some changes that are not really clear to me.

Thank you again.

packages/react-native/ReactAndroid/cmake-utils/ReactNative-application.cmake Outdated

Comment on lines 29 to 30

		set_property(GLOBAL PROPERTY RULE_LAUNCH_COMPILE ccache)
		set_property(GLOBAL PROPERTY RULE_LAUNCH_LINK ccache)

Contributor

cipolleschi Nov 17, 2024

can you revert this change? A good practice is to try and minimize the changes between the two files and this formatting is not necessary.
Plus it will probably requires us to reformatting because of different internal formatting setting.

packages/react-native/ReactAndroid/cmake-utils/ReactNative-application.cmake Outdated

-              # variable is defined if user need to access it.
-              endif ()
+              # Glob input source files

Contributor

cipolleschi Nov 17, 2024

Why are we removing this if?

packages/react-native/ReactAndroid/cmake-utils/ReactNative-application.cmake Outdated

-                              -DFOLLY_NO_CONFIG=1
+                      -Wall
+                      -Werror
+                      # Suppress cpp #error and #warning to prevent build failures

Contributor

cipolleschi Nov 17, 2024

could you put back the comment that got removed?

packages/react-native/ReactAndroid/cmake-utils/ReactNative-application.cmake Outdated

-                      foreach(autolinked_library ${AUTOLINKED_LIBRARIES})
-                          target_link_libraries(${autolinked_library} common_flags)
-                      endforeach()
+              # Autolinked libraries if available

Contributor

cipolleschi Nov 17, 2024

Could you put back the removed comment?

packages/react-native/ReactAndroid/cmake-utils/ReactNative-application.cmake Outdated

Comment on lines 91 to 92

		if(EXISTS ${BUILD_DIR}/generated/autolinking/src/main/jni/Android-autolinking.cmake)
		include(${BUILD_DIR}/generated/autolinking/src/main/jni/Android-autolinking.cmake)

Contributor

cipolleschi Nov 17, 2024

Why are we moving from PROJECT_BUILD_DIR to just BUILD_DIR? Which issue does this solve?

packages/react-native/ReactAndroid/cmake-utils/ReactNative-application.cmake Outdated

Comment on lines 93 to 96

+                  target_link_libraries(${CMAKE_PROJECT_NAME} ${AUTOLINKED_LIBRARIES})
+                  foreach(autolinked_library ${AUTOLINKED_LIBRARIES})
+                      target_link_libraries(${autolinked_library} common_flags)
+                  endforeach()

Contributor

cipolleschi Nov 17, 2024

Could you please revert the formatting?

packages/react-native/ReactAndroid/cmake-utils/ReactNative-application.cmake Outdated

-                              -DREACT_NATIVE_APP_COMPONENT_REGISTRATION=${APP_CODEGEN_HEADER}_registerComponentDescriptorsFromCodegen
-                              -DREACT_NATIVE_APP_MODULE_PROVIDER=${APP_CODEGEN_HEADER}_ModuleProvider
-                      )
+              # Link and build the generated library for codegen if available

Contributor

cipolleschi Nov 17, 2024

Could you please keep the old comment?

packages/react-native/ReactAndroid/cmake-utils/ReactNative-application.cmake Outdated

+                  target_link_libraries(${CMAKE_PROJECT_NAME} ${APP_CODEGEN_TARGET})
+                  target_link_libraries(${APP_CODEGEN_TARGET} common_flags)
+                  # Pass generated header and module provider to OnLoad.cpp

Contributor

cipolleschi Nov 17, 2024

Keep the old comment please

packages/react-native/ReactAndroid/cmake-utils/ReactNative-application.cmake Outdated

               endif()
-              # We set REACTNATIVE_MERGED_SO so libraries/apps can selectively decide to depend on either libreactnative.so
-              # or link against a old prefab target (this is needed for React Native 0.76 on).
+              # Set REACTNATIVE_MERGED_SO for libraries/apps to selectively link

Contributor

cipolleschi Nov 17, 2024

Keep the old comment please

Contributor Author

FouadMagdy01 commented Nov 17, 2024

Hello @cipolleschi thank you for your time and replies
I will look into it today and commit the new code
I will also answer all of your questions
Thank you!

FouadMagdy01 added 2 commits

November 17, 2024 18:00


          revert changes

5a5aead


          fix build failure on windows in android and minimize changes

d3e9f2f

FouadMagdy01 commented

View reviewed changes

packages/react-native/ReactAndroid/cmake-utils/ReactNative-application.cmake

Comment on lines +35 to +36

		string(REPLACE "\\" "/" BUILD_DIR ${BUILD_DIR})
		string(REPLACE "\\" "/" REACT_ANDROID_DIR ${REACT_ANDROID_DIR})

Contributor Author

FouadMagdy01 Nov 17, 2024 •

edited

Loading

Problem: The variables REACT_ANDROID_DIR and BUILD_DIR might also contain backslashes if they are set on a Windows system. These backslashes need to be replaced to avoid similar issues.
Solution: Check if the script is running on Windows using if(CMAKE_HOST_WIN32). If it is, replace backslashes with forward slashes for these variables as well:

FouadMagdy01 commented

View reviewed changes

packages/react-native/ReactAndroid/cmake-utils/ReactNative-application.cmake

Comment on lines +48 to +52

+              # Ensure that `input_SRC` paths use forward slashes
+              foreach(path IN LISTS input_SRC)
+                  string(REPLACE "\\" "/" path "${path}")
+              endforeach()

Contributor Author

FouadMagdy01 Nov 17, 2024 •

edited

Loading

Problem: On Windows, backslashes () in file paths are often interpreted by CMake as escape characters, which can lead to invalid sequences like \S. CMake and many other tools can understand forward slashes (/) as directory separators, even on Windows.
Solution: Use string(REPLACE "\" "/" path "${path}") to convert all backslashes in the input_SRC file paths to forward slashes. This ensures CMake correctly interprets the paths without treating them as escape sequences.

Contributor Author

FouadMagdy01 commented Nov 17, 2024 •

edited

Loading

@cipolleschi Can you look into the changes in This new commit

and i am sorry that the comments was removed unintentionally in my first commit

this PR is tested on Android running on Windows 11

cipolleschi approved these changes

View reviewed changes

Contributor

cipolleschi left a comment

Code looks good now! Thank you so much for applying the review's feedback.

Contributor

facebook-github-bot commented Nov 17, 2024

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

FouadMagdy01 mentioned this pull request

not building in windows android - 0.76.2 #47654

Closed

facebook-github-bot closed this in

794154e

facebook-github-bot added the Merged label

Contributor

facebook-github-bot commented Nov 18, 2024

@cipolleschi merged this pull request in 794154e.

Collaborator

react-native-bot commented Nov 18, 2024

This pull request was successfully merged by @FouadMagdy01 in 794154e

^{When will my fix make it into a release? | How to file a pick request?}

javache mentioned this pull request

[0.76.2] windows android build fails #47626

Closed

migueldaipre mentioned this pull request

[CXX1429] error when building with cmake #47666

Closed

lovegaoshi mentioned this pull request

fix: 0.76.2 windows android build #47627

Closed

drsag-mgr mentioned this pull request

Build Failed Cmake configuration issue for new project setup #47729

Closed

2004durgesh mentioned this pull request

Execution failed for task ':react-native-reanimated:buildCMakeRelWithDebInfo[armeabi-v7a] software-mansion/react-native-reanimated#6718

Open

blakef pushed a commit that referenced this pull request


          fix build failure on windows in android (#47641)

08b8300

Summary:
This pull request addresses a CMake configuration issue where an invalid escape character in file paths caused the build process to fail. Specifically, it resolves the issue in the React Native CMake configuration file where the path separator was incorrectly handled, leading to an error in the build system.

the issue is in [This Issue](expo/expo#32955) and [This](expo/expo#32957)

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[INTERNAL] [FIXED] - Corrected invalid escape character in CMake path handling

Pull Request resolved: #47641

Test Plan:
To test the changes, I performed the following steps:

1. Cloned the repository and checked out the `fix-cmake-invalid-escape-character` branch.
2. Ran the CMake build on a Windows environment where the issue was previously occurring.
3. Verified that the build process completed successfully without the "invalid character escape" error.
4. Ensured that the path handling now works correctly in CMake on Windows platforms.

Reviewed By: rshest

Differential Revision: D66073896

Pulled By: cipolleschi

fbshipit-source-id: bd2a71bb00ce5c5509ed403842c995c32f58f91d

Collaborator

react-native-bot commented Nov 21, 2024

This pull request was successfully merged by @FouadMagdy01 in 08b8300

^{When will my fix make it into a release? | How to file a pick request?}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Merged Shared with Meta