Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 29, 2020

This allows the closure compiler to successfully fall back to the
java version if the native version is not found.

See: google/closure-compiler-npm#160

Fixes #10304

@sbc100 sbc100 requested a review from juj January 29, 2020 04:34
@juj
Copy link
Collaborator

juj commented Jan 29, 2020

Can we only apply this forced java if user did not pass a --closure-args --platform=javascript flag? We are not seeing these issues in our own testing, so we'd like to be able to use the JS version, perhaps explicitly passing --closure-args --platform=javascript if we don't like that on by default?

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 29, 2020

Are you sure you are not using the native version rather than the JS version? It seems the native version is always the default internally. I tried the JS version and it was super slow.

But yes I think we should do as you say.

I'd also like to find a way to allow the native version to continue to take precedence.

@juj
Copy link
Collaborator

juj commented Jan 29, 2020

I'd also like to find a way to allow the native version to continue to take precedence.

Oh right, actually when I wrote above comment I forgot the existence of the native version altogether and thought it was the JS version I've been using, yeah, we'd then be using --platform=native with precedence instead of --platform=javascript.

@sbc100 sbc100 changed the title Force closure compiler to use java version Set JAVA_HOME when running closure compiler Jan 29, 2020
@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 29, 2020

Ok, I found a better solution which is simply to set JAVA_HOME. This allows the java backend to be selected in the absence of the native backend.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 29, 2020

PTAL ASAP, I'm hoping this change will unblock out release builder and enable us to cut a release.

This allows the closure compiler to successfully fall back to the
java version if the native version is not found.

See: google/closure-compiler-npm#160

Fixes #10304
@sbc100 sbc100 requested a review from kripken January 29, 2020 23:13
tools/shared.py Outdated
env = os.environ.copy()
env['PATH'] = env['PATH'] + os.pathsep + get_node_directory()
# Closure compiler expects JAVA_HOME to be set in order to enable the java backend. Without
# this is will only try the native and javascriptn version.
Copy link
Member

Choose a reason for hiding this comment

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

typo in javascript, extra n at the end

@sbc100 sbc100 merged commit ad899df into master Jan 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the closure_java branch January 29, 2020 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Closure compiler breakage on windows emscripten-releases builder

4 participants