improve compatibility for closure build

[gengjiawen]

Summary

java print some output to stderr, caused wrong error detect

-- PLUGIN_ERROR (scripts/rollup/plugins/closure-plugin) --
Error: Picked up JAVA_TOOL_OPTIONS:  -Xmx3489m

    at /workspace/react/scripts/rollup/plugins/closure-plugin.js:16:16
    at ChildProcess.<anonymous> (/workspace/react/node_modules/google-closure-compiler/lib/node/closure-compiler.js:103:9)
    at ChildProcess.emit (node:events:513:28)
    at maybeClose (node:internal/child_process:1093:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:302:5)
error Command failed with exit code 1.                                                                       
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

How did you test this change?

This will fail: https://gitpod.io/#https://github.com/facebook/react

This works: https://gitpod.io/#https://github.com/facebook/react/pull/25150

[sizebot]

Comparing: 9ff738f53f228ea4343886a1e719ec2f1533d300...f294a63b1b9752cc6def4e02fbab45aacbfa34c5

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	=	134.97 kB	134.97 kB	=	43.23 kB	43.23 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	141.64 kB	141.64 kB	=	45.20 kB	45.20 kB
facebook-www/ReactDOM-prod.classic.js	=	486.06 kB	486.06 kB	=	86.51 kB	86.51 kB
facebook-www/ReactDOM-prod.modern.js	=	471.34 kB	471.34 kB	=	84.29 kB	84.29 kB
facebook-www/ReactDOMForked-prod.classic.js	=	486.06 kB	486.06 kB	=	86.51 kB	86.51 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by :no_entry_sign: dangerJS against f294a63b1b9752cc6def4e02fbab45aacbfa34c5

[gengjiawen]

@gaearon Can you take a look ? thx.

[gaearon]

Can you verify that both stdout and stderr always get printed? Both locally and on CI. If not please add logs.

[gengjiawen]

Can you verify that both stdout and stderr always get printed? Both locally and on CI. If not please add logs.

I am not sure I get this, stdOut and stdErr is not printed in original logic.

The stdout is build js content, not sure it's what wanted.

[gaearon]

Ah I see.

Could there be a case where stdErr is non-empty but errorCode is 0 so we miss some important information that previously would've gotten the build to reject?

[gaearon]

Like, what if GCC prints some important warning but doesn't fail the build. Would we want to see it? Should we maybe fix the original problem that you're running into? Like presumably some env variable being set incorrectly.

[gengjiawen]

Like, what if GCC prints some important warning but doesn't fail the build. Would we want to see it? Should we maybe fix the original problem that you're running into? Like presumably some env variable being set incorrectly.

Maybe this ?

if (exitCode === 0) {
    if (stdErr) {
        console.log(stdErr.toString())
    }
    resolve(stdOut);
}

Java and R is quite weirdo on this. They even print version in stderr. You can see https://sourcegraph.com/github.com/tabrindle/envinfo@main/-/blob/src/helpers/languages.js?L47

[gengjiawen]

@gaearon ping

[xc1427]

I meet the same issue when using gitpod.io to build the source.

I think this issue may be due to some rare behavior of java in some Linux distro.

@gengjiawen could you please resolve the PR conflict?

[gengjiawen]

@xc1427 done. @gaearon Can you take another look ?

[react-sizebot]

Comparing: 6f18664b82b61d34b30c794a151d7b032f8eabe0...59fe29f351195722bbe8e13da3f6c737c96170a8

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.js	=	497.71 kB	497.71 kB	=	88.93 kB	88.93 kB
oss-experimental/react-dom/cjs/react-dom.production.js	=	504.00 kB	504.00 kB	=	89.95 kB	89.95 kB
facebook-www/ReactDOM-prod.classic.js	=	591.14 kB	591.14 kB	=	103.91 kB	103.91 kB
facebook-www/ReactDOM-prod.modern.js	=	566.95 kB	566.95 kB	=	100.12 kB	100.12 kB
test_utils/ReactAllWarnings.js	Deleted	64.67 kB	0.00 kB	Deleted	16.09 kB	0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
test_utils/ReactAllWarnings.js	Deleted	64.67 kB	0.00 kB	Deleted	16.09 kB	0.00 kB

Generated by :no_entry_sign: dangerJS against 59fe29f351195722bbe8e13da3f6c737c96170a8

[xc1427]

@xc1427 done. @gaearon Can you take another look ?

@gengjiawen 3q. Dan is no longer in facebook. Maybe ask someone else to review the PR ?

[github-actions[bot]]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.