Bug 745515 - Review Request: yuicompressor - Tool that supports the compression of both JavaScript and CSS files
Summary: Review Request: yuicompressor - Tool that supports the compression of both Ja...
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 821123
Blocks: 725768 821136
TreeView+ depends on / blocked
 
Reported: 2011-10-12 14:48 UTC by Stanislav Ochotnicky
Modified: 2015-02-12 18:45 UTC (History)
8 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-02-12 18:45:43 UTC


Attachments (Terms of Use)
yuicompressor-2.4.7.pom (1.96 KB, application/octet-stream)
2012-05-12 09:18 UTC, gil cattaneo
no flags Details
port to rhino 1.7R4 (353.53 KB, patch)
2013-05-30 12:51 UTC, Michal Srb
no flags Details | Diff

Description Stanislav Ochotnicky 2011-10-12 14:48:40 UTC
Spec URL: http://sochotni.fedorapeople.org/packages/yuicompressor.spec
SRPM URL: http://sochotni.fedorapeople.org/packages/yuicompressor-2.4.7-0.1.sha4c54e628.fc15.src.rpm

Description:
YUI Compressor is an open source tool that supports the compression of
both JavaScript and CSS files. The JavaScript compression removes
comments and white-spaces as well as obfuscates local variables using
the smallest possible variable name. CSS compression is done using a
regular-expression-based CSS minifier.

Comment 1 Ville Skyttä 2011-10-15 18:17:49 UTC
* LICENSE.TXT UTF-8 conversion should be from Windows-1252, not ISO-8859-1.

* Use %ant instead of plain ant.

* Upstream does not build or ship javadocs, should we?  If you think it's a good idea, please use %javadoc instead of plain javadoc, and crosslink them with java javadocs (BuildRequires: java-javadoc + Requires: java-javadoc + %javadoc -link %{_javadocdir}/java).  Also, drop "-subpackages org" - there's nothing in it that ends up in javadocs with the currently used options.

* Add doc/README and doc/CHANGELOG to %docs.

* rhino dep and build dep needs to be versioned: >= 1.7R3

* Please include a script in the srpm that can be used to recreate the tarball, for example like this one: http://pkgs.fedoraproject.org/gitweb/?p=reptyr.git;a=blob;f=reptyr-snapshot.sh;h=2edfe4e

* Unlike upstream 2.4.6 release, the packaged version seems to have some
comment related issues which I think are severe enough that they need to be fixed before the package can be included, for example:

$ echo "/* hello */" | yuicompressor --type js

[ERROR] 1:11:unterminated comment

[ERROR] 1:0:Compilation produced 1 syntax errors.
org.mozilla.javascript.EvaluatorException: Compilation produced 1 syntax errors.
	at com.yahoo.platform.yui.compressor.YUICompressor$1.runtimeError(YUICompressor.java:154)
[...]

Comment 2 Stanislav Ochotnicky 2011-10-31 15:16:44 UTC
I have fixed all of the problems except the last one (i.e. javascript comment compilation). I believe this might be a problem with rhino itself, will test later. Upstream fails the same way.

http://sochotni.fedorapeople.org/packages/yuicompressor.spec
http://sochotni.fedorapeople.org/packages/yuicompressor-2.4.8-0.1.sha6e2bc23.fc15.src.rpm

Comment 3 Ville Skyttä 2011-10-31 17:45:16 UTC
* As mentioned in comment 1, upstream 2.4.6 does not fail the same way:

$ wget http://yui.zenfs.com/releases/yuicompressor/yuicompressor-2.4.6.zip
$ unzip -q yuicompressor-2.4.6.zip
$ echo "/* hello */" | java -jar yuicompressor-2.4.6/build/yuicompressor-2.4.6.jar --type js

This produces no output, which is the expected result.

* The script to recreate the tarball requested in comment 1 is still missing, and there was no comment about it.  Maybe it went unnoticed?

Comment 4 Stanislav Ochotnicky 2011-11-01 08:13:20 UTC
Upstream version doesn't work with latest Rhino so I'd have to port the patch provided to upstream for latest release. It's weird that upstream has 2.4.7 in git but they haven't released it (and as you said it's not exactly error-free).

The tarball generation script was added, but I guess I haven't re-uploaded the spec/srpm. I've done so now (no release changed, hope you don't mind)

Comment 5 Ville Skyttä 2011-11-01 17:16:00 UTC
Ok, looks good now otherwise, but I think the problem with comments is such a fundamental one that I don't think this package should be approved/pushed without it fixed.  I'm sure upstream will get to it at some point...

Comment 6 Stanislav Ochotnicky 2011-11-02 08:20:05 UTC
Sure. I'll look for a fix in the meantime

Comment 7 gil cattaneo 2012-05-12 09:18:44 UTC
Created attachment 583970 [details]
yuicompressor-2.4.7.pom

hi
can you add this pom? (attached)
is required for build https://github.com/davidB/yuicompressor-maven-plugin.git
thanks

Comment 9 Stanislav Ochotnicky 2012-05-16 14:26:42 UTC
Hmm, my need for yuicompressor has disappeared so I'd like to "move" the review onto gil if he still wants this package. Not sure if that's possible without creating a new review? I guess that would be cleanest way. In any case: Sorry Ville for wasting your time. I wasn't expecting this

Comment 10 Ville Skyttä 2012-05-16 17:28:01 UTC
I don't think it's possible to change the bug reporter, but then again if you don't mind receiving the mails, I'm fine with continuing the review in this bug with gil as the submitter.  Comment 5 still holds though...

Comment 11 Ville Skyttä 2012-10-16 21:27:23 UTC
gil: ping?

Comment 12 gil cattaneo 2012-10-17 14:40:21 UTC
Ville: pong ?

Comment 13 Ville Skyttä 2012-10-17 16:25:58 UTC
gil: I'm interested in thoughts/progress wrt comment 5 (whether the comment bug is being fixed) and comments 9 and 10 (whether you'd like to continue this review as the submitter)?

Comment 14 gil cattaneo 2012-10-17 17:17:48 UTC
hi Ville,
i cant fixed this problem using rhino available in rawhide
there is another approach to the problem (http://anonscm.debian.org/viewvc/pkg-java/trunk/yui-compressor/debian/) but is not allowed
thanks

Comment 15 Ville Skyttä 2012-10-17 18:24:08 UTC
Ok, I'll leave it up to you to either leave this as is waiting for a solution or to close at least for now.

Comment 16 Vít Ondruch 2012-10-18 05:05:48 UTC
Please don't give up! I'd love to see yuicompressor in Fedora, since there is demand for rubygem-yui-compressor. Unfortunately I am not Java guy. Thank you.

Comment 18 Christopher Meng 2013-05-27 09:56:34 UTC
Any news here?

I'm packaging jq which needs rubygem-xxx which needs this.

Oh...

Comment 19 gil cattaneo 2013-05-28 03:13:19 UTC
hi
no any news sorry ... our rhino isnt compatible with this package
there are only one option possible create a new rhino package (e.g. rhino17r"X")
usable only with yuicompressor
regards

p.s. yuicompressor development is stopped?

Comment 20 Matthias Runge 2013-05-28 06:58:58 UTC
> p.s. yuicompressor development is stopped?

That's, what I thought. Taking a second look, it turned out, developers are active

https://github.com/yui/yuicompressor

Comment 21 gil cattaneo 2013-05-28 08:49:12 UTC
hi
yes new release is out but use again rhino 1.7R2...
in fedora rawhide there is 1.7R4 also in F19 (in F18 1.7R3)
problems with sync moz and yiu...
regards

Comment 22 Michal Srb 2013-05-30 12:51:34 UTC
Created attachment 754839 [details]
port to rhino 1.7R4

This patch should make yuicompressor work with our Rhino 1.7R4, tested also with 1.7R3 (in F18). The patch is based on gil's work.

Comment 23 gil cattaneo 2013-05-30 13:52:40 UTC
Spec URL: http://gil.fedorapeople.org/yuicompressor.spec
SRPM URL:
http://gil.fedorapeople.org/yuicompressor-2.4.8-1.fc18.src.rpm

- 2.4.8
- Thanks to Michal Srb, applied patch rhino 1.7R4

Comment 24 Ville Skyttä 2013-06-02 12:42:38 UTC
The BuildRequires and Requires are still rhino >= 1.7R3-2, should they be R4?

At least when running on F-17 (with rhino-1.7R3-4.fc17.noarch), the test case noted in comment 1 still exhibits a bug, although a different one than before:

$ echo "/* hello */" | yuicompressor --type js
Exception in thread "main" java.lang.reflect.InvocationTargetException
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:601)
	at com.yahoo.platform.yui.compressor.Bootstrap.main(Bootstrap.java:21)
Caused by: java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
	at java.util.ArrayList.rangeCheck(ArrayList.java:604)
	at java.util.ArrayList.get(ArrayList.java:382)
	at com.yahoo.platform.yui.compressor.JavaScriptCompressor.getToken(JavaScriptCompressor.java:581)
	at com.yahoo.platform.yui.compressor.JavaScriptCompressor.printSymbolTree(JavaScriptCompressor.java:1097)
	at com.yahoo.platform.yui.compressor.JavaScriptCompressor.compress(JavaScriptCompressor.java:559)
	at com.yahoo.platform.yui.compressor.YUICompressor.main(YUICompressor.java:186)
	... 5 more

Comment 25 Ville Skyttä 2013-06-02 12:51:32 UTC
Also, this version doesn't preserve the C-style comments starting with /*! as documented. The old 2.4.7 version I'm using locally does.

https://github.com/yui/yuicompressor/blob/master/README.md

$ cat t.js 
/*!
  Hello, this comment should be preserved.
*/
var a;
$ yuicompressor t.js
var a;

Comment 26 gil cattaneo 2013-06-02 14:11:11 UTC
work fine also with rhino = 1.7R3, and i have intention of import also for F18 
the errors seem caused by empty js file...
in my system work fine

yuicompressor --type js cssmin.js
var YAHOO=YAHOO||{};YAHOO.compressor=YAHOO.compressor||{};YAHOO.compressor._extractDataUrls=function(i,d){var g=i.length-1,l=0,n,h,a,e,k=[],c,b,f,j=/url\(\s*(["']?)data\:/gi;while((c=j.exec(i))!==null){n=c.index+4;a=c[1];if(a.length===0){a=")"}e=false;h=j.lastIndex-1;while(e===false&&h+1<=g){h=i.indexOf(a,h+1);if((h>0)&&(i.charAt(h-1)!=="\")){e=true;if(")"!=a){h=i.indexOf(")",h)}}}k.push(i.substring(l,c.index));if(e){f=i.substring(n,h);f=f.replace(/\s+/g,"");d.push(f);b="url(___YUICSSMIN_PRESERVED_TOKEN_"+(d.length-1)+"___)";k.push(b);l=h+1}else{k.push(i.substring(c.index,j.lastIndex));l=j.lastIndex}}k.push(i.substring(l));return k.join("")};YAHOO.compressor._compressHexColors=function(d){var e=/(\=\s*?["']?)?#([0-9a-f])([0-9a-f])([0-9a-f])([0-9a-f])([0-9a-f])([0-9a-f])(\}|[^0-9a-f{][^{]*?\})/gi,a,c=0,b,f=[];while((a=e.exec(d))!==null){f.push(d.substring(c,a.index));b=a[1];if(b){f.push(a[1]+"#"+(a[2]+a[3]+a[4]+a[5]+a[6]+a[7]))}else{if(a[2].toLowerCase()==a[3].toLowerCase()&&a[4].toLowerCase()==a[5].toLowerCase()&&a[6].toLowerCase()==a[7].toLowerCase()){f.push("#"+(a[3]+a[5]+a[7]).toLowerCase())}else{f.push("#"+(a[2]+a[3]+a[4]+a[5]+a[6]+a[7]).toLowerCase())}}c=e.lastIndex=e.lastIndex-a[8].length}f.push(d.substring(c));return f.join("")};YAHOO.compressor.cssmin=function(e,g){var k=0,d=0,c=0,h=0,a=[],f=[],b="",l=e.length,j="";e=this._extractDataUrls(e,a);while((k=e.indexOf("/*",k))>=0){d=e.indexOf("*/",k+2);if(d<0){d=l}b=e.slice(k+2,d);f.push(b);e=e.slice(0,k+2)+"___YUICSSMIN_PRESERVE_CANDIDATE_COMMENT_"+(f.length-1)+"___"+e.slice(d);k+=2}e=e.replace(/("([^\\"]|\\.|\\)*")|('([^\\']|\\.|\\)*')/g,function(o){var p,m,n=o.substring(0,1);o=o.slice(1,-1);if(o.indexOf("___YUICSSMIN_PRESERVE_CANDIDATE_COMMENT_")>=0){for(p=0,m=f.length;p<m;p=p+1){o=o.replace("___YUICSSMIN_PRESERVE_CANDIDATE_COMMENT_"+p+"___",f[p])}}o=o.replace(/progid:DXImageTransform\.Microsoft\.Alpha\(Opacity=/gi,"alpha(opacity=");a.push(o);return n+"___YUICSSMIN_PRESERVED_TOKEN_"+(a.length-1)+"___"+n});for(c=0,h=f.length;c<h;c=c+1){b=f[c];j="___YUICSSMIN_PRESERVE_CANDIDATE_COMMENT_"+c+"___";if(b.charAt(0)==="!"){a.push(b);e=e.replace(j,"___YUICSSMIN_PRESERVED_TOKEN_"+(a.length-1)+"___");continue}if(b.charAt(b.length-1)==="\"){a.push("\");e=e.replace(j,"___YUICSSMIN_PRESERVED_TOKEN_"+(a.length-1)+"___");c=c+1;a.push("");e=e.replace("___YUICSSMIN_PRESERVE_CANDIDATE_COMMENT_"+c+"___","___YUICSSMIN_PRESERVED_TOKEN_"+(a.length-1)+"___");continue}if(b.length===0){k=e.indexOf(j);if(k>2){if(e.charAt(k-3)===">"){a.push("");e=e.replace(j,"___YUICSSMIN_PRESERVED_TOKEN_"+(a.length-1)+"___")}}}e=e.replace("/*"+j+"*/","")}e=e.replace(/\s+/g," ");e=e.replace(/(^|\})(([^\{:])+:)+([^\{]*\{)/g,function(i){return i.replace(":","___YUICSSMIN_PSEUDOCLASSCOLON___")});e=e.replace(/\s+([!{};:>+\(\)\],])/g,"$1");e=e.replace(/___YUICSSMIN_PSEUDOCLASSCOLON___/g,":");e=e.replace(/:first-(line|letter)(\{|,)/gi,function(m,i,n){return":first-"+i.toLowerCase()+" "+n});e=e.replace(/\*\/ /g,"*/");e=e.replace(/^(.*)(@charset)( "[^"]*";)/gi,function(m,i,o,n){return o.toLowerCase()+n+i});e=e.replace(/^((\s*)(@charset)( [^;]+;\s*))+/gi,function(m,i,p,o,n){return p+o.toLowerCase()+n});e=e.replace(/\band\(/gi,"and (");e=e.replace(/@(font-face|import|(?:-(?:atsc|khtml|moz|ms|o|wap|webkit)-)?keyframe|media|page|namespace)/gi,function(m,i){return"@"+i.toLowerCase()});e=e.replace(/:(active|after|before|checked|disabled|empty|enabled|first-(?:child|of-type)|focus|hover|last-(?:child|of-type)|link|only-(?:child|of-type)|root|:selection|target|visited)/gi,function(m,i){return":"+i.toLowerCase()});e=e.replace(/:(lang|not|nth-child|nth-last-child|nth-last-of-type|nth-of-type|(?:-(?:moz|webkit)-)?any)\(/gi,function(m,i){return":"+i.toLowerCase()+"("});e=e.replace(/([:,\( ]\s*)(attr|color-stop|from|rgba|to|url|(?:-(?:atsc|khtml|moz|ms|o|wap|webkit)-)?(?:calc|max|min|(?:repeating-)?(?:linear|radial)-gradient)|-webkit-gradient)/gi,function(m,i,n){return i+n.toLowerCase()});e=e.replace(/([!{}:;>+\(\[,])\s+/g,"$1");e=e.replace(/;+\}/g,"}");e=e.replace(/(^|[^0-9])(?:0?\.)?0(?:px|em|%|in|cm|mm|pc|pt|ex|deg|g?rad|m?s|k?hz)/gi,"$10");e=e.replace(/:0 0 0 0(;|\})/g,":0$1");e=e.replace(/:0 0 0(;|\})/g,":0$1");e=e.replace(/:0 0(;|\})/g,":0$1");e=e.replace(/(background-position|transform-origin|webkit-transform-origin|moz-transform-origin|o-transform-origin|ms-transform-origin):0(;|\})/gi,function(m,n,i){return n.toLowerCase()+":0 0"+i});e=e.replace(/(:|\s)0+\.(\d+)/g,"$1.$2");e=e.replace(/rgb\s*\(\s*([0-9,\s]+)\s*\)/gi,function(){var m,n=arguments[1].split(",");for(m=0;m<n.length;m=m+1){n[m]=parseInt(n[m],10).toString(16);if(n[m].length===1){n[m]="0"+n[m]}}return"#"+n.join("")});e=this._compressHexColors(e);e=e.replace(/(:|\s)(#f00)(;|})/g,"$1red$3");e=e.replace(/(:|\s)(#000080)(;|})/g,"$1navy$3");e=e.replace(/(:|\s)(#808080)(;|})/g,"$1gray$3");e=e.replace(/(:|\s)(#808000)(;|})/g,"$1olive$3");e=e.replace(/(:|\s)(#800080)(;|})/g,"$1purple$3");e=e.replace(/(:|\s)(#c0c0c0)(;|})/g,"$1silver$3");e=e.replace(/(:|\s)(#008080)(;|})/g,"$1teal$3");e=e.replace(/(:|\s)(#ffa500)(;|})/g,"$1orange$3");e=e.replace(/(:|\s)(#800000)(;|})/g,"$1maroon$3");e=e.replace(/(border|border-top|border-right|border-bottom|border-left|outline|background):none(;|\})/gi,function(m,n,i){return n.toLowerCase()+":0"+i});e=e.replace(/progid:DXImageTransform\.Microsoft\.Alpha\(Opacity=/gi,"alpha(opacity=");e=e.replace(/[^\};\{\/]+\{\}/g,"");if(g>=0){k=0;c=0;while(c<e.length){c=c+1;if(e[c-1]==="}"&&c-k>g){e=e.slice(0,c)+"
"+e.slice(c);k=c}}}e=e.replace(/;;+/g,";");for(c=0,h=a.length;c<h;c=c+1){e=e.replace("___YUICSSMIN_PRESERVED_TOKEN_"+c+"___",a[c])}e=e.replace(/^\s+|\s+$/g,"");return e}; 



also tested with yuicompressor maven plugin ...

Comment 27 Ville Skyttä 2013-06-02 17:19:09 UTC
It would be helpful if you used the same test files I've shown when showing that it works on your system -- cssmin.js doesn't seem to have anything to do with them. Neither of the files I've shown are empty BTW (one has only comments, the other comments and code) so I don't understand what the comment about an empty file means.

Comment 28 gil cattaneo 2013-06-02 17:36:26 UTC
$ echo "/* hello */" | yuicompressor --type js
isnt a js file (it is not nothing) and is empty and consequently fails

Comment 29 gil cattaneo 2013-06-02 18:54:21 UTC
(In reply to Ville Skyttä from comment #25)
> Also, this version doesn't preserve the C-style comments starting with /*!
> as documented. The old 2.4.7 version I'm using locally does.
> 
isnt this version is our rhino which dont support c-style...

Comment 30 Ville Skyttä 2013-06-15 14:47:44 UTC
It seems that this review is going to take more effort than I'm willing to commit at the moment, so I'll let someone else look into it.

Comment 31 Orion Poplawski 2013-10-19 04:29:51 UTC
Has anyone reported any of these issues upstream?

Comment 32 gil cattaneo 2013-10-19 10:09:28 UTC
yes the answer was: use an internal copy of rhino == 1.7R2

Comment 33 gil cattaneo 2013-10-19 15:03:03 UTC
see https://github.com/yui/yuicompressor/issues/31

Comment 34 Orion Poplawski 2013-10-19 19:37:49 UTC
So it sounds like yuicompressor bundles a modified version of rhino - not just that they use and old version.  Has anyone approached rhino to see if yuicopressor's changes could be integrated?  Do we know what the modifications are?

Comment 35 gil cattaneo 2013-10-19 21:03:15 UTC
(In reply to Orion Poplawski from comment #34)
> So it sounds like yuicompressor bundles a modified version of rhino - not
> just that they use and old version.  Has anyone approached rhino to see if
> yuicopressor's changes could be integrated?  

i read something about this a lot of of time ago... as a result...
yuicompressor and rhino developers dont have the same planes

Do we know what the
> modifications are?

yes have discussed privately with msrb but you can see the Debian package for an e.g.

Comment 36 Orion Poplawski 2013-10-20 01:33:21 UTC
Hmm, if rhino will not accept yuicompressor's changes, it seems you will need to file for a bundling exception.  But the FPC will want to know details of the changes and why rhino will not accept them.

Comment 37 gil cattaneo 2013-10-20 01:40:02 UTC
fine, for me is a bit complicated ...

Comment 38 gil cattaneo 2015-02-12 18:45:43 UTC
Is no longer of interest to me


Note You need to log in before you can comment on or make changes to this bug.