Skip to content

Conversation

@juj
Copy link
Collaborator

@juj juj commented Mar 4, 2022

Fix native Closure Compiler to work.

Reverts my earlier PR #803 which was a great mistake, as it caused Emscripten to silently fall back to Java version of the Closure Compiler.

After this PR, Closure Compiler will work on user platforms that do not have Java installed.

Also forcibly remove Java version of Closure Compiler on systems where installing the native version succeeds, in order to save on disk size.

emsdk.py Outdated
closure_compiler_native = 'google-closure-compiler-windows'

if closure_compiler_native:
closure_compiler_native += '@20220104.0.0'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this need updating every time we do an npm update on the emscripten-side? (iirc we have updated several times over the years so any one value here might be work).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it does, and that is quite unfortunate. I wrote a little bit about that earlier in #802 (comment) . I don't see another way but to keep these in sync.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we maybe do something like this:

closure_version = json.load('package.json')['dependencies']['google-closure-compiler']

@juj juj enabled auto-merge (squash) March 4, 2022 15:32
# Manually install the appropriate native Closure Compiler package
# This is currently needed because npm ci will install the packages
# for Closure for all platforms, adding 180MB to the download size
# There are two problems here:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since npm ci is actually run above, perhaps mention either up there, or here in this comment that the above command deliberately excludes the native versions of closure compiler by the use of the --no-optional flag.

emsdk.py Outdated
closure_compiler_native = 'google-closure-compiler-windows'

if closure_compiler_native:
closure_compiler_native += '@20220104.0.0'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we maybe do something like this:

closure_version = json.load('package.json')['dependencies']['google-closure-compiler']

@juj juj disabled auto-merge March 4, 2022 15:43
@juj
Copy link
Collaborator Author

juj commented Mar 4, 2022

Can we maybe do something like this:

That is a great idea! I'll look into that a bit later.

juj added 3 commits April 13, 2022 11:00
… was a great mistake, as it caused Emscripten to silently fall back to Java version of the Closure Compiler. After this PR, Closure Compiler will work on user platforms that do not have Java installed. Also forcibly remove Java version of Closure Compiler on systems where installing the native version succeeds, in order to save on code size.
@juj juj force-pushed the fix_closure_compiler branch from 70b9341 to fc92dbd Compare April 13, 2022 08:03
@juj juj force-pushed the fix_closure_compiler branch from fc92dbd to 09cc9cb Compare April 13, 2022 09:57
@juj juj merged commit 051d745 into main Apr 13, 2022
@juj juj deleted the fix_closure_compiler branch April 13, 2022 13:45
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.

3 participants