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

refs #17809, Fix Dojo build #18

Closed agavrilov closed 10 years ago

agavrilov commented 10 years ago

Fix #17809: Dojo build doesn't work with the latest Closure Compiler

Use com.google.javascript.jscomp.SourceFile instead of deprecated (and deleted) com.google.javascript.jscomp.JSSourceFile

neonstalwart commented 10 years ago

i assume this would mean we also need to update util/closureCompiler/compiler.jar. is that right?

cjolif commented 10 years ago

If we happen to do something like that I would like the code to still be compatible with older Closure compiler versions as well which might not be the case of the proposed PR (for those who can't easily upgrade closure compiler for legal reasons).

agavrilov commented 10 years ago

The Pull Request is compatible with the version of Closure Compiler distributed with Dojo 1.9.3

cjolif commented 10 years ago

The Pull Request is compatible with the version of Closure Compiler distributed with Dojo 1.9.3

Thanks is that true with the version shipped in 1.8 as well?

agavrilov commented 10 years ago

Yes, changes in PR are compatible with Dojo 1.8.6 as well

cjolif commented 10 years ago

Cool, then I have no objection but I will let others with probably more experience related to this decides what to do.

dylans commented 10 years ago

Seems like a really trivial fix.

Confirmed with the commit at https://github.com/google/closure-compiler/commit/6b49cfd9022fe111bdad745c78ea877fbc2f7ba3#diff-04fe91ccdc40db0596f4449c7eb63406 that they first copied this file, and then later removed it.

The "new" file was copied in 2009, and is there in our old version of Closure as well.

We should accept and merge this pull request.

wkeese commented 10 years ago

Sounds good to me.

csnover commented 10 years ago

Is there a reason to not update Closure Compiler and Rhino to latest versions at the same time? The latest CC requires Java 7, but…we’re up to Java 8 now, so I don’t see this as being an issue. Builds still complete successfully with the latest.

csnover commented 10 years ago

The main issue from this PR was fixed in 5560ac3ef5eeed782118dec180be444d8695645e; there is more than one place where this had to be changed. I’ll leave it open momentarily for people to respond to the question about CC/Rhino.

dylans commented 10 years ago

I would suggest we update to newer versions, as they both presumably fix and improve things. I'm sure that anything to help make a build run faster would be greatly appreciated by the community.

csnover commented 10 years ago

Turns out that ShrinkSafe relies on some parser API that changed in some way in the newer Rhino and so crashes. I’m not going to bother to try to fix it. We could either remove ShrinkSafe completely, or deal with the fact that things are never going to be upgradeable for as long as it continues to come with.