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 React-bridging header not found for third party modules #34214

Merged
merged 1 commit into from
Jul 21, 2022

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Jul 18, 2022

Summary

Some third party modules will have build errors on react-native 0.69.1 if they have dependency to React-bridging (or ReactCommon/turbomodule/core from transitive dependency). We setup correct header search paths in ReactCommon.podspec. We should also apply the change to third party modules. This PR is a workaround for the migration without introducing breaking changes to third party modules.

Note that is pr is based on 0.69-stable. Because on main branch, the react_native_pods.rb has much refactoring. I'll probably create a new pr based on main branch after this one landed.

Fix #34102

Changelog

[iOS] [Fixed] - Fix React-bridging headers import not found

Test Plan

$ npx react-native init TestApp --version 0.69
$ cd TestApp
$ yarn add react-native-vision-camera
$ yarn ios

@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. Contributor A React Native contributor. p: Expo Partner: Expo Partner labels Jul 18, 2022
@pull-bot
Copy link

pull-bot commented Jul 18, 2022

Fails
🚫

node failed.

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/34214/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: 'Wed, 20 Jul 2022 16:05:39 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': '8C7A:8B4A:7F4CA9:F1D430:62D827D3',
      'x-oauth-scopes': 'public_repo',
      'x-ratelimit-limit': '5000',
      'x-ratelimit-remaining': '4992',
      'x-ratelimit-reset': '1658336738',
      '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/34214/labels',
    headers: {
      accept: 'application/vnd.github.v3+json',
      'user-agent': 'octokit-rest.js/18.12.0 octokit-core.js/3.5.1 Node.js/14.19.0 (linux; 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 fa2acc3

@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Jul 18, 2022
@analysis-bot
Copy link

analysis-bot commented Jul 18, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,794,939 -25,545
android hermes armeabi-v7a 7,196,378 -17,994
android hermes x86 8,103,483 -30,721
android hermes x86_64 8,083,335 -28,974
android jsc arm64-v8a 9,667,992 -30,011
android jsc armeabi-v7a 8,438,449 -15,678
android jsc x86 9,616,895 -32,905
android jsc x86_64 10,214,308 -33,715

Base commit: 361d939
Branch: main

@Kudo Kudo marked this pull request as ready for review July 18, 2022 18:06
@facebook-github-bot 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 Jul 18, 2022
@cipolleschi
Copy link
Contributor

Hi @Kudo, thanks for the PR. I see that you created it against the 0.69-stable. Is this something required for 0.69 only because it works on 0.70 or do we have to implement it also in main?

@Kudo
Copy link
Contributor Author

Kudo commented Jul 19, 2022

@cipolleschi because the react_native_pods.rb is quite different on main because #33982, i am thinking to land this first on 0.69-stable. if the pr landed, i can create another pr for main branch. does this make sense to you?

@cipolleschi
Copy link
Contributor

Yes, it makes sense. However, there are failures in CI that shouldn't be there. The only failure that should be there is the test_ios_unit. Could you try rerun the jobs, please?

@Kudo
Copy link
Contributor Author

Kudo commented Jul 19, 2022

@cipolleschi i also wondered for the failures. however i don't have permissions to rerun the jobs. could you help to do that?

@cipolleschi
Copy link
Contributor

So... for analyze_pr it looks like we are actually missing some permissions. I asked @cortinico about that.

For the other task, the issue is that Hermes is deciding that it won't build from source.. I think that we need to take this case into consideration...

@Kudo
Copy link
Contributor Author

Kudo commented Jul 19, 2022

if the ci failures come from pr based on 0.69-stable, i can also create a new pr based on main branch first and leave this pr as reference when cherry-picking to 0.69-stable. let me know which one makes sense to you.

@cipolleschi
Copy link
Contributor

@Kudo PR #34221 should fix this problem as well.
As soon as we cherry pick that, we could rebase this one and the CI should be green.

@Kudo
Copy link
Contributor Author

Kudo commented Jul 19, 2022

sounds good! i'll rebase after that pr is cherry-picked. thanks!

@kelset
Copy link
Contributor

kelset commented Jul 19, 2022

@Kudo my PR landed and I've already cherry picked it in both 0.70 and 0.69 branches 👍 you can try rebasing your branch

@Kudo
Copy link
Contributor Author

Kudo commented Jul 19, 2022

rebased, thanks @kelset!

@Kudo
Copy link
Contributor Author

Kudo commented Jul 19, 2022

looks like the ci jobs failed at the same places again 🤔

@cipolleschi
Copy link
Contributor

Yep... Basically, in this setup we don't need to build hermes from source. The prepare_hermes_workspace does that check. But, it expect to have a couple of folders to copy... which, in that case, are not there.

I'm working on a fix here: #34223

@Kudo
Copy link
Contributor Author

Kudo commented Jul 19, 2022

thanks @cipolleschi ! i'll rebase after #34223 landed.

@cipolleschi
Copy link
Contributor

Hi @Kudo, I merged #34223 in 0.69-stable. If you can rebase, let's see if the CI goes green!

@Kudo
Copy link
Contributor Author

Kudo commented Jul 20, 2022

sure, thanks! let's see how it going.

@Kudo
Copy link
Contributor Author

Kudo commented Jul 20, 2022

@cipolleschi looks like there're some failure jobs for building hermes, is it expected?

@cipolleschi
Copy link
Contributor

No... I'm looking into it, right now.

@cipolleschi
Copy link
Contributor

@kelset @Kudo
Sorry, the previous fix was not the correct one. This #34228 will actually do. I'm testing it right now. It has to be ported also in main, though, but I'll do it in a separate PR becauseI want to see if it fixes our issues in 0.69

@cipolleschi
Copy link
Contributor

@Kudo, merged #34228. Could you rebase, so that we check that now it builds properly?

@Kudo
Copy link
Contributor Author

Kudo commented Jul 20, 2022

@cipolleschi all ci jobs finished. are they in good state or any further jobs you would like to pass?

@cipolleschi
Copy link
Contributor

And now the CI is green! we can merge this!

@Kudo
Copy link
Contributor Author

Kudo commented Jul 21, 2022

@cipolleschi cool, i don't have permission to merge. maybe you could help merging it. thanks!

@cipolleschi cipolleschi merged commit b9e9b53 into facebook:0.69-stable Jul 21, 2022
@Kudo Kudo deleted the fix-34102 branch July 21, 2022 14:08
Kudo added a commit to Kudo/react-native that referenced this pull request Jul 25, 2022
@Kudo Kudo mentioned this pull request Jul 25, 2022
facebook-github-bot pushed a commit that referenced this pull request Jul 27, 2022
Summary:
cherry-pick changes from #34214 to main. because the `react_native_pods.rb` on main is quite different from 0.69, i have separated pr for the change.

## Changelog

[iOS] [Fixed] - Fix React-bridging headers import not found

Pull Request resolved: #34271

Test Plan: RNTester + pod install and verify pod targets to have `React-bridging` in header search paths.

Reviewed By: cipolleschi

Differential Revision: D38122074

Pulled By: dmitryrykun

fbshipit-source-id: 64569abbfa3a684f0d6b84c9e3222bfc9a171061
kelset pushed a commit that referenced this pull request Jul 27, 2022
Summary:
cherry-pick changes from #34214 to main. because the `react_native_pods.rb` on main is quite different from 0.69, i have separated pr for the change.

## Changelog

[iOS] [Fixed] - Fix React-bridging headers import not found

Pull Request resolved: #34271

Test Plan: RNTester + pod install and verify pod targets to have `React-bridging` in header search paths.

Reviewed By: cipolleschi

Differential Revision: D38122074

Pulled By: dmitryrykun

fbshipit-source-id: 64569abbfa3a684f0d6b84c9e3222bfc9a171061
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
Summary:
cherry-pick changes from facebook#34214 to main. because the `react_native_pods.rb` on main is quite different from 0.69, i have separated pr for the change.

## Changelog

[iOS] [Fixed] - Fix React-bridging headers import not found

Pull Request resolved: facebook#34271

Test Plan: RNTester + pod install and verify pod targets to have `React-bridging` in header search paths.

Reviewed By: cipolleschi

Differential Revision: D38122074

Pulled By: dmitryrykun

fbshipit-source-id: 64569abbfa3a684f0d6b84c9e3222bfc9a171061
roryabraham pushed a commit to Expensify/react-native that referenced this pull request Aug 17, 2022
Summary:
cherry-pick changes from facebook#34214 to main. because the `react_native_pods.rb` on main is quite different from 0.69, i have separated pr for the change.

## Changelog

[iOS] [Fixed] - Fix React-bridging headers import not found

Pull Request resolved: facebook#34271

Test Plan: RNTester + pod install and verify pod targets to have `React-bridging` in header search paths.

Reviewed By: cipolleschi

Differential Revision: D38122074

Pulled By: dmitryrykun

fbshipit-source-id: 64569abbfa3a684f0d6b84c9e3222bfc9a171061
@evelant
Copy link

evelant commented Dec 12, 2022

@Kudo This patch works in some configurations, but doesn't work when using pnpm as a package manager. In the case of pnpm the bridging headers get copied to the wrong directory. They end up at ios/build/Build/node_modules/.pnpm/[email protected]_2hkxedd44to6uuf252s62q5boq/node_modules/react-native/ReactCommon/react/bridging.

Where in the build process does it determine where to copy headers and why might it be using a bad path? I'm guessing it doesn't like pnpm's symlinking, but I'm not sure where to look to work around it.

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. Contributor A React Native contributor. p: Expo Partner: Expo Partner Platform: iOS iOS applications. 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.

8 participants