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

Use NDK 23 only for Windows users. #33611

Closed
wants to merge 1 commit into from

Conversation

cortinico
Copy link
Contributor

Summary:
Bumping the NDK to 23 to prevent build failures due to the NDK
using longer paths.

Changelog:
[Android] [Fixed] - Use NDK 23 only for Windows users.

Differential Revision: D35547459

Summary:
Bumping the NDK to 23 to prevent build failures due to the NDK
using longer paths.

Changelog:
[Android] [Fixed] - Use NDK 23 only for Windows users.

Differential Revision: D35547459

fbshipit-source-id: 82fc0c843528e2c81b4cf8f66e3ef832e0027dec
@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. fb-exported p: Facebook Partner: Facebook Partner labels Apr 11, 2022
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D35547459

@analysis-bot
Copy link

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

Base commit: 65abc36
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,770,560 +0
android hermes armeabi-v7a 7,176,577 +0
android hermes x86 8,079,751 +0
android hermes x86_64 8,058,467 +0
android jsc arm64-v8a 9,645,279 +0
android jsc armeabi-v7a 8,419,644 +0
android jsc x86 9,594,494 +0
android jsc x86_64 10,191,921 +0

Base commit: 65abc36
Branch: main

@RalissonMattias
Copy link

RalissonMattias commented Apr 11, 2022

@cortinico I can help in the tests, basically just change the file?

@cortinico
Copy link
Contributor Author

cortinico commented Apr 11, 2022

@cortinico I can help in the tests, basically just change the file?

Thanks for that @RalissonMattias

You can find simple instructions on how to test it here: https://gist.github.com/cortinico/6a297ac4c6a2a4633f58f1e4b375dd88

Please publish the gradle scans once you're done with it.

@RalissonMattias
Copy link

@cortinico I can help in the tests, basically just change the file?

Thanks for that @RalissonMattias

You can find simple instructions on how to test it here: https://gist.github.com/cortinico/6a297ac4c6a2a4633f58f1e4b375dd88

Please publish the gradle scans once you're done with it.

I tested here and it didn't work
Captura de tela 2022-04-11 133734

@cortinico
Copy link
Contributor Author

I tested here and it didn't work

I've updated the .patch file in gist with a new one (fix33611-v2.patch). Could you give it another try (make sure to delete the project and re-create it).

@Ralisson-Mattias
Copy link

After applying the new patch, generate new error
image

@cortinico
Copy link
Contributor Author

After applying the new patch, generate new error

Thanks for the feedback. I ended up finding a windows machine and finding a (hopefully) working solution. It works locally for me but I'd like someone else to give it another try.

I've updated the .patch file in gist (here https://gist.github.com/cortinico/6a297ac4c6a2a4633f58f1e4b375dd88) with a new one (fix33611-v3.patch). Could you give it another try (make sure to delete the project and re-create it).

@cortinico cortinico marked this pull request as draft April 12, 2022 11:47
@RalissonMattias
Copy link

Here still no success.

First git applyt throws errors:
image

After manually inserting only the fix33611-v3.patch:
image

After manually combining and entering patches v1 and v3:
image

@mikehardy
Copy link
Contributor

@cortinico I gave it a go.
-- ❌ First build failed https://gradle.com/s/qzgdhi6kbxlbc
-- ❌ Second build still failed, using v3 of patch from gist https://gradle.com/s/uly2zz74hljzo
-- ironically, when I run git apply fix33611-v3.patch as downloaded via "raw" view mode + save file from browser, in powershell I get "Corrupt file at line 40" which seems to be some sort of unix vs dos patch format thing. Using unix2dos did not result in success but using notepad fix33611-v3.patch and just adding a newline at end of file + save + close resulted in patch that successfully applied 🤷


  make: *** Deleting file 'C:\Users\micro\work\react-random\RN068\.cxx/local/armeabi-v7a/librrc_slider.a'
  C:/Users/micro/AppData/Local/Android/Sdk/ndk/21.4.7075529/build//../toolchains/llvm/prebuilt/windows-x86_64/bin/arm-linux-androideabi-ar: C:\Users\micro\work\react-random\RN068\.cxx/local/armeabi-v7a/objs-debug/rrc_slider/C_/Users/micro/work/react-random/RN068/node_modules/react-native/ReactAndroid/__/ReactCommon/react/renderer/components/slider/platform/android/react/renderer/components/slider/SliderMeasurementsManager.o: No such file or directory
  make: *** [C:/Users/micro/AppData/Local/Android/Sdk/ndk/21.4.7075529/build//../build/core/build-binary.mk:600: C:\Users\micro\work\react-random\RN068\.cxx/local/armeabi-v7a/librrc_slider.a] Error 1
  make: *** Waiting for unfinished jobs....
  make: *** Deleting file 'C:\Users\micro\work\react-random\RN068\.cxx/local/armeabi-v7a/librrc_progressbar.a'
  C:/Users/micro/AppData/Local/Android/Sdk/ndk/21.4.7075529/build//../toolchains/llvm/prebuilt/windows-x86_64/bin/arm-linux-androideabi-ar: C:\Users\micro\work\react-random\RN068\.cxx/local/armeabi-v7a/objs-debug/rrc_progressbar/C_/Users/micro/work/react-random/RN068/node_modules/react-native/ReactAndroid/__/ReactCommon/react/renderer/components/progressbar/android/react/renderer/components/progressbar/AndroidProgressBarShadowNode.o: No such file or directory
  make: *** [C:/Users/micro/AppData/Local/Android/Sdk/ndk/21.4.7075529/build//../build/core/build-binary.mk:600: C:\Users\micro\work\react-random\RN068\.cxx/local/armeabi-v7a/librrc_progressbar.a] Error 1
  make: *** Deleting file 'C:\Users\micro\work\react-random\RN068\.cxx/local/armeabi-v7a/librrc_switch.a'
  C:/Users/micro/AppData/Local/Android/Sdk/ndk/21.4.7075529/build//../toolchains/llvm/prebuilt/windows-x86_64/bin/arm-linux-androideabi-ar: C:\Users\micro\work\react-random\RN068\.cxx/local/armeabi-v7a/objs-debug/rrc_switch/C_/Users/micro/work/react-random/RN068/node_modules/react-native/ReactAndroid/__/ReactCommon/react/renderer/components/switch/androidswitch/react/renderer/components/androidswitch/AndroidSwitchShadowNode.o: No such file or directory
  make: *** [C:/Users/micro/AppData/Local/Android/Sdk/ndk/21.4.7075529/build//../build/core/build-binary.mk:600: C:\Users\micro\work\react-random\RN068\.cxx/local/armeabi-v7a/librrc_switch.a] Error 1

-- ✔️ Advised to move project from C:\Users\micro\work\react-random\RN068 to root of C: and re-try, so, a recompile while it is in C:\RN068 worked https://gradle.com/s/2yhpn6nkfxjro

@cortinico
Copy link
Contributor Author

-- ✔️ Advised to move project from C:\Users\micro\work\react-random\RN068 to root of C: and re-try, so, a recompile while it is in C:\RN068 worked gradle.com/s/2yhpn6nkfxjro

Awesome thanks for giving it a try. I'll close this PR as we'll merge this instead:

Which contains the v3 patch I've added in the gist.

I hope that with RN 0.69, as we moved to CMake, those kind of problems won't appear anymore.

@cortinico cortinico closed this Apr 12, 2022
@RalissonMattias
Copy link

@cortinico I know you already closed PR but I wanted to report that following the steps of @mikehardy worked correctly here too. Good job, see you later

https://gradle.com/s/qxh3ndc6bel4c
https://gradle.com/s/c4r6epirook4m

@cortinico
Copy link
Contributor Author

I know you already closed PR but I wanted to report that following the steps of @mikehardy worked correctly here too. Good job, see you later

Thanks for your feedback @Ralisson-Mattias 🙏 Yup I've closed this one as we're going to merge the other PR: #33582
Thanks again for testing this on your windows machine.

@mganandraj
Copy link
Contributor

On my trials, ndk23 is able to handle long paths, but the build fails because of long commmand lines (>8192 characters) .. I've created another PR with the ndk bump too ..

@cortinico cortinico reopened this Apr 13, 2022
@cortinico cortinico reopened this Apr 13, 2022
@cortinico cortinico marked this pull request as ready for review April 13, 2022 12:15
fortmarek pushed a commit that referenced this pull request Apr 13, 2022
Summary:
Pull Request resolved: #33611

Bumping the NDK to 23 to prevent build failures due to the NDK
using longer paths.

Changelog:
[Android] [Fixed] - Use NDK 23 only for Windows users.

Reviewed By: motiz88

Differential Revision: D35547459

fbshipit-source-id: 4e44c0b5fd8d1c559b04fb43eb4eeadc13943394
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @cortinico in e48a580.

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 Apr 13, 2022
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
Summary:
Pull Request resolved: facebook#33611

Bumping the NDK to 23 to prevent build failures due to the NDK
using longer paths.

Changelog:
[Android] [Fixed] - Use NDK 23 only for Windows users.

Reviewed By: motiz88

Differential Revision: D35547459

fbshipit-source-id: 4e44c0b5fd8d1c559b04fb43eb4eeadc13943394
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. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants