* 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)
[...]

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

* 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?

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)

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...

Sure. I'll look for a fix in the meantime

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

Spec URL: http://gil.fedorapeople.org/yuicompressor.spec
SRPM URL:
http://gil.fedorapeople.org/yuicompressor-2.4.8-0.2.sha6e2bc23.fc16.src.rpm

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

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...

gil: ping?

Ville: pong ?

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)?

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

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.

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.

http://www.yuiblog.com/blog/2012/10/16/state-of-yui-compressor/

Any news here?

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

Oh...

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?

> 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

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

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.

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

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

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;

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 ...

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.

$ echo "/* hello */" | yuicompressor --type js
isnt a js file (it is not nothing) and is empty and consequently fails

(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...

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.

Has anyone reported any of these issues upstream?

yes the answer was: use an internal copy of rhino == 1.7R2

see https://github.com/yui/yuicompressor/issues/31

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?

(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.

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.

fine, for me is a bit complicated ...

Is no longer of interest to me