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 build_codegen! not finding @react-native/codegen in pnpm setups #41399

Closed
wants to merge 4 commits into from

Conversation

tido64
Copy link
Collaborator

@tido64 tido64 commented Nov 9, 2023

Summary:

build_codegen! currently assumes that @react-native/codegen gets installed next to react-native. In a pnpm setup, it's found under /~/react-native/node_modules/@react-native/codegen instead.

However, as @dmytrorykun pointed out, we don't actually need to build it outside of this repository.

Changelog:

[GENERAL] [FIXED] - @react-native/codegen shouldn't be built unless it's in the repo — fixes pod install failures in pnpm setups

Test Plan:

We have a patched version of react-native working in a pnpm setup here: microsoft/rnx-kit#2811

@tido64 tido64 requested a review from cipolleschi November 9, 2023 19:33
@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 Nov 9, 2023
@tido64 tido64 force-pushed the tido/codegen/pnpm-path branch from 5c14018 to 51a1171 Compare November 9, 2023 19:36
@cipolleschi
Copy link
Contributor

The changes are fine to me, but I'm a bit worried about its scalability in other setups. Also, we already have kind-of a node resolution in ruby here.
WDYT about reusing this approach and cleanup those three paths to a single one?

@tido64
Copy link
Collaborator Author

tido64 commented Nov 10, 2023

The changes are fine to me, but I'm a bit worried about its scalability in other setups. Also, we already have kind-of a node resolution in ruby here. WDYT about reusing this approach and cleanup those three paths to a single one?

I can reuse that approach for now. I'm really worried about adding node invocations as they're not exactly cheap. It may be okay with one or two, but they can quickly add up if we add more. In react-native-test-app, we have Node module resolution implemented in pure Ruby (see resolve_module). If this sounds interesting to you, maybe I can upstream it and replace the node calls in a separate PR?

@cipolleschi
Copy link
Contributor

Wow, they are just 3 or 4 functions of 3/4 lines each!
I think it would be great to have them in Core, tbh!

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@dmytrorykun dmytrorykun self-requested a review November 10, 2023 15:03
Copy link
Contributor

@dmytrorykun dmytrorykun left a comment

Choose a reason for hiding this comment

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

@tido64 Could you please clarify the use case?
If I understand the purpose of this function correctly, it is only supposed to be used when we are working from within react-native monorepo, and consuming codegen package form source, form packages/react-native-codegen.
And in that case it is sufficient to run the build only inside packages/react-native-codegen, and then it will be symlinked to any location the package manager decides.
So I'd suggest to try to delete all the code, and only leave the part that runs the build inside packages/react-native-codegen, and it should just work.

@tido64
Copy link
Collaborator Author

tido64 commented Nov 10, 2023

@tido64 Could you please clarify the use case?

We have a repo with pnpm setup. If I run pod install in this repo, it fails. I'm just trying to fix the failure. We don't use this script directly.

@dmytrorykun
Copy link
Contributor

@tido64 I assume in that case codegen is downloaded from NPM, and we shouldn't be trying to build it. I think the proper fix for this issue should be not trying to build it anywhere but in packages/react-native-codegen. Something like:

def build_codegen!(react_native_path, relative_installation_root, dir_manager: Dir)
  codegen_repo_path = "#{basePath(react_native_path, relative_installation_root)}/../react-native-codegen";
  if dir_manager.exist?(codegen_repo_path) && !dir_manager.exist?("#{codegen_repo_path}/lib")
    Pod::UI.puts "[Codegen] building #{codegen_repo_path}."
    system("#{codegen_repo_path}/scripts/oss/build.sh")
  end
end

Copy link
Contributor

@dmytrorykun dmytrorykun left a comment

Choose a reason for hiding this comment

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

Amazing, thank you!

@tido64
Copy link
Collaborator Author

tido64 commented Nov 13, 2023

Amazing, thank you!

Thanks for pointing out the fact that we don't need to build it. I thought it was a bit weird, but didn't give it a second thought.

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link

This pull request was successfully merged by @tido64 in 3dd6a83.

When will my fix make it into a release? | Upcoming Releases

@github-actions github-actions bot added the Merged This PR has been merged. label Nov 13, 2023
@tido64 tido64 deleted the tido/codegen/pnpm-path branch November 13, 2023 17:25
tido64 added a commit that referenced this pull request Nov 13, 2023
…ps (#41399)

Summary:
`build_codegen!` currently assumes that `react-native/codegen` gets installed next to `react-native`. In a pnpm setup, it's found under `/~/react-native/node_modules/react-native/codegen` instead.

However, as dmytrorykun pointed out, we don't actually need to build it outside of this repository.

## Changelog:

[GENERAL] [FIXED] - `react-native/codegen` shouldn't be built unless it's in the repo — fixes `pod install` failures in pnpm setups

Pull Request resolved: #41399

Test Plan: We have a patched version of `react-native` working in a pnpm setup here: microsoft/rnx-kit#2811

Reviewed By: dmytrorykun

Differential Revision: D51201643

Pulled By: cipolleschi

fbshipit-source-id: 53767ae08686a20f03b3b93abcbc7d5383083872
huntie pushed a commit that referenced this pull request Nov 27, 2023
…ps (#41399)

Summary:
`build_codegen!` currently assumes that `react-native/codegen` gets installed next to `react-native`. In a pnpm setup, it's found under `/~/react-native/node_modules/react-native/codegen` instead.

However, as dmytrorykun pointed out, we don't actually need to build it outside of this repository.

## Changelog:

[GENERAL] [FIXED] - `react-native/codegen` shouldn't be built unless it's in the repo — fixes `pod install` failures in pnpm setups

Pull Request resolved: #41399

Test Plan: We have a patched version of `react-native` working in a pnpm setup here: microsoft/rnx-kit#2811

Reviewed By: dmytrorykun

Differential Revision: D51201643

Pulled By: cipolleschi

fbshipit-source-id: 53767ae08686a20f03b3b93abcbc7d5383083872
Othinn pushed a commit to Othinn/react-native that referenced this pull request Jan 9, 2024
…ps (facebook#41399)

Summary:
`build_codegen!` currently assumes that `react-native/codegen` gets installed next to `react-native`. In a pnpm setup, it's found under `/~/react-native/node_modules/react-native/codegen` instead.

However, as dmytrorykun pointed out, we don't actually need to build it outside of this repository.

## Changelog:

[GENERAL] [FIXED] - `react-native/codegen` shouldn't be built unless it's in the repo — fixes `pod install` failures in pnpm setups

Pull Request resolved: facebook#41399

Test Plan: We have a patched version of `react-native` working in a pnpm setup here: microsoft/rnx-kit#2811

Reviewed By: dmytrorykun

Differential Revision: D51201643

Pulled By: cipolleschi

fbshipit-source-id: 53767ae08686a20f03b3b93abcbc7d5383083872
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.

4 participants