search

dojo / util

Dojo 1 - build utilities. Please submit bugs to https://bugs.dojotoolkit.org/
https://dojotoolkit.org/
Other
60 stars 105 forks source link

Fixes #18919 - New version of closure compiler breaks builds #50

Closed vansimke closed 7 years ago

vansimke commented 7 years ago

This PR was originally intended to revert a008758314572cb17fab19a0ab286c5f4fd4e9b0, but further research has shown that the options "deadAssignmentElmination" was causing the issues observed in 18919. This PR has been updated to force this setting to false in order to eliminate the issue.

Original Description: Revert change to update version of Google Closure Compiler due to optimizations causing builds to break.

This reverts commit a008758314572cb17fab19a0ab286c5f4fd4e9b0.

sindilevich commented 7 years ago

Hello @vansimke,

An updated version of Google Closure compiler is highly required. It will allow using ECMAScript 2015 and newer syntax with code that targets Dojo 1.x.

Is there an option to still support a newer version of the Google Closure compiler and all its options as in https://github.com/dojo/util/commit/a008758314572cb17fab19a0ab286c5f4fd4e9b0? Maybe, disabling some of its optimizations by default?

vansimke commented 7 years ago

@sindilevich I completely agree and I'm going to spend a bit more time trying to figure out how to get this to work safely. If you have time to help out, I have created a repo that shows my test environment (https://github.com/vansimke/dojo_18919). To test it, just run http-server (or similar file server) from the repo root and navigate to http://localhost:8080 to see the error. If you switch line 11 of index.html to point to dojo.js.uncompressed.js, then the page loads properly.

The util folder's closureCompiler subfolder contains both compiler versions (1.12's version is currently being used). You can switch between the two by copying the appropriate .jar to "compiler.jar" and rebuilding with

node dojo/dojo.js load=build --profile build.profile.js

vansimke commented 7 years ago

I've found that if I manually compile the uncompressed dojo.js.uncompressed.js file, it seems to generate a layer that works when using the compiler included in 1.12.0.rc2. At this point, I'm trying to figure out what settings are being provided by the build system that might be causing the issue.

To manually compile I'm running

java -jar {path to compiler.jar} dojo.js.uncompressed.jar > {dest file}

sindilevich commented 7 years ago

@vansimke, thank you! By the way, why https://github.com/dojo/util/commit/a008758314572cb17fab19a0ab286c5f4fd4e9b0 proposes updating the Google Closure compiler to v20160713, whereas there are newer versions with bug fixes and new implemented features available?

vansimke commented 7 years ago

Yeah, I tried that originally, but then I ran into a problem where the compilation itself failed. I didn't dig into why, but I decided that I already had enough on my plate that I didn't want to open up another line of investigation.

I think what is required is to build an actual Java app that consumes the source code for the compiler and tries different configuration options until one works. However, I've run out of time to work on this right now. I'll get back to it as soon as I can.

vansimke commented 7 years ago

I've been able to spend some time with the closure compiler and tracked down the differences between the default options (i.e. those used by the compiler when used on the command line) and those used by Dojo builds. It turns out that the deadAssignmentElimination flag was causing the build issue. I have updated this PR to reinstate the newer version of the closure compiler and update the optimizeRunner.js file to hard-code this setting to prevent broken builds. This does not guarantee that some other change in the compiler could result in issues, but it does fix the issue raised in 18919.

vansimke commented 7 years ago

I've made another small change to the optimizeRunner. It was setting the default options for compilation level after processing the optimizeOptions. This resulted in overwriting the explicitly set options. I've flipped those around to give more control over the options.

sindilevich commented 7 years ago

@vansimke, you have done a great job. Thank you!

Because I still couldn't build a dojo development environment on my machine, can I ask you check the compatibility of Google Closure Compiler v20160911 (https://github.com/google/closure-compiler/releases/tag/v20160911) or newer with dojo 1.12 please?

This version and up introduce important bug fixes in transpilation. They also feature this commit https://github.com/google/closure-compiler/commit/0a1dfc21846bd98a60f2e86387e4766fcb3d1b76, that may fix dojo compilation, even with the enabled deadAssignmentElimination flag.

I believe the latest version of the Google Closure Compiler will produce a lot of errors, compiling dojo 1.12, but v20160911 should be more or less OK. The reason the latest version of the Google Closure Compiler may produce errors, compiling dojo 1.12, is that commit https://github.com/google/closure-compiler/commit/72aa5a7d132f05b4290f2f05052ecc2f520816c8.

vansimke commented 7 years ago

I updated the JAR to v20160911 and confirmed that the test case mentioned in 18919 continues to work as expected. The PR has been updated to reflect this new version.

sindilevich commented 7 years ago

@vansimke, hope your PR gets merged ASAP.

vansimke commented 7 years ago

Closed via https://github.com/dojo/util/commit/f8656829ad3a84b2e41f901758dd83532db80a8e

gerpres commented 7 years ago

I can still reproduce https://bugs.dojotoolkit.org/ticket/18919 using the latest files!?

sindilevich commented 7 years ago

@dylans, @vansimke, @gerpres, I can still reproduce the same issue too. I think the deal is that the above commit has fixed it in the https://github.com/dojo/util/blob/master/build/optimizeRunner.js file, without taking care of the https://github.com/dojo/util/blob/master/build/transforms/optimizer/closure.js file. According to this comment (​https://github.com/dojo/util/pull/27#issuecomment-127580856): "Changes made in this area will also have to be applied twice. Once to optimizeRunner.js (used in a 'Node build' when spawning a JVM to run closure): build/optimizeRunner.js. And once to closure.js (used in a 'Java build'): build/transforms/optimizer/closure.js".

I run a Java build for Dojo, using the https://github.com/dojo/util/blob/master/buildscripts/build.bat with Java 7. Such builds do not have any of the recent changes, made for Node builds.

I could re-check the issue, when you bring the build/transforms/optimizer/closure.js on par with the Node-version.

vansimke commented 7 years ago

Excellent find. I completely forgot that Java builds use different files. I'll take a look at that tomorrow. Thanks!

sindilevich commented 7 years ago

@vansimke, thank you. I'll be waiting for an updated version/new RC to try it out. It'd be super-cool if we could act as described at the end of that https://github.com/dojo/util/pull/27#issuecomment-127580856:

These two pieces of code (that are responsible for orchestrating the closure compiler) are almost identical, and could (should?) probably be refactored into a single module to be shared.

vansimke commented 7 years ago

@sindilevich I've submitted a new PR (https://github.com/dojo/util/pull/54) to fix this issue with Java builds. I also refactored my previous fix for Node builds a bit to allow the user to overwrite the deadAssignmentElimination parameter via the build profile if they desire to. Could you (or @gerpres) check this out to see if it builds properly now?