Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#7913 closed enhancement (fixed)

Shrinksafe refactor

Reported by: Adam Peller Owned by: James Burke
Priority: high Milestone: 1.3
Component: ShrinkSafe Version: 1.2.0
Keywords: rhino Cc: backhous@…, alex
Blocked By: Blocking:

Description (last modified by Adam Peller)

Patches courtesy of Richard Backhouse (IBM Rational)

The following patch breaks apart the Dojo mods into a separate jar from Rhino, relieving us of the patch mess and more clearly delineating the MPL license, I think.

There is a compile-time dependency on JS, so the shrinksafe.jar will have to be rebuilt for new Rhino releases.

This patch also addresses upgrading to Rhino 1.7 (#7127)

Richard adds an -escape-unicode switch (#6628)

It also provides a new Main and a smaller set of options, so the user interface is entirely different than Rhino (though one can continue to run JS from js.jar, or compressScript programmatically by including the new jar in the classpath)

Attachments (2)

buildscripts.patch (8.9 KB) - added by Adam Peller 11 years ago.
modifies buildscripts to use new shrinksafe.jar
shrinksafe.patch (47.3 KB) - added by Adam Peller 11 years ago.
creates separate shrinksafe.jar, just add js.jar and run build.js. Patch from Richard Backhouse (IBM, CCLA)

Download all attachments as: .zip

Change History (10)

Changed 11 years ago by Adam Peller

Attachment: buildscripts.patch added

modifies buildscripts to use new shrinksafe.jar

Changed 11 years ago by Adam Peller

Attachment: shrinksafe.patch added

creates separate shrinksafe.jar, just add js.jar and run build.js. Patch from Richard Backhouse (IBM, CCLA)

comment:1 Changed 11 years ago by Adam Peller

Description: modified (diff)

Richard cracked the 1.7 problem (#7127):

The culprit was collectFuncNodes .It was not setting the parent param and arg values at the correct level, Basically the parent values were only ever set at the inner most function when they should be set at each function level. The reason why it worked in 1.6 was that for some reason ScriptOrFnNode?.getParamAndVarNames was not returning the "bar" function name as one of its values. This sounds like it was a rhino bug in 1.6 that they fixed in 1.7.

comment:2 Changed 11 years ago by Adam Peller

we may wish to introduce a call to shrinksafe/build.js in our main build scripts -- it will be necessary to check in a copy of js.jar, and we may want to check in an up-to-date built copy of shrinksafe.jar as well.

comment:3 Changed 11 years ago by James Burke

Very cool! I hope to look at it more in a couple days. Some comments/questions:

1) I'm partial to keeping the name as custom_rhino.jar, any good arguments for calling it js.jar?

2) I saw that Rhino 1.7R2 release candidate is available, with AST walking. I think we are OK just sticking with 1.7 for now. But it would be good to update the BUILD test file in the shrinksafe directory with notes on where/how to fetch the rhino JAR file that corresponds with these changes, and where to put it to do the build.

3) Any thoughts on the command-line options change? Does that follow into the area of breaking backwards API? I guess we can just document in the release notes.

comment:4 Changed 11 years ago by Richard Backhouse

Regarding 1) js.jar is the name of the jar that you get when you download Rhino from mozilla. I'm not sure it makes sense to continue calling it custom_rhino.jar as we are leaving it untouched.

comment:5 Changed 11 years ago by Adam Peller

1) What Richard said. I know renaming the jar will cause some pain, but I think it's the logical thing to do (and if we're changing the API and command line interface, why not rename it, too) The change in the command-line interface further highlights the importance of not calling this custom_rhino, since it's not an interpreter when you invoke that jar. It's more like an application built with Rhino as an implementation detail. I also like the fact that two jars make explicit which parts of our build are actually using an interpreter (js.jar) and which require the shrinksafe code.

2) Wow, AST walking could change the way we do Shrinksafe! As for what we've got now, I'm tempted to continue to check in js.jar and the shrinksafe.jar result, yet invoke the build scripts with the full build to make sure it's up-to-date.

3) Yeah, it will be perceived as breaking build scripts :( but it's tooling -- I don't think the promise extends to non-JS. Either way, we'd have to issue a clear yet humble warning in the release notes.

comment:6 Changed 11 years ago by James Burke

1) Sorry, I just skimmed the patch: I was assuming we would take the js.jar, unjar it, and add our classes to it. I see now the files are kept separate.

I don't like having the extra JAR file: it starts down the path of having to juggle jars, and their order, and such. But I can see where it helps with avoiding conflicts with other js.jars. It would be really neat if the shrinksafe Main could detect what version of js.jar it was using and advise if it is compatible. Anyway icing for later. Thanks for the clarification.

comment:7 Changed 11 years ago by James Burke

Resolution: fixed
Status: newclosed

SVN/Trac integration broken? Fixed in [15529]. Just updated the release notes. Here is the comment I made with the checkin:

Refs #7913: Great patch from Richard Backhouse, IBM Rational, CCLA, that pulls out shrinksafe code from js.jar. No more custom diffs/patches to rhino. Also Fixes #6628 and Fixes #7127. Need to update release notes, then I will close the bug. Using Rhino 1.7R1 for the js.jar file.

comment:8 Changed 10 years ago by Greg Wilkins

(In [16290]) Refs #7913 new build java exec

Note: See TracTickets for help on using tickets.