-
Notifications
You must be signed in to change notification settings - Fork 24.9k
[iOS] Use autolinking react-native-config output in iOS artifacts generator #53503
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
5c06b10 to
045db60
Compare
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.
code makes sense, but there is one important part that needs to be addressed.
packages/react-native/scripts/codegen/generate-artifacts-executor/utils.js
Outdated
Show resolved
Hide resolved
|
I'm having some trouble building on 0.81-stable...kitten:react-native:@kitten/0.81-stable/fix/pick-broken-ios-autolinking I'll have to look at the lint failures but the |
packages/react-native/scripts/codegen/generate-artifacts-executor/utils.js
Outdated
Show resolved
Hide resolved
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.
Talked at the CCS with @byCedric. I think this is good to go, as soon as the linting and JS tests are fixed.
28de9a8 to
c8dfe0d
Compare
c8dfe0d to
a9ab883
Compare
|
Should be back to normal and passing. Rebased on |
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this in D81490755. |
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.
Review automatically exported from Phabricator review in Meta.
|
This pull request was successfully merged by @kitten in f170db4 When will my fix make it into a release? | How to file a pick request? |
|
@cipolleschi merged this pull request in f170db4. |
…#53503) Summary: Resolves #53501 This is a pretty major oversight of (presumably) the old autolinking refactor. The iOS autolinking's second stage, invoked in `use_react_native!` does not accept the `react-native-config` sub-command's `react-native-config` output. This is only invoked and used in the prior step, `use_native_modules`. The second step instead invokes old code that does something _similar_ to the new autolinking in `scripts/generate-artifacts-executor`, and happens to align in most cases. (But it does "autolinking" from scratch). tl;dr: When the results don't match up, things go wrong. Instead, we now write the autolinking (react native config) results to a file, then read the output back in the second step. This doesn't affect Android/Gradle, which are implemented correctly. ## Changelog: [IOS] [FIXED] - Use autolinking-generated react-native-config output in second step of cocoapods linking that generates artifacts and generated source Pull Request resolved: #53503 Test Plan: - See #53501 for failing repro - Clone for working repro: https://github.com/byCedric/react-native-codegen-ios-autolinking/tree/fix-54503 - Note: Contains this PR's changes as a patch - `bun install` - `bun expo run:ios` Reviewed By: cortinico Differential Revision: D81490755 Pulled By: cipolleschi fbshipit-source-id: eefe786a116404f4ed24bd7125dfb108a811f71e
…#53503) Summary: Resolves #53501 This is a pretty major oversight of (presumably) the old autolinking refactor. The iOS autolinking's second stage, invoked in `use_react_native!` does not accept the `react-native-config` sub-command's `react-native-config` output. This is only invoked and used in the prior step, `use_native_modules`. The second step instead invokes old code that does something _similar_ to the new autolinking in `scripts/generate-artifacts-executor`, and happens to align in most cases. (But it does "autolinking" from scratch). tl;dr: When the results don't match up, things go wrong. Instead, we now write the autolinking (react native config) results to a file, then read the output back in the second step. This doesn't affect Android/Gradle, which are implemented correctly. [IOS] [FIXED] - Use autolinking-generated react-native-config output in second step of cocoapods linking that generates artifacts and generated source Pull Request resolved: #53503 Test Plan: - See #53501 for failing repro - Clone for working repro: https://github.com/byCedric/react-native-codegen-ios-autolinking/tree/fix-54503 - Note: Contains this PR's changes as a patch - `bun install` - `bun expo run:ios` Reviewed By: cortinico Differential Revision: D81490755 Pulled By: cipolleschi fbshipit-source-id: eefe786a116404f4ed24bd7125dfb108a811f71e
|
This pull request was successfully merged by @kitten in a2eb29e When will my fix make it into a release? | How to file a pick request? |
…#53503) Summary: Resolves #53501 This is a pretty major oversight of (presumably) the old autolinking refactor. The iOS autolinking's second stage, invoked in `use_react_native!` does not accept the `react-native-config` sub-command's `react-native-config` output. This is only invoked and used in the prior step, `use_native_modules`. The second step instead invokes old code that does something _similar_ to the new autolinking in `scripts/generate-artifacts-executor`, and happens to align in most cases. (But it does "autolinking" from scratch). tl;dr: When the results don't match up, things go wrong. Instead, we now write the autolinking (react native config) results to a file, then read the output back in the second step. This doesn't affect Android/Gradle, which are implemented correctly. [IOS] [FIXED] - Use autolinking-generated react-native-config output in second step of cocoapods linking that generates artifacts and generated source Pull Request resolved: #53503 Test Plan: - See #53501 for failing repro - Clone for working repro: https://github.com/byCedric/react-native-codegen-ios-autolinking/tree/fix-54503 - Note: Contains this PR's changes as a patch - `bun install` - `bun expo run:ios` Reviewed By: cortinico Differential Revision: D81490755 Pulled By: cipolleschi fbshipit-source-id: eefe786a116404f4ed24bd7125dfb108a811f71e
|
This pull request was successfully merged by @kitten in bb73315 When will my fix make it into a release? | How to file a pick request? |
Summary: Follow-up to #53503 for a regression When no React Native module is present this bail condition stops us from generating the artifacts podspec that's needed to complete build. ## Changelog: [IOS] [FIXED] - Fix regression that skips artifacts code generation Pull Request resolved: #53690 Test Plan: - Create an app **without** any React Native modules, run `pod install`; without this fix the podspec will be missing and the build will fail - With expo this can be reproduced using `create-expo-app --template blank-typescript@next` on `[email protected]` - With the community CLI this can be reproduced using `npx react-native-community/cli@latest init test --skip-install --version 0.81.2` and uninstalling `react-native-safe-area-context` Reviewed By: javache Differential Revision: D82103491 Pulled By: cipolleschi fbshipit-source-id: 3d9619b5a935ca920220824b3963a9a107f926ca
Summary: Follow-up to #53503 for a regression When no React Native module is present this bail condition stops us from generating the artifacts podspec that's needed to complete build. ## Changelog: [IOS] [FIXED] - Fix regression that skips artifacts code generation Pull Request resolved: #53690 Test Plan: - Create an app **without** any React Native modules, run `pod install`; without this fix the podspec will be missing and the build will fail - With expo this can be reproduced using `create-expo-app --template blank-typescript@next` on `[email protected]` - With the community CLI this can be reproduced using `npx react-native-community/cli@latest init test --skip-install --version 0.81.2` and uninstalling `react-native-safe-area-context` Reviewed By: javache Differential Revision: D82103491 Pulled By: cipolleschi fbshipit-source-id: 3d9619b5a935ca920220824b3963a9a107f926ca
Summary: Follow-up to #53503 for a regression When no React Native module is present this bail condition stops us from generating the artifacts podspec that's needed to complete build. ## Changelog: [IOS] [FIXED] - Fix regression that skips artifacts code generation Pull Request resolved: #53690 Test Plan: - Create an app **without** any React Native modules, run `pod install`; without this fix the podspec will be missing and the build will fail - With expo this can be reproduced using `create-expo-app --template blank-typescript@next` on `[email protected]` - With the community CLI this can be reproduced using `npx react-native-community/cli@latest init test --skip-install --version 0.81.2` and uninstalling `react-native-safe-area-context` Reviewed By: javache Differential Revision: D82103491 Pulled By: cipolleschi fbshipit-source-id: 3d9619b5a935ca920220824b3963a9a107f926ca
…se (#54066) 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.json` correctly; See: #53503 This PR "double" fixes this by: - simply passing the "real" output directory to the artifacts generator in (`script_phases.sh`) where it's called by Xcode, rather than a temporary directory - preferring `$RCT_SCRIPT_OUTPUT_DIR` if it's set as an environment variable in the artifacts generator (which it is by `script_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 Pull Request resolved: #54066 Test Plan: - Prefer `$RCT_SCRIPT_OUTPUT_DIR` env var for finding `build/generated/autolinking/autolinking.json` - Always use real `$RCT_SCRIPT_OUTPUT_DIR` as output in `withCodegenDiscovery` in `react_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 Reviewed By: javache Differential Revision: D85673625 Pulled By: cipolleschi fbshipit-source-id: 9d297fb0ee24f52a0bb7c5a8f41bf770bf63b18f
Summary:
Resolves #53501
This is a pretty major oversight of (presumably) the old autolinking refactor. The iOS autolinking's second stage, invoked in
use_react_native!does not accept thereact-native-configsub-command'sreact-native-configoutput. This is only invoked and used in the prior step,use_native_modules.The second step instead invokes old code that does something similar to the new autolinking in
scripts/generate-artifacts-executor, and happens to align in most cases. (But it does "autolinking" from scratch). tl;dr: When the results don't match up, things go wrong.Instead, we now write the autolinking (react native config) results to a file, then read the output back in the second step.
This doesn't affect Android/Gradle, which are implemented correctly.
Changelog:
[IOS] [FIXED] - Use autolinking-generated react-native-config output in second step of cocoapods linking that generates artifacts and generated source
Test Plan:
bun installbun expo run:ios