-
Notifications
You must be signed in to change notification settings - Fork 24.9k
[iOS] Fix outputs when iOS artifacts generator is run from Xcode script phase #54066
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
Conversation
87f82c4 to
79a1ca4
Compare
79a1ca4 to
d08506e
Compare
…pt_phases.sh Avoid ending up in in derive data output folder to be able to discover `autolinking.json` generated in Cocoapods step in all cases, regardless of if the script checks `RCT_SCRIPT_OUTPUT_DIR` itself.
d08506e to
b464256
Compare
An earlier change (0.79 and onwards, I believe?) runs the iOS artifacts code generator script in Xcode as well as from Cocoapods. This duplication runs it twice, but the second step isn't able to load the new autolinking.json correctly; See: #53503 This has been fixed upstream here: facebook/react-native#54066 Untill this one is merged, released and available to Expo, we need a workaround. Added build phase script for a workaround to autolinking-generated react-native-config output not being used in ReactCodegen script phase due to temp output directory.
|
The change is much earlier. We always run codegen in xcode. This allows users to write native modules and native components locally in their app, without having to reinstall cocopods everytime they change the specs of those custom modules/components. |
cipolleschi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the changes make sense to me and I'm positive in supporting them. I don't like the approach of looping over some outputPatchCandidates... Cn we find a way not to have that loop?
packages/react-native/scripts/react_native_pods_utils/script_phases.sh
Outdated
Show resolved
Hide resolved
| moveOutputs | ||
| describe "Generating codegen artifacts" | ||
| pushd "$RCT_SCRIPT_RN_DIR" >/dev/null || exit 1 | ||
| "$NODE_BINARY" "scripts/generate-codegen-artifacts.js" --path "$RCT_SCRIPT_APP_PATH" --outputPath "$RCT_SCRIPT_OUTPUT_DIR" --targetPlatform "ios" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value of RCT_SCRIPT_OUTPUT_DIR is always RCT_SCRIPT_POD_INSTALLATION_ROOT, which is set to be
pushd "$PODS_ROOT/../" > /dev/null
RCT_SCRIPT_POD_INSTALLATION_ROOT=$(pwd)
So it will always be Pods/.., which is where we want codegen to be generated.
Just adding the comment for the future.
packages/react-native/scripts/codegen/generate-artifacts-executor/utils.js
Show resolved
Hide resolved
90bc77f to
bb178a4
Compare
bb178a4 to
d5955f2
Compare
An earlier change (0.79 and onwards, I believe?) runs the iOS artifacts code generator script in Xcode as well as from Cocoapods. This duplication runs it twice, but the second step isn't able to load the new autolinking.json correctly; See: #53503 This has been fixed upstream here: facebook/react-native#54066 Untill this one is merged, released and available to Expo, we need a workaround. Added build phase script for a workaround to autolinking-generated react-native-config output not being used in ReactCodegen script phase due to temp output directory.
|
@cipolleschi, just out of curiosity, why is the (I know you are busy with React Conf, this can wait until after) |
# Why An earlier change (0.79 and onwards, I believe?) runs the iOS artifacts code generator script in Xcode as well as from Cocoapods. This duplication runs it twice, but the second step isn't able to load the new autolinking.json correctly; See: #53503 This has been fixed upstream here: facebook/react-native#54066 Untill this one is merged, released and available to Expo, we need a workaround. # How Added build phase script for a workaround to autolinking-generated react-native-config output not being used in ReactCodegen script phase due to temp output directory. @byCedric participated in the script, and it has been tested with a new Expo project using pnpm. # Checklist - [x] I added a `changelog.md` entry and rebuilt the package sources according to [this short guide](https://github.com/expo/expo/blob/main/CONTRIBUTING.md#-before-submitting) - [x] This diff will work correctly for `npx expo prebuild` & EAS Build (eg: updated a module plugin). --------- Co-authored-by: Cedric van Putten <me@bycedric.com>
An earlier change (0.79 and onwards, I believe?) runs the iOS artifacts code generator script in Xcode as well as from Cocoapods. This duplication runs it twice, but the second step isn't able to load the new autolinking.json correctly; See: #53503 This has been fixed upstream here: facebook/react-native#54066 Untill this one is merged, released and available to Expo, we need a workaround. Added build phase script for a workaround to autolinking-generated react-native-config output not being used in ReactCodegen script phase due to temp output directory. @byCedric participated in the script, and it has been tested with a new Expo project using pnpm. - [x] I added a `changelog.md` entry and rebuilt the package sources according to [this short guide](https://github.com/expo/expo/blob/main/CONTRIBUTING.md#-before-submitting) - [x] This diff will work correctly for `npx expo prebuild` & EAS Build (eg: updated a module plugin). --------- Co-authored-by: Cedric van Putten <me@bycedric.com>
cipolleschi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the PR.
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this in D85673625. |
|
@cipolleschi merged this pull request in c029032. |
|
This pull request was successfully merged by @kitten in c029032 When will my fix make it into a release? | How to file a pick request? |
Summary:
An earlier change (0.79 and onwards, I believe?) runs the iOS artifacts code generator script in Xcode as well as from Cocoapods. This duplication runs it twice, but the second step isn't able to load the new
autolinking.jsoncorrectly; See: #53503This PR "double" fixes this by:
script_phases.sh) where it's called by Xcode, rather than a temporary directory$RCT_SCRIPT_OUTPUT_DIRif it's set as an environment variable in the artifacts generator (which it is byscript_phases.sh)While this is technically redundant, future changes here make this feel like a safer option, since both conventions overlap in these two places, and the double fix may prevent a regression here in the shortterm and convey what this path is supposed to be in both places.
Changelog:
[IOS] [FIXED] - Fix autolinking-generated react-native-config output not being used in ReactCodegen script phase due to temp output directory
Test Plan:
$RCT_SCRIPT_OUTPUT_DIRenv var for findingbuild/generated/autolinking/autolinking.json$RCT_SCRIPT_OUTPUT_DIRas output inwithCodegenDiscoveryinreact_native_pods_utils/script_phases.sh(which is called by Xcode rather than Cocoapods to invoke the artifacts generator) since the temporary output directory isn't necessary