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 errors with codegen discovery flag enabled #1320

Merged
merged 8 commits into from
Aug 25, 2022

Conversation

shwanton
Copy link

@shwanton shwanton commented Aug 2, 2022

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

Although fabric is not supported on RN Desktop, the codegen aspect of turbomodules should work correctly.

Building RNTester with the USE_CODEGEN_DISCOVERY=1 flag was throwing errors.

build command: USE_CODEGEN_DISCOVERY=1 bundle exec pod install;

[Codegen] Done.
TypeError [ERR_INVALID_ARG_TYPE]: The "args" argument must be of type object. Received type string ('/Users/shawndempsey/src/...)
    at new NodeError (node:internal/errors:372:5)
    at normalizeSpawnArguments (node:child_process:515:11)
    at execFileSync (node:child_process:854:13)
    at /Users/shawndempsey/src/shwanton_react-native-macos/scripts/generate-artifacts.js:238:7
    at Array.forEach (<anonymous>)
    at main (/Users/shawndempsey/src/shwanton_react-native-macos/scripts/generate-artifacts.js:211:15)
    at Object.<anonymous> (/Users/shawndempsey/src/shwanton_react-native-macos/scripts/generate-artifacts.js:322:1)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32) {
  code: 'ERR_INVALID_ARG_TYPE'
}

There was a change made to generate-artifacts d68d209 that refactored usage to execFileSync. The error is thrown because execFileSync expects an array as it's second argument.

Changelog

[macOS] [Revert] Revert "Use execFileSync over execSync when messing with file paths in generate-artifacts.js"
[macOS] [Fix] Don't try and build Fabric example when USE_CODEGEN_DISCOVERY=1

Test Plan

Build RNTester w/ flag

USE_CODEGEN_DISCOVERY=1 bundle exec pod install

ScreenshotManager Native TurboModule should work from JS

CleanShot.2022-08-02.at.15.37.55-converted.mp4

@shwanton shwanton marked this pull request as ready for review August 2, 2022 22:59
@shwanton shwanton requested a review from a team as a code owner August 2, 2022 22:59
scripts/generate-artifacts.js Fixed Show fixed Hide fixed
scripts/generate-artifacts.js Fixed Show fixed Hide fixed
scripts/generate-artifacts.js Fixed Show fixed Hide fixed
@shwanton shwanton changed the title Shwanton/fix build with codegen Fix build errors with codegen discovery flag enabled Aug 2, 2022
@Saadnajmi
Copy link
Collaborator

@shwanton Do you know if this change would work on RN-macOS 0.68? Considering whether it's worth back porting.

@shwanton
Copy link
Author

shwanton commented Aug 4, 2022

@Saadnajmi I tested this on the 0.68 branch so will work fine.

christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Aug 9, 2022
Summary:
**Context**

PR: microsoft#1320

Although fabric is not supported on RN Desktop, the codegen aspect of turbomodules should work correctly.

Building RNTester with the `USE_CODEGEN_DISCOVERY=1` flag was throwing errors.

build command: `USE_CODEGEN_DISCOVERY=1 bundle exec pod install;`

```
[Codegen] Done.
TypeError [ERR_INVALID_ARG_TYPE]: The "args" argument must be of type object. Received type string ('/Users/shawndempsey/src/...)
    at new NodeError (node:internal/errors:372:5)
    at normalizeSpawnArguments (node:child_process:515:11)
    at execFileSync (node:child_process:854:13)
    at /Users/shawndempsey/src/shwanton_react-native-macos/scripts/generate-artifacts.js:238:7
    at Array.forEach (<anonymous>)
    at main (/Users/shawndempsey/src/shwanton_react-native-macos/scripts/generate-artifacts.js:211:15)
    at Object.<anonymous> (/Users/shawndempsey/src/shwanton_react-native-macos/scripts/generate-artifacts.js:322:1)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32) {
  code: 'ERR_INVALID_ARG_TYPE'
}
```

There was a change made to `generate-artifacts` d68d209 that refactored usage to `execFileSync`. The error is thrown because `execFileSync` expects an [array as it's second argument](https://nodejs.org/api/child_process.html).

**Changes**

* Revert ["Use execFileSync over execSync when messing with file paths in generate-artifacts.js"](microsoft@d68d209)
* Don't try and build Fabric example when USE_CODEGEN_DISCOVERY=1

Test Plan:
### Build RNTester w/ flag

`USE_CODEGEN_DISCOVERY=1 bundle exec pod install`

Screenshot test (Which uses codegen), should still work.

https://pxl.cl/29g5K

Reviewers: lyahdav, chpurrer

Reviewed By: chpurrer

Subscribers: generatedunixname89002005327315

Differential Revision: https://phabricator.intern.facebook.com/D38436886
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Aug 9, 2022
Summary:
**Context**

PR: microsoft#1320

Although fabric is not supported on RN Desktop, the codegen aspect of turbomodules should work correctly.

Building RNTester with the `USE_CODEGEN_DISCOVERY=1` flag was throwing errors.

build command: `USE_CODEGEN_DISCOVERY=1 bundle exec pod install;`

```
[Codegen] Done.
TypeError [ERR_INVALID_ARG_TYPE]: The "args" argument must be of type object. Received type string ('/Users/shawndempsey/src/...)
    at new NodeError (node:internal/errors:372:5)
    at normalizeSpawnArguments (node:child_process:515:11)
    at execFileSync (node:child_process:854:13)
    at /Users/shawndempsey/src/shwanton_react-native-macos/scripts/generate-artifacts.js:238:7
    at Array.forEach (<anonymous>)
    at main (/Users/shawndempsey/src/shwanton_react-native-macos/scripts/generate-artifacts.js:211:15)
    at Object.<anonymous> (/Users/shawndempsey/src/shwanton_react-native-macos/scripts/generate-artifacts.js:322:1)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32) {
  code: 'ERR_INVALID_ARG_TYPE'
}
```

There was a change made to `generate-artifacts` d68d209 that refactored usage to `execFileSync`. The error is thrown because `execFileSync` expects an [array as it's second argument](https://nodejs.org/api/child_process.html).

**Changes**

* Revert ["Use execFileSync over execSync when messing with file paths in generate-artifacts.js"](microsoft@d68d209)
* Don't try and build Fabric example when USE_CODEGEN_DISCOVERY=1

Test Plan:
### Build RNTester w/ flag

`USE_CODEGEN_DISCOVERY=1 bundle exec pod install`

Screenshot test (Which uses codegen), should still work.

https://pxl.cl/29g5K

Reviewers: lyahdav, chpurrer

Reviewed By: chpurrer

Subscribers: generatedunixname89002005327315

Differential Revision: https://phabricator.intern.facebook.com/D38436886
@Saadnajmi
Copy link
Collaborator

@amgleitman Could you take a second look at this? I worry about the changes of execFileSync -> execSync because security.

@Saadnajmi Saadnajmi added the Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) label Aug 9, 2022
@Saadnajmi
Copy link
Collaborator

@shwanton execSync is more vulnerable to malformed input moreso than execFileSync, which is why we made the original change. Any chance you can refactor this to use execFileSync?

@Saadnajmi Saadnajmi added Needs: Author Feedback and removed Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) labels Aug 9, 2022
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this pull request Aug 10, 2022
Summary:
**Context**

PR: microsoft#1320

Although fabric is not supported on RN Desktop, the codegen aspect of turbomodules should work correctly.

Building RNTester with the `USE_CODEGEN_DISCOVERY=1` flag was throwing errors.

build command: `USE_CODEGEN_DISCOVERY=1 bundle exec pod install;`

```
[Codegen] Done.
TypeError [ERR_INVALID_ARG_TYPE]: The "args" argument must be of type object. Received type string ('/Users/shawndempsey/src/...)
    at new NodeError (node:internal/errors:372:5)
    at normalizeSpawnArguments (node:child_process:515:11)
    at execFileSync (node:child_process:854:13)
    at /Users/shawndempsey/src/shwanton_react-native-macos/scripts/generate-artifacts.js:238:7
    at Array.forEach (<anonymous>)
    at main (/Users/shawndempsey/src/shwanton_react-native-macos/scripts/generate-artifacts.js:211:15)
    at Object.<anonymous> (/Users/shawndempsey/src/shwanton_react-native-macos/scripts/generate-artifacts.js:322:1)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32) {
  code: 'ERR_INVALID_ARG_TYPE'
}
```

There was a change made to `generate-artifacts` d68d209 that refactored usage to `execFileSync`. The error is thrown because `execFileSync` expects an [array as it's second argument](https://nodejs.org/api/child_process.html).

**Changes**

* Revert ["Use execFileSync over execSync when messing with file paths in generate-artifacts.js"](microsoft@d68d209)
* Don't try and build Fabric example when USE_CODEGEN_DISCOVERY=1

Test Plan:
### Build RNTester w/ flag

`USE_CODEGEN_DISCOVERY=1 bundle exec pod install`

Screenshot test (Which uses codegen), should still work.

https://pxl.cl/29g5K

Reviewers: lyahdav, chpurrer

Reviewed By: chpurrer

Subscribers: generatedunixname89002005327315

Differential Revision: https://phabricator.intern.facebook.com/D38436886
@shwanton
Copy link
Author

Yes, I'll refactor to use execFileSync & call w/ correct args.

@ghost ghost removed the Needs: Author Feedback label Aug 16, 2022
@shwanton
Copy link
Author

@Saadnajmi Updated to use execFileSync!

@Saadnajmi Saadnajmi merged commit aace54c into microsoft:main Aug 25, 2022
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Aug 25, 2022
* Revert "Use execFileSync over execSync when messing with file paths in generate-artifacts.js"

This reverts commit d68d209.

* [RN][macos] Don't try and build Fabric example when USE_CODEGEN_DISCOVERY=1

* use `execFileSync` for secure argument passing

* Fix tag comments

* Don't use template literals as arguments

Co-authored-by: Shawn Dempsey <shawndempsey@fb.com>
Saadnajmi added a commit that referenced this pull request Aug 25, 2022
* Revert "Use execFileSync over execSync when messing with file paths in generate-artifacts.js"

This reverts commit d68d209.

* [RN][macos] Don't try and build Fabric example when USE_CODEGEN_DISCOVERY=1

* use `execFileSync` for secure argument passing

* Fix tag comments

* Don't use template literals as arguments

Co-authored-by: Shawn Dempsey <shawndempsey@fb.com>

Co-authored-by: Shawn Dempsey <96719+shwanton@users.noreply.github.com>
Co-authored-by: Shawn Dempsey <shawndempsey@fb.com>
@shwanton shwanton deleted the shwanton/fix-build-with-codegen branch November 3, 2022 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants