Invoke node directly in generate-specs.sh

[ivanmoskalev]

Summary

Currently, Codegen bash wrapper (generate-specs.sh) for Xcode invokes JS-based Codegen tooling via yarn --silent node <...>. This breaks both:

• when Yarn is not installed (if NPM is used), for obvious reasons
• when Yarn v2 ("Berry") is active

This PR changes the way generate-specs.sh locates node executable to the following algorithm:

• use the path provided in the NODE_BINARY env var
• if NODE_BINARY env var is not defined, find node with command -v node

Changelog

[iOS] [Fixed] - Fix Codegen silently failing when Yarn is not installed, or when Yarn v2 is active.

Test Plan

Case 1 (no Yarn installed)

1. Ensure yarn is not present in PATH
2. Run Xcode build
3. Check that Codegen artifacts are produced

Case 2 (Yarn v2 is used)

1. Ensure yarn is running in the v2 ("Berry") mode
2. Run Xcode build
3. Check that Codegen artifacts are produced

[facebook-github-bot]

Hi @ivanmoskalev!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[analysis-bot]

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

Base commit: 505f9fc

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,898,590	-812
android	hermes	armeabi-v7a	8,402,662	-707
android	hermes	x86	9,385,003	-3,074
android	hermes	x86_64	9,330,444	-1,665
android	jsc	arm64-v8a	10,350,443	-235
android	jsc	armeabi-v7a	9,837,614	-128
android	jsc	x86	10,398,363	-2,512
android	jsc	x86_64	10,984,069	-1,067

Base commit: 505f9fc

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[janicduplessis]

LGTM

cc @hramos

[ivanmoskalev]

@janicduplessis I actually would want to have some advice from the code owners on this before merging. Currently, we still need to work around that line, since it breaks Codegen for Yarn-less machines. I can either work in some fallback for that case, OR, instead, get rid of yarn invocations altogether, if the callsite is now intended to be unreachable.

[janicduplessis]

@ivanmoskalev The link doesn't work, but I assume you are talking about the last YARN_BINARY callsite. This one should only happen when building RN from source. Still a good idea to fix it I think. One possibility is just call this script instead https://github.com/facebook/react-native/blob/bec5e147cb0a84225f88b3dbc2f87c39c0ef0bc7/packages/react-native-codegen/scripts/oss/build.sh. It already handles npm and yarn.

[janicduplessis]

We're also in the process of landing #30792 which will make changes to this code. I think after this you will want to check for whether the NODE_BINARY exists and make sure to add || true after the new command -v.

[ivanmoskalev]

Adding fixes in a couple hours, was on a sick leave for ~ last week.

[ivanmoskalev]

@janicduplessis @hramos I have updated the script, would you have a look, please?

I've also got 2 questions:

• Do CI tests cover the usecase in which $CODEGEN_PATH/scripts/oss/build.sh will be called? I'm asking because I don't know how to properly test it on my machine.
• I expect merge conflicts with Make codegen more reliable on iOS #30792 – do I need to wait for it to land and rebase my fork myself, or will conflict resolution be done by the FB team when they import this PR into their monorepo?

[facebook-github-bot]

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

[hramos]

I'll take care of the conflict.

[facebook-github-bot]

@hramos merged this pull request in 07e4953.