Building new architecture sources on Windows #33582

mganandraj · 2022-04-06T23:52:20Z

On Windows there are limits on number of character in file paths and in command lines
Ref: https://android.googlesource.com/platform/ndk/+/master/docs/BuildSystemMaintainers.md#Path-Length-Limits
NDK allows circumventing command line limits using response(RSP) files as inputs using NDK_APP_SHORT_COMMANDS flag.

Windows can support long file paths if configured through registry or by prefixing all file paths with a special character sequence
The latter requires changes in NDK. And there are tools in NDK (AR) which is not able to handle long paths (>256) even after setting the registry key.
The new architecutre source tree is too deep, and the object file naming conventions in NDK makes the matters worse, by producing incredibly long file paths.
Other solutions such as symlinking source code etc. didn't work as expected, and makes the build scripts complicated and hard to manage.
This change temporarily works around the issue by placing the temporary build outputs as short a path as possible within the project path.

Summary

Changelog

[CATEGORY] [TYPE] - Message

Test Plan

pull-bot · 2022-04-06T23:57:02Z

	Fails
🚫	node` failed.

	Messages
📖	📋 Verify Changelog Format - A changelog entry has the following format: `[CATEGORY] [TYPE] - Message`. CATEGORY may be: General iOS Android JavaScript Internal (for changes that do not need to be called out in the release notes) TYPE may be: Added, for new features. Changed, for changes in existing functionality. Deprecated, for soon-to-be removed features. Removed, for now removed features. Fixed, for any bug fixes. Security, in case of vulnerabilities. MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Log

Error:  RequestError [HttpError]: Must have admin rights to Repository.
    at /root/react-native/bots/node_modules/@octokit/request/dist-node/index.js:86:21
    at processTicksAndRejections (internal/process/task_queues.js:95:5) {
  status: 403,
  response: {
    url: 'https://api.github.com/repos/facebook/react-native/issues/33582/labels',
    status: 403,
    headers: {
      'access-control-allow-origin': '*',
      'access-control-expose-headers': 'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset',
      connection: 'close',
      'content-encoding': 'gzip',
      'content-security-policy': "default-src 'none'",
      'content-type': 'application/json; charset=utf-8',
      date: 'Tue, 12 Apr 2022 11:50:03 GMT',
      'referrer-policy': 'origin-when-cross-origin, strict-origin-when-cross-origin',
      server: 'GitHub.com',
      'strict-transport-security': 'max-age=31536000; includeSubdomains; preload',
      'transfer-encoding': 'chunked',
      vary: 'Accept-Encoding, Accept, X-Requested-With',
      'x-accepted-oauth-scopes': '',
      'x-content-type-options': 'nosniff',
      'x-frame-options': 'deny',
      'x-github-media-type': 'github.v3; format=json',
      'x-github-request-id': 'CE54:7611:3EA982:907346:6255676B',
      'x-oauth-scopes': 'public_repo',
      'x-ratelimit-limit': '5000',
      'x-ratelimit-remaining': '4964',
      'x-ratelimit-reset': '1649766854',
      'x-ratelimit-resource': 'core',
      'x-ratelimit-used': '36',
      'x-xss-protection': '0'
    },
    data: {
      message: 'Must have admin rights to Repository.',
      documentation_url: 'https://docs.github.com/rest/reference/issues#add-labels-to-an-issue'
    }
  },
  request: {
    method: 'POST',
    url: 'https://api.github.com/repos/facebook/react-native/issues/33582/labels',
    headers: {
      accept: 'application/vnd.github.v3+json',
      'user-agent': 'octokit.js/16.43.2 Node.js/14.18.1 (Linux 5.13; x64)',
      authorization: 'token [REDACTED]',
      'content-type': 'application/json; charset=utf-8'
    },
    body: '{"labels":["Pick Request"]}',
    request: { hook: [Function: bound bound register] }
  }
}
danger-results://tmp/danger-results.json

Generated by 🚫 dangerJS against 5a8033d

cortinico

Great stuff! Thanks for looking into this @mganandraj and thanks for leaving the great explanation. Left a couple of comments that needs to be addressed before we can land this

cortinico · 2022-04-07T00:03:28Z

ReactAndroid/build.gradle

+                    project.hasProperty("newArchEnabled") && 
+                    project.newArchEnabled == "true") {


Those two are probably unnecessary. You will be including ReactAndroid/build.gradle only when using the new arch.

I think we need to keep those check for those who directly build the react-native repo to launch rntester, but don't want to enable new arch..

I think we need to keep those check for those who directly build the react-native repo to launch rntester, but don't want to enable new arch..

RN-Tester has New Arch enabled by default and can't be turned off there. It consumes RN from source so you'll end up failing the build there regardeless (or maybe the paths are short enough as there is no node_modules/react-native/ReactAndroid extra involved in the path).

ReactAndroid/build.gradle

analysis-bot · 2022-04-07T00:20:53Z

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

Base commit: 7dceb9b
Branch: main

analysis-bot · 2022-04-07T00:26:21Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,125,041	+354,335
android	hermes	armeabi-v7a	7,725,933	+549,266
android	hermes	x86	8,495,213	+415,267
android	hermes	x86_64	8,447,341	+388,853
android	jsc	arm64-v8a	9,790,667	+145,153
android	jsc	armeabi-v7a	8,776,411	+356,556
android	jsc	x86	9,757,454	+162,651
android	jsc	x86_64	10,353,618	+161,497

Base commit: 7dceb9b
Branch: main

cortinico · 2022-04-07T18:41:18Z

Could you also point this PR to main? It's easier to merge it there and cherry-pick it into the release branch.

cortinico · 2022-04-08T18:43:53Z

Could you also point this PR to main? It's easier to merge it there and cherry-pick it into the release branch.

Sorry, I've just realized this can't target main as we're on CMake there. I believe we should be fine with it so this patch should not even need to be backported to main (or maybe only the template/android changes).

mganandraj · 2022-04-10T13:38:02Z

Could you also point this PR to main? It's easier to merge it there and cherry-pick it into the release branch.

Sorry, I've just realized this can't target main as we're on CMake there. I believe we should be fine with it so this patch should not even need to be backported to main (or maybe only the template/android changes).

Makese sense .. Ill limit the PR to 'template/android' changes ..

cortinico · 2022-04-11T14:40:12Z

I've submitted also #33611 which could superseed this one. I'm looking for someone to test it though. @mganandraj would you be able to give it a try?

cortinico · 2022-04-12T11:32:33Z

Hey @mganandraj
As this is quite critical for 0.68.1, I've managed to find a win machine and test your solution. It correctly works (I had to move my project to C:\ though).

I've force pushed including my suggestions. Let me know if that doesn't work for you

We should be good to go merging this then 👍

On Windows there are limits on number of character in file paths and in command lines Ref: https://android.googlesource.com/platform/ndk/+/master/docs/BuildSystemMaintainers.md#Path-Length-Limits NDK allows circumventing command line limits using response(RSP) files as inputs using NDK_APP_SHORT_COMMANDS flag. Windows can support long file paths if configured through registry or by prefixing all file paths with a special character sequence The latter requires changes in NDK. And there are tools in NDK (AR) which is not able to handle long paths (>256) even after setting the registry key. The new architecutre source tree is too deep, and the object file naming conventions in NDK makes the matters worse, by producing incredibly long file paths. Other solutions such as symlinking source code etc. didn't work as expected, and makes the build scripts complicated and hard to manage. This change temporarily works around the issue by placing the temporary build outputs as short a path as possible within the project path. Changelog: [Android] [Fixed] - Fix for building new architecture sources on Windows

mikehardy · 2022-04-12T13:21:23Z

Here's my full trace testing v3 of this patch from a couple hours ago (last push I think)
#33611 (comment)

TL;DR: it worked to a point but still may have some issues? (that is, it worked for project rooted in C:\RN068 but failed with project rooted in C:\Users\micro\work\react-random\RN068

mganandraj · 2022-04-12T14:25:56Z

Hey @mganandraj
As this is quite critical for 0.68.1, I've managed to find a win machine and test your solution. It correctly works (I had to move my project to C:\ though).

I've force pushed including my suggestions. Let me know if that doesn't work for you

We should be good to go merging this then 👍

Great.. I'll merge the change asap ..

mganandraj · 2022-04-12T14:29:31Z

Here's my full trace testing v3 of this patch from a couple hours ago (last push I think)
#33611 (comment)

TL;DR: it worked to a point but still may have some issues? (that is, it worked for project rooted in C:\RN068 but failed with project rooted in C:\Users\micro\work\react-random\RN068

Yeah, I'm looking for a more general solution.. ndk r23 improves the situation by avoiding ar, ld etc. but still has some issues which I'm looking into..

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 labels Apr 6, 2022

cortinico reviewed Apr 7, 2022

View reviewed changes

cortinico added the Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules) label Apr 11, 2022

cortinico force-pushed the NewArchWinBuild branch from d8af84e to 5d61e63 Compare April 12, 2022 11:30

cortinico added Needs: Attention Issues where the author has responded to feedback. patch-release labels Apr 12, 2022

cortinico marked this pull request as ready for review April 12, 2022 11:30

cortinico force-pushed the NewArchWinBuild branch from 5d61e63 to b899bbc Compare April 12, 2022 11:40

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Apr 12, 2022

cortinico force-pushed the NewArchWinBuild branch from b899bbc to 5a8033d Compare April 12, 2022 11:46

cortinico mentioned this pull request Apr 12, 2022

Use NDK 23 only for Windows users. #33611

Closed

fortmarek merged commit 44ee801 into facebook:0.68-stable Apr 13, 2022

cortinico mentioned this pull request Apr 13, 2022

Building new architecture sources on Windows (NDK Bump) #33627

Closed

snyk-bot mentioned this pull request May 5, 2022

[Snyk] Upgrade react-native from 0.63.3 to 0.68.1 NOUIY/react-native-macos#99

Open

snyk-bot mentioned this pull request May 30, 2022

[Snyk] Upgrade react-native from 0.63.3 to 0.68.2 NOUIY/react-native-macos#220

Open

Skullcan mentioned this pull request Jul 15, 2022

[Windows] Task :ReactAndroid:hermes-engine:buildCMakeRelease[arm64-v8a][libhermes] FAILED #34084

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Building new architecture sources on Windows #33582

Building new architecture sources on Windows #33582

mganandraj commented Apr 6, 2022

pull-bot commented Apr 6, 2022 •

edited

Loading

cortinico left a comment

cortinico Apr 7, 2022

mganandraj Apr 7, 2022

cortinico Apr 7, 2022 •

edited

Loading

analysis-bot commented Apr 7, 2022 •

edited

Loading

analysis-bot commented Apr 7, 2022 •

edited

Loading

cortinico commented Apr 7, 2022

cortinico commented Apr 8, 2022

mganandraj commented Apr 10, 2022

cortinico commented Apr 11, 2022

cortinico commented Apr 12, 2022 •

edited

Loading

mikehardy commented Apr 12, 2022

mganandraj commented Apr 12, 2022

mganandraj commented Apr 12, 2022

		project.hasProperty("newArchEnabled") &&
		project.newArchEnabled == "true") {

Building new architecture sources on Windows #33582

Building new architecture sources on Windows #33582

Conversation

mganandraj commented Apr 6, 2022

Summary

Changelog

Test Plan

pull-bot commented Apr 6, 2022 • edited Loading

Log

cortinico left a comment

Choose a reason for hiding this comment

cortinico Apr 7, 2022

Choose a reason for hiding this comment

mganandraj Apr 7, 2022

Choose a reason for hiding this comment

cortinico Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

analysis-bot commented Apr 7, 2022 • edited Loading

analysis-bot commented Apr 7, 2022 • edited Loading

cortinico commented Apr 7, 2022

cortinico commented Apr 8, 2022

mganandraj commented Apr 10, 2022

cortinico commented Apr 11, 2022

cortinico commented Apr 12, 2022 • edited Loading

mikehardy commented Apr 12, 2022

mganandraj commented Apr 12, 2022

mganandraj commented Apr 12, 2022

pull-bot commented Apr 6, 2022 •

edited

Loading

cortinico Apr 7, 2022 •

edited

Loading

analysis-bot commented Apr 7, 2022 •

edited

Loading

analysis-bot commented Apr 7, 2022 •

edited

Loading

cortinico commented Apr 12, 2022 •

edited

Loading