-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Android builds with New Arch fail on Windows build host #36475
Comments
Tested: - Windows build - Android (Old Arch) build on Windows Presumably works: - Android (Old Arch) build on Ubuntu - Android (New Arch) build on Ubuntu Broken: - Android (New Arch) build on Windows; but I believe, it is due to bugs in React Native itself, at least I found and filed a bug in Codegen: facebook/react-native#36475 and even with that bug fix, the build still fails later, seemingly, due to some other bugs in RN (New Arch) on Windows build host. To be tested: - iOS (Old Arch) build - iOS (New Arch) build
Tested: - Windows build - Android (Old Arch) build on Windows - iOS (Old Arch) build Presumably works: - Android (Old Arch) build on Ubuntu - Android (New Arch) build on Ubuntu Broken: - Android (New Arch) build on Windows; but I believe, it is due to bugs in React Native itself, at least I found and filed a bug in Codegen: facebook/react-native#36475 and even with that bug fix, the build still fails later, seemingly, due to some other bugs in RN (New Arch) on Windows build host. To be tested: - iOS (New Arch) build
Thanks for reporting this. Yup we should fix this one. Are you up for sending a PR against codegen? |
Yeap, I'll probably do the next week. Still have to figure out what else doesn't work right there. |
Hey @cortinico , I created a PR in Codegen above, fixing I tried to further look into why my build still fail, could not figure it out so far. As you see in the screenshot below, it now fails in Ninja build step, saying "No such file or directory", but as you see in the lower half of the screenshot, that folder is actually created by Ninja, although empty. I tried to look around for some additional info in the logs / ninja.build file, so far could not understand why it happens. Saw in Google, it might be a consequence of long pathnames and little Thus, I decided to make a pause, create the PR with the first bit of the fix, and ask you for advice, if you have ideas what can be the trouble with Ninja, and what / where should I look to figure it out? |
Let's merge the first PR fix and get back to it 👍 I don't normally develop on Windows but I do have a windows machine and can help debug. Also @mganandraj could help as well here |
@cortinico I've changed the PR. It turned out that
Same here 😅 I just happened to enter a rabbit hole with developing @dr.pogodin/react-native-static-server library, and although not many people use it yet, some already asked about Windows support, and that's how I bumped into this issue here 🤷♂️ |
Hi @birdofpreyru |
And the schema generation from a typescript file work fine on my Windows machine without fixing the slashes the directory input .. Could you share some more details ? such as which shell your are running from, and the full command line ? |
Hi @mganandraj 👋
Tried, it does not help.
PowerShell 7.3.3
So, I am working on this RN library, set up using create-react-native-library for scaffolding. All standard stuff: react-native/packages/react-native-codegen/src/cli/combine/combine-js-to-schema-cli.js Lines 24 to 28 in e188585
file contains \ the glob.sync does not match anything, generates empty schema, while with \ replaced by / it matches files. It is consistent with what glob README tells about slashes — by default it considers \ as escape characters, not path separators, even on Windows.
|
Hi @birdofpreyru But the schema file and artefacts are generated at react-native-static-server\android\build\generated\source\codegen, and not under "examples" ! Looks like the repo has a react-native project nested within another which is confusing the codegen scripts .. |
@mganandraj well... I don't know... maybe you have something configured for PowerShell to use Linux-style path separators, if there is such option? I just got it installed as it comes, and when I do
This is the standard project layout by https://www.npmjs.com/package/create-react-native-library : the RN library is in the root of the codebase, and an example app using it is inside |
Well... it does not. I just worked it around by manually executing in the codebase root: .\node_modules\.bin\react-native-windows-codegen
--libraryName RNReactNativeStaticServerSpec
--file .\src\NativeReactNativeStaticServer.ts
--namespace winrt::ReactNativeStaticServer
--modulesWindows true then moving and committing generated NativeReactNativeStaticServerSpec.g.h as a part of library code. With this done, then Example app builds and run from MSVS. Without it, it does not generate that header (and schema i guess), and the Example build fails because of it. |
(cc @kelset if we need to pass this to someone at Microsoft) |
cc @ZihanChen-MSFT maybe you know how to help here? |
Sorry .. I got extremely busy last week .. Adding @rasaha91 and @shivenmian who can help as well . |
Summary: Android builds with new arch fail on Windows build host (#36475). This tiny correction fixes generation of `schema.json` (without it codegen failed to find any files, and generated empty schema). Unfortunately, does not fixes the issue with Windows host entirely, as builds still fail later (on ninja build step). ## Changelog: [Android] [Fixed] - A bug fix for Android builds with new arch on Windows host. Pull Request resolved: #36542 Reviewed By: NickGerleman Differential Revision: D48563587 Pulled By: cortinico fbshipit-source-id: acd510308ce9768fb17d3a33c7927de3237748ac
For the record, it looks like PR #39190 aims to fix the CMake part of the issue. |
Summary: Android builds with new arch fail on Windows build host (#36475). This tiny correction fixes generation of `schema.json` (without it codegen failed to find any files, and generated empty schema). Unfortunately, does not fixes the issue with Windows host entirely, as builds still fail later (on ninja build step). ## Changelog: [Android] [Fixed] - A bug fix for Android builds with new arch on Windows host. Pull Request resolved: #36542 Reviewed By: NickGerleman Differential Revision: D48563587 Pulled By: cortinico fbshipit-source-id: acd510308ce9768fb17d3a33c7927de3237748ac
@birdofpreyru as #39190 is merged, can we close this one? |
I usually like to keep my tickets open until the fix is released with a regular version of a product, and the issue is verified to be fixed by it. Especially in this case, I have no idea yet whether that and other PRs will completely fix the problem, or not. |
Great I've assigned this to you so we know you're on top of it. I general we tend to close issues more proactively as we have so many, and we close issues once a fix lands on main and eventually re-open. I'm fine keeping this open though |
@birdofpreyru I'm closing this one as we're grooming New Architecture issues. I know that the fix hasn't been shipped yet (will be in 0.73), we can reopen this issue if the problem pops back. |
Hey @cortinico , at last I arrived to do a rapid test, trying to build the example app of my lib for Android on Windows with the new RN arch, and it still does not work for me :( According to my understanding of this log piece, it seems the JNI project generated by RN Codegen easily hits the CMake restriction on the maximum path length on Windows. Perhaps setting in the generated |
Description
The following line is wrong, because
glob
expects patterns with/
separators, even on Windows; but when the codegen is executed on Windows thefile
will contain\
separators, causing that query not matching anything:react-native/packages/react-native-codegen/src/cli/combine/combine-js-to-schema-cli.js
Line 25 in ca0d565
Changing that line into:
fixes the issue (well, I verified that codegen then matches expected files there, but it still does not generate expected outputs, so I am still looking for other issues down the line).
React Native Version
0.71.4
Output of
npx react-native info
N/A
Steps to reproduce
N/A
Snack, code example, screenshot, or link to a repository
N/A
The text was updated successfully, but these errors were encountered: