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

[Main][Windows] Working around Long paths limitation on Windows #33707

Merged
merged 5 commits into from
May 9, 2022

Conversation

mganandraj
Copy link
Contributor

This change is extending the changes made by alespergl to reduce the file paths and command lengths of ndk build commands
Essentially we are shortening the length of the source files by using relative paths instead of absolute paths as enumerated by the wildcard expression
This commit is extending the fix by including all the new modules introduced into RN for the new architecture, including the generated modules.
We are also reverting the ndk bump as ndk23 is crashing frequently when building RN with new arch. The reduced file paths lengths ensures the ndk bump is not required for relatively short application paths.

Summary

Fix building RN with new architecture on Windows boxes by using relative paths for C++ sources

Changelog

Fix building RN with new architecture on Windows boxes by using relative paths for C++ sources
-->

[CATEGORY] [TYPE] - Message

Test Plan

Verified building and running RNTester.

alespergl and others added 3 commits October 26, 2020 15:29
…file paths and command lengths of ndk build commands

Essentially we are shortening the length of the source files by using relative paths instead of absolute paths as enumerated by the wildcard expression
This commit is extending the fix by including all the new modules introduced into RN for the new architecture, including the generated modules.
@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 Apr 24, 2022
@pull-bot
Copy link

pull-bot commented Apr 24, 2022

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/33707/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: 'Thu, 05 May 2022 20:27:20 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': 'D12E:7937:684F0C:BFCC13:62743328',
      'x-oauth-scopes': 'public_repo',
      'x-ratelimit-limit': '5000',
      'x-ratelimit-remaining': '4992',
      'x-ratelimit-reset': '1651786039',
      'x-ratelimit-resource': 'core',
      'x-ratelimit-used': '8',
      '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/33707/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 6ae1f8e

@mganandraj
Copy link
Contributor Author

@cortinico @kelset This change is extending the PR by @alespergl to use relative paths in ndk builds ..

@analysis-bot
Copy link

analysis-bot commented Apr 24, 2022

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

Base commit: 870755f
Branch: main

@analysis-bot
Copy link

analysis-bot commented Apr 24, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,123,587 +344,737
android hermes armeabi-v7a 7,724,094 +540,172
android hermes x86 8,493,687 +405,934
android hermes x86_64 8,445,866 +377,402
android jsc arm64-v8a 9,789,461 +136,117
android jsc armeabi-v7a 8,774,809 +347,690
android jsc x86 9,756,165 +153,198
android jsc x86_64 10,352,382 +151,760

Base commit: 870755f
Branch: main

@hetanthakkar

This comment was marked as off-topic.

Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

I was able to test this and verify that it works correctly @mganandraj 👍 Thanks for sending it over.

Sorry it took so long but I had some problems with my Win machine.

I've left one comments that needs to be addressed here.

Moreover, we'll have to:

  • Backport this to main
  • Release a new version of react-native-codegen from the release branch that contains your fixes.

@kelset for the second point, let's sync on how we're going to do this.

Co-authored-by: Aleš Pergl <alespergl@users.noreply.github.com>
@cortinico cortinico changed the title Long paths [Main][Windows] Working around Long paths limitation on Windows May 9, 2022
@cortinico cortinico merged commit 62ef6f5 into facebook:0.68-stable May 9, 2022
facebook-github-bot pushed a commit that referenced this pull request May 9, 2022
Summary:
Cherry picking #33707 to main branch

This change is extending the changes made by alespergl to reduce the file paths and command lengths of ndk build commands
Essentially we are shortening the length of the source files by using relative paths instead of absolute paths as enumerated by the wildcard expression
This commit is extending the fix by including all the new modules introduced into RN for the new architecture, including the generated modules.
We are also reverting the ndk bump as ndk23 is crashing frequently when building RN with new arch. The reduced file paths lengths ensures the ndk bump is not required for relatively short application paths.

Fix building RN with new architecture on Windows boxes by using relative paths for C++ sources

## Changelog

Fix building RN with new architecture on Windows boxes by using relative paths for C++ sources

[CATEGORY] [TYPE] - Message

Pull Request resolved: #33784

Test Plan: Verified building on windows box

Reviewed By: javache

Differential Revision: D36241928

Pulled By: cortinico

fbshipit-source-id: 1ce428a271724cbd3b00a24fe03e7d69253f169b
@cortinico cortinico added the Merged This PR has been merged. label May 10, 2022
cortinico pushed a commit that referenced this pull request May 11, 2022
Co-authored-by: Aleš Pergl <alespergl@users.noreply.github.com>
Co-authored-by: Ales Pergl <alpergl@microsoft.com>
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.

7 participants