Skip to content

Fixed broken build script --unsafe-partial flag#22324

Merged
bvaughn merged 1 commit into
react:mainfrom
bvaughn:fix-build-script-ncp
Sep 15, 2021
Merged

Fixed broken build script --unsafe-partial flag#22324
bvaughn merged 1 commit into
react:mainfrom
bvaughn:fix-build-script-ncp

Conversation

@bvaughn

@bvaughn bvaughn commented Sep 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Resolves #22323

The following example command sequence will fail:

# Grab the latest build artifacts from CI
scripts/release/download-experimental-build.js --commit=main

# Rebuild only the local NODE bundle from the source of react-dom
# Leave all other artifacts untouched
yarn build --unsafe-partial --type=NODE_DEV react-dom/index

# Throws

The above sequence should work but instead fails with one of the two errors below:

Error: ENOENT: no such file or directory

Error: SyntaxError: Unexpected end of JSON input

This leaves the repo in a broken state (tests won't run, DevTools test shell will crash, etc).

These errors are thrown because this function prematurely resolves:
https://github.com/facebook/react/blob/67222f044c582a1a10f3b9d15d9daf67d5dc5c4f/scripts/rollup/utils.js#L10-L24

Either before the package.json file has been copied, or after it has been copied (but while its contents are still empty).

This causes the subsequent read of package.json to throw:
https://github.com/facebook/react/blob/67222f044c582a1a10f3b9d15d9daf67d5dc5c4f/scripts/rollup/packaging.js#L145-L148

The error above can be verified by adding logging to the asyncTopyTo method:

function asyncCopyTo(from, to) {
  return asyncMkDirP(path.dirname(to)).then(
    () =>
      new Promise((resolve, reject) => {
        ncp(from, to, error => {
          if (error) {
            // Wrap to have a useful stack trace.
            reject(new Error(error));
            return;
          }
          if (to.includes('package.json')){
            console.log(`asyncCopyTo() -> ncp("${from}", "${to}") -> resolve`);

            // This line will either throw (no file) or print an empty string in many cases:
            console.log(require('fs').readFileSync(to).toString());
          }
          resolve();
        });
      })
  );
}

I'm able to "fix" this issue by introducing a small amount of delay, e.g.

diff --git a/scripts/rollup/packaging.js b/scripts/rollup/packaging.js
index eaf78959a6..5844d9116f 100644
--- a/scripts/rollup/packaging.js
+++ b/scripts/rollup/packaging.js
@@ -196,6 +196,10 @@ async function prepareNpmPackage(name) {
     ),
     asyncCopyTo(`packages/${name}/npm`, `build/node_modules/${name}`),
   ]);
+
+  // Wait for copied files to exist; asyncCopyTo() completes prematurely.
+  await new Promise(resolve => setTimeout(resolve, 100));
+
   filterOutEntrypoints(name);
   const tgzName = (
     await asyncExecuteCommand(`npm pack build/node_modules/${name}`)

But that's pretty hacky and fragile.

I evaluated replacing ncp with some other alternatives:

Seems like this is perhaps related to AvianFlu/ncp#127

With this change, I am able to successfully run the command shown above:

Screen Shot 2021-09-15 at 11 13 48 AM

Fixed broken build script --unsafe-partial flag
c1a5676 
This flag was broken due to a buggy race case in the ncp() command. The fix is amittedly a hack but improves on the existing behavior (of leaving the workspace in a broken state).
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 15, 2021
gaearon
gaearon approved these changes Sep 15, 2021
View reviewed changes

@gaearon gaearon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@sizebot

sizebot commented Sep 15, 2021

Copy link
Copy Markdown

Comparing: 50263d3...c1a5676

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 129.49 kB 129.49 kB = 41.26 kB 41.26 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 132.31 kB 132.31 kB = 42.21 kB 42.21 kB
facebook-www/ReactDOM-prod.classic.js = 410.86 kB 410.86 kB = 76.01 kB 76.01 kB
facebook-www/ReactDOM-prod.modern.js = 399.43 kB 399.43 kB = 74.29 kB 74.29 kB
facebook-www/ReactDOMForked-prod.classic.js = 410.86 kB 410.86 kB = 76.01 kB 76.01 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against c1a5676

@bvaughn bvaughn merged commit f4ac680 into react:main Sep 15, 2021
@bvaughn bvaughn deleted the fix-build-script-ncp branch September 15, 2021 17:32
facebook-github-bot pushed a commit to react/react-native that referenced this pull request Sep 22, 2021
@kacieb @facebook-github-bot
React Native sync for revisions 95d762e...e8feb11
8802189 
Summary:
This sync includes the following changes:
- **[f4ac680](react/react@f4ac680c7 )**: Fixed broken build script --unsafe-partial flag ([#22324](react/react#22324)) //<Brian Vaughn>//
- **[67222f0](react/react@67222f044 )**: [Experiment] Warn if callback ref returns a function ([#22313](react/react#22313)) //<Dan Abramov>//
- **[263cfa6](react/react@263cfa6ec )**: [Experimental] Add useInsertionEffect ([#21913](react/react#21913)) //<Ricky>//
- **[806aaa2](react/react@806aaa2e2 )**: [useSES shim] Import prefixed native API ([#22310](react/react#22310)) //<Andrew Clark>//
- **[fd5e01c](react/react@fd5e01c2e )**: [useSES/extra] Reuse old selection if possible ([#22307](react/react#22307)) //<Andrew Clark>//
- **[33226fa](react/react@33226fada )**: Check for store mutations before commit ([#22290](react/react#22290)) //<Andrew Clark>//
- **[86c7ca7](react/react@86c7ca70a )**: Fix link ([#22296](react/react#22296)) //<Konstantin Popov>//
- **[0fd195f](react/react@0fd195f29 )**: update error message to include useLayoutEffect or useEffect on bad e… ([#22279](react/react#22279)) //<salazarm>//
- **[8f96c6b](react/react@8f96c6b2a )**: [Bugfix] Prevent infinite update loop caused by a synchronous update in a passive effect ([#22277](react/react#22277)) //<Andrew Clark>//
- **[4ce89a5](react/react@4ce89a58d )**: Test bad useEffect return value with noop-renderer ([#22258](react/react#22258)) //<Sebastian Silbermann>//
- **[a3fde23](react/react@a3fde2358 )**: Detect subscriptions wrapped in startTransition ([#22271](react/react#22271)) //<salazarm>//

Changelog:
[General][Changed] - React Native sync for revisions 95d762e...e8feb11

jest_e2e[run_all_tests]

Reviewed By: rickhanlonii

Differential Revision: D30966369

fbshipit-source-id: 6c88e591005deb1fd93493628ef4695add49186c
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
@zhengjitf
Fixed broken build script --unsafe-partial flag (react#22324)
1556166 
This flag was broken due to a buggy race case in the ncp() command. The fix is amittedly a hack but improves on the existing behavior (of leaving the workspace in a broken state).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gaearon gaearon gaearon approved these changes

@jstejada jstejada Awaiting requested review from jstejada

@rickhanlonii rickhanlonii Awaiting requested review from rickhanlonii

@lunaruan lunaruan Awaiting requested review from lunaruan

@acdlite acdlite Awaiting requested review from acdlite

Assignees

No one assigned

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rollup build script --unsafe-partial flag is broken

4 participants

@bvaughn @sizebot @gaearon @facebook-github-bot