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

Fix building Android on Windows #39190

Closed
wants to merge 1 commit into from
Closed

Conversation

alespergl
Copy link
Contributor

Summary:

Currently Android fails to build on Windows, because CMake cannot work with
native Windows paths that are passed to it as build arguments. This change
converts the input paths to the CMake format.

Changelog:

[Android] [Fixed] - Fix building Android on Windows.

Test Plan:

Build the React Native Android project on Windows. It should complete successfully.

@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: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 29, 2023
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

Amazing, thanks @alespergl for this.
I'm adding a suggestion as I'm a bit scared that this could break other pipelines otherwise.

What do you think?

Comment on lines +11 to +14
# Convert input paths to CMake format (with forward slashes)
file(TO_CMAKE_PATH "${REACT_ANDROID_DIR}" REACT_ANDROID_DIR)
file(TO_CMAKE_PATH "${REACT_BUILD_DIR}" REACT_BUILD_DIR)
file(TO_CMAKE_PATH "${REACT_COMMON_DIR}" REACT_COMMON_DIR)
Copy link
Contributor

Choose a reason for hiding this comment

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

@alespergl I'm not an expert of CMake, but can we wrap this change in a platform check?
Something like:

if(${CMAKE_SYSTEM_NAME} MATCHES "Win32") 
  file(TO_CMAKE_PATH "${REACT_ANDROID_DIR}" REACT_ANDROID_DIR)
	file(TO_CMAKE_PATH "${REACT_BUILD_DIR}" REACT_BUILD_DIR)
	file(TO_CMAKE_PATH "${REACT_COMMON_DIR}" REACT_COMMON_DIR)
endif

I'm scared this could cause harm on other systems, otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

using file(TO_CMAKE_PATH should actually be correct on every platform.
As the CI is green, we can just merge it 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this operation should be cross-platform.

Note: There is a similar function cmake_path which seems to be preferred, but requires CMake 3.20 or later, whereas the minimum specified in this CMakeLists.txt is 3.13.

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,961,799 -66
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 9,554,866 -44
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: b8bf393
Branch: main

@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.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Sep 4, 2023
@facebook-github-bot
Copy link
Contributor

@cortinico merged this pull request in 054ab62.

fortmarek pushed a commit that referenced this pull request Sep 4, 2023
Summary:
Currently Android fails to build on Windows, because CMake cannot work with
native Windows paths that are passed to it as build arguments. This change
converts the input paths to the CMake format.

## Changelog:

[Android] [Fixed] - Fix building Android on Windows.

Pull Request resolved: #39190

Test Plan: Build the React Native Android project on Windows. It should complete successfully.

Reviewed By: NickGerleman

Differential Revision: D48948140

Pulled By: cortinico

fbshipit-source-id: 6d584c2a10e683cdb6df0dd6dcd5875da7e06e2b
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. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner 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.

5 participants