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

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

Merged
merged 1 commit into from
Sep 15, 2021

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Sep 15, 2021

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:

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;
}
resolve();
});
})
);
}

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:

function filterOutEntrypoints(name) {
// Remove entry point files that are not built in this configuration.
let jsonPath = `build/node_modules/${name}/package.json`;
let packageJSON = JSON.parse(readFileSync(jsonPath));

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

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
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

sure

@sizebot
Copy link

sizebot commented Sep 15, 2021

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 facebook: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 facebook/react-native that referenced this pull request Sep 22, 2021
Summary:
This sync includes the following changes:
- **[f4ac680c7](facebook/react@f4ac680c7 )**: Fixed broken build script --unsafe-partial flag ([#22324](facebook/react#22324)) //<Brian Vaughn>//
- **[67222f044](facebook/react@67222f044 )**: [Experiment] Warn if callback ref returns a function ([#22313](facebook/react#22313)) //<Dan Abramov>//
- **[263cfa6ec](facebook/react@263cfa6ec )**: [Experimental] Add useInsertionEffect ([#21913](facebook/react#21913)) //<Ricky>//
- **[806aaa2e2](facebook/react@806aaa2e2 )**: [useSES shim] Import prefixed native API ([#22310](facebook/react#22310)) //<Andrew Clark>//
- **[fd5e01c2e](facebook/react@fd5e01c2e )**: [useSES/extra] Reuse old selection if possible ([#22307](facebook/react#22307)) //<Andrew Clark>//
- **[33226fada](facebook/react@33226fada )**: Check for store mutations before commit ([#22290](facebook/react#22290)) //<Andrew Clark>//
- **[86c7ca70a](facebook/react@86c7ca70a )**: Fix link ([#22296](facebook/react#22296)) //<Konstantin Popov>//
- **[0fd195f29](facebook/react@0fd195f29 )**: update error message to include useLayoutEffect or useEffect on bad e… ([#22279](facebook/react#22279)) //<salazarm>//
- **[8f96c6b2a](facebook/react@8f96c6b2a )**: [Bugfix] Prevent infinite update loop caused by a synchronous update in a passive effect ([#22277](facebook/react#22277)) //<Andrew Clark>//
- **[4ce89a58d](facebook/react@4ce89a58d )**: Test bad useEffect return value with noop-renderer ([#22258](facebook/react#22258)) //<Sebastian Silbermann>//
- **[a3fde2358](facebook/react@a3fde2358 )**: Detect subscriptions wrapped in startTransition ([#22271](facebook/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
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
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rollup build script --unsafe-partial flag is broken
4 participants