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

Update CodeGen to leverage the outputDir as suggested in diff review #33729

Closed
wants to merge 4 commits into from

Conversation

cipolleschi
Copy link
Contributor

Summary:
This PR addresses this comment.

It makes the CodeGen to the outputDir as base directory for the codegen.

Finally, it updates the unit tests accordingly.

Changelog

[iOS][Changed] - use outputDir as base directory for the codegen and remove the possibility to customize the intermediate path. The generated code requires specific paths in the #include directive.

Reviewed By: cortinico, dmitryrykun

Differential Revision: D35935282

@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: Facebook Partner: Facebook Partner fb-exported labels Apr 28, 2022
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D35935282

@react-native-bot react-native-bot added the Platform: iOS iOS applications. label Apr 28, 2022
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D35935282

cipolleschi pushed a commit to cipolleschi/react-native that referenced this pull request Apr 28, 2022
facebook#33729)

Summary:
Pull Request resolved: facebook#33729

This PR addresses [this comment](https://www.internalfb.com/diff/D35820848?dst_version_fbid=496290878846487&transaction_fbid=355967423221044).

It makes the CodeGen to the `outputDir` as base directory for the codegen.

Finally, it updates the unit tests accordingly.

## Changelog
[iOS][Changed] - use `outputDir` as base directory for the codegen and remove the possibility to customize the intermediate path. The generated code requires specific paths in the `#include` directive.

Reviewed By: cortinico, dmitryrykun

Differential Revision: D35935282

fbshipit-source-id: 4c575798a9f2317c92056f567eaf372813f2b430
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D35935282

cipolleschi pushed a commit to cipolleschi/react-native that referenced this pull request Apr 28, 2022
facebook#33729)

Summary:
Pull Request resolved: facebook#33729

This PR addresses [this comment](https://www.internalfb.com/diff/D35820848?dst_version_fbid=496290878846487&transaction_fbid=355967423221044).

It makes the CodeGen to the `outputDir` as base directory for the codegen.

Finally, it updates the unit tests accordingly.

## Changelog
[iOS][Changed] - use `outputDir` as base directory for the codegen and remove the possibility to customize the intermediate path. The generated code requires specific paths in the `#include` directive.

Reviewed By: cortinico, dmitryrykun

Differential Revision: D35935282

fbshipit-source-id: 1f044852c27d2e14b031613592bc4b941f366218
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D35935282

cipolleschi pushed a commit to cipolleschi/react-native that referenced this pull request Apr 28, 2022
facebook#33729)

Summary:
Pull Request resolved: facebook#33729

This PR addresses [this comment](https://www.internalfb.com/diff/D35820848?dst_version_fbid=496290878846487&transaction_fbid=355967423221044).

It makes the CodeGen to the `outputDir` as base directory for the codegen.

Finally, it updates the unit tests accordingly.

## Changelog
[iOS][Changed] - use `outputDir` as base directory for the codegen and remove the possibility to customize the intermediate path. The generated code requires specific paths in the `#include` directive.

Reviewed By: cortinico, dmitryrykun

Differential Revision: D35935282

fbshipit-source-id: 65806090bf31384ce8d4b12d192041afb1b726b5
@analysis-bot
Copy link

analysis-bot commented Apr 28, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,775,895 +1
android hermes armeabi-v7a 7,181,346 +0
android hermes x86 8,084,894 -2
android hermes x86_64 8,065,069 -2
android jsc arm64-v8a 9,650,077 +0
android jsc armeabi-v7a 8,424,169 +0
android jsc x86 9,599,556 +0
android jsc x86_64 10,197,138 +0

Base commit: d992ae0
Branch: main

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D35935282

cipolleschi pushed a commit to cipolleschi/react-native that referenced this pull request Apr 28, 2022
facebook#33729)

Summary:
Pull Request resolved: facebook#33729

This PR addresses [this comment](https://www.internalfb.com/diff/D35820848?dst_version_fbid=496290878846487&transaction_fbid=355967423221044).

It makes the CodeGen to the `outputDir` as base directory for the codegen.

Finally, it updates the unit tests accordingly.

## Changelog
[iOS][Changed] - use `outputDir` as base directory for the codegen and remove the possibility to customize the intermediate path. The generated code requires specific paths in the `#include` directive.

Reviewed By: cortinico, dmitryrykun

Differential Revision: D35935282

fbshipit-source-id: 6fbf284b50948c4f137801bcb180103ed7536e3b
@pull-bot
Copy link

pull-bot commented Apr 28, 2022

Warnings
⚠️ 🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against 49dae72

cipolleschi pushed a commit to cipolleschi/react-native that referenced this pull request Apr 29, 2022
facebook#33729)

Summary:
Pull Request resolved: facebook#33729

This PR addresses [this comment](https://www.internalfb.com/diff/D35820848?dst_version_fbid=496290878846487&transaction_fbid=355967423221044).

It makes the CodeGen to the `outputDir` as base directory for the codegen.

Finally, it updates the unit tests accordingly.

## Changelog
[iOS][Changed] - use `outputDir` as base directory for the codegen and remove the possibility to customize the intermediate path. The generated code requires specific paths in the `#include` directive.

Reviewed By: cortinico, dmitryrykun

Differential Revision: D35935282

fbshipit-source-id: eb87f94db64eadc4400d9b29a92712b494135f19
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D35935282

cipolleschi pushed a commit to cipolleschi/react-native that referenced this pull request Apr 29, 2022
facebook#33729)

Summary:
Pull Request resolved: facebook#33729

This PR addresses [this comment](https://www.internalfb.com/diff/D35820848?dst_version_fbid=496290878846487&transaction_fbid=355967423221044).

It makes the CodeGen to the `outputDir` as base directory for the codegen.

Finally, it updates the unit tests accordingly.

## Changelog
[iOS][Changed] - use `outputDir` as base directory for the codegen and remove the possibility to customize the intermediate path. The generated code requires specific paths in the `#include` directive.

Reviewed By: cortinico, dmitryrykun

Differential Revision: D35935282

fbshipit-source-id: 4885ffa8c9e116929c198de3cb67fb92d30a0457
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D35935282

@analysis-bot
Copy link

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

Base commit: d992ae0
Branch: main

Riccardo Cipolleschi added 2 commits May 1, 2022 07:54
Summary:
This Diff introduces some changes in the CodeGen to properly generate the types in the right folder.

## Issue
The codegen on iOS defines the output folder once, before creating the generated code.
When the code we have to generate is just a TurboModule (TM) or a Fabric Component (FC), this mechanism works properly.

However, if a library has to generate both TM and FC, actually using the library type `all`, all the code is generated using the TurboModules' output folder.
(**Note:** Android only works in this way)

This generates invalid code because all the FC's `#import` directives assumes that the code is generated in the FC output path which, in this case, is not.

## Solution

The adopted solution moves the responsibility to decide where the files has to be generated to the CodeGen step instead of in the preparatory phases.

The two paths are precomputed in the `generate-artifacts.js` script (the entry point for the CodeGen) and they are passed to all the scripts that requires them.

Once they reach the `RNCodegen.js` file, the generators creates the files and save them in the proper paths.

## Changelog
[iOS][Changed] - CodeGen now supports the `"all"` library type.

Differential Revision: D35820848

fbshipit-source-id: e7abb0632caad16fa359e7b1b78ca7f7afd44b2a
Summary:
This Diff splits the `generate-artifacts.js` script into:
* `generate-artifacts-executor.js`: which contains the logic to generate the code for iOS.
* `generate-artifacts.js`: which contains the argument parsing logic and invokes the executor.

Finally, it introduces some tests.

## Changelog
[iOS][Changed] - Refactor part of the codegen scripts and add tests.

Differential Revision: D35846674

fbshipit-source-id: bd5d7ea46bfa32b345b8ef808d1ea499308089dc
Riccardo Cipolleschi added 2 commits May 1, 2022 07:54
Summary:
This Diff splits the `generate-specs-cli.js` script into:
* `generate-specs-cli-executor.js`: which contains the logic to generate the code for iOS.
* `generate-specs-cli.js`: which contains the argument parsing logic and invokes the executor.

Finally it introduces some tests.

## Changelog
[iOS][Changed] - Refactor part of the codegen scripts and add tests.

Differential Revision: https://www.internalfb.com/diff/D35892576?entry_point=27

fbshipit-source-id: 17b63da3d81da6a9482ed6f5268e8f00191bd7b8
facebook#33729)

Summary:
Pull Request resolved: facebook#33729

This PR addresses [this comment](https://www.internalfb.com/diff/D35820848?dst_version_fbid=496290878846487&transaction_fbid=355967423221044).

It makes the CodeGen to the `outputDir` as base directory for the codegen.

Finally, it updates the unit tests accordingly.

## Changelog
[iOS][Changed] - use `outputDir` as base directory for the codegen and remove the possibility to customize the intermediate path. The generated code requires specific paths in the `#include` directive.

Reviewed By: cortinico, dmitryrykun

Differential Revision: D35935282

fbshipit-source-id: 3fdfb0adaa28be10471fd13893433217a739fa1a
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D35935282

cipolleschi pushed a commit to cipolleschi/react-native that referenced this pull request May 3, 2022
facebook#33729)

Summary:
Pull Request resolved: facebook#33729

This PR addresses [this comment](https://www.internalfb.com/diff/D35820848?dst_version_fbid=496290878846487&transaction_fbid=355967423221044).

It makes the CodeGen to the `outputDir` as base directory for the codegen.

Finally, it updates the unit tests accordingly.

## Changelog
[iOS][Changed] - use `outputDir` as base directory for the codegen and remove the possibility to customize the intermediate path. The generated code requires specific paths in the `#include` directive.

Differential Revision: D35935282

fbshipit-source-id: f9e0d29d987fdb4f88b37d540bf6e195798c95db
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @cipolleschi in e4d0153.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label May 4, 2022
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. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants