Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: rubygem-closure-compiler - Ruby Wrapper for the Google Closure Compiler
https://bugzilla.redhat.com/show_bug.cgi?id=725733
Summary: Review Request: rubygem-closure-compiler - Ruby Wrapper for the Google Closure Compiler Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: vondruch@redhat.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, package-review@lists.fedoraproject.org Classification: Fedora Story Points: --- Type: ---
Spec URL: http://people.redhat.com/vondruch/rubygem-closure-compiler.spec SRPM URL: http://people.redhat.com/vondruch/rubygem-closure-compiler-1.1.1-1.fc16.src.... Description: A Ruby Wrapper for the Google Closure Compiler
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=725733
Vít Ondruch vondruch@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status Whiteboard| |NotReady
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=725733
Vít Ondruch vondruch@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |705513
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=725733
Marcela Mašláňová mmaslano@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |725739
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=725733
Vít Ondruch vondruch@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|725739 | Depends on| |725739
https://bugzilla.redhat.com/show_bug.cgi?id=725733
Bug 725733 depends on bug 725739, which changed state.
Bug 725739 Summary: Java dependencies for rubygems https://bugzilla.redhat.com/show_bug.cgi?id=725739
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution|--- |NOTABUG
https://bugzilla.redhat.com/show_bug.cgi?id=725733
Vít Ondruch vondruch@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugzilla.redhat.com | |/show_bug.cgi?id=1090187 Depends On| |1023848
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1023848 [Bug 1023848] Review Request: closure-compiler - JavaScript minifier and checker
https://bugzilla.redhat.com/show_bug.cgi?id=725733
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |zbyszek@in.waw.pl Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=725733
--- Comment #1 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- Oh, this is rather old. If you still want this reviewed, it should be updated to depend on closure-compiler.
https://bugzilla.redhat.com/show_bug.cgi?id=725733
--- Comment #2 from Vít Ondruch vondruch@redhat.com --- Well, there were always issues with bundling. Have to check this package again, since it seems it might be useful for recent rubygem-sprockets.
https://bugzilla.redhat.com/show_bug.cgi?id=725733
--- Comment #3 from Vít Ondruch vondruch@redhat.com --- Spec URL: https://fedorapeople.org/cgit/vondruch/public_git/rubygem-closure-compiler.g... SRPM URL: http://people.redhat.com/vondruch/rubygem-closure-compiler-1.1.11-1.fc24.src...
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=10820973
So here is the updated version. I am not sure about several things.
* How to best use the CC. - Originally, the cc.jar was included in library. I remove it and patch the library quite extensively to use %{_bindir}/closure-compiler instead. Nevertheless, the far simpler approach would be to replace the .jar file by symlink, but that would mean that the filename will not correctly reflect the actual version of CC neither the internal constant would be correct. Another option would be to stay closer to upstream and use the "java -jar" call, but that fails with "Closure::Error: no main manifest attribute, in /usr/share/java/closure-compiler/closure-compiler.jar" error and I was told that this is not supported way how to execute Java stuff on Fedora. So any opinion in this matter?
* Best way to require CC. - So far, I used BR: closure-compiler but I might change it to BR: %{_bindir}/closure-compiler, since that is the executable used in the end. - As a alrernative, I could require mvn(com.google.javascript:closure-compiler) instead, but that probably does not express clearly what the package needs
https://bugzilla.redhat.com/show_bug.cgi?id=725733
--- Comment #4 from Vít Ondruch vondruch@redhat.com --- Seem that similarly, nodejs-package went with closure-compiler executable ...
[1] http://pkgs.fedoraproject.org/cgit/nodejs-closure-compiler.git/tree/
https://bugzilla.redhat.com/show_bug.cgi?id=725733
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #5 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- (In reply to Vít Ondruch from comment #3)
- How to best use the CC. Another option would be to stay closer to upstream and use the "java -jar" call, but that fails with "Closure::Error: no main manifest attribute, in /usr/share/java/closure-compiler/closure-compiler.jar"
error and I was told that this is not supported way how to execute Java stuff on Fedora. So any opinion in this matter?
IIUC, specifying the main class in the manifest onlly works if jar is self-contained and has all the dependencies, or if the classpath is specified through some other means. But could add the manifest, but I don't think that would be useful.
- Originally, the cc.jar was included in library. I remove it and patch the library quite extensively to use %{_bindir}/closure-compiler instead. Nevertheless, the far simpler approach would be to replace the .jar file by symlink, but that would mean that the filename will not correctly reflect the actual version of CC neither the internal constant would be correct.
Dunno. I'd say that whatever works... is good enough.
- Best way to require CC.
- So far, I used BR: closure-compiler but I might change it to BR: %{_bindir}/closure-compiler, since that is the executable used in the end.
Yeah, for BR either is OK. For R using the path would be annoying because the file list would have to downloaded.
Wouldn't it be easier to use the tarball from github for everything, and ignore the gem? Hm, I see that the guidelines say that the released gem *must* be used. Too bad.
+ license is specified OK + license file is present + sources match the license + %license macro is used + package name is OK + Guidelines:Ruby are followed (I'm not an ruby expert though) + package owns all directories except for /usr/share/gems and /usr/share/gems/doc, but those are owned by rubygems + cc was unbundled + fs layout is OK + spec file is legible - subpackage -doc bundles Lato fonts and jquery... See below. + latest version is packaged + %check is present and passes + %gem_install is used + rpmlint: rubygem-closure-compiler.noarch: W: no-documentation rubygem-closure-compiler.src: W: invalid-url Source1: closure-compiler-1.1.11-tests.tgz 3 packages and 0 specfiles checked; 0 errors, 2 warnings.
The only issue seems to be the bundling of Lato and jquery. But that seems to be completely standard in rubygems. So please just add Provides: bundled(jquery). At some point we should unbundle jquery from everything, but most likely it's better to do it in some automated fashion instead of manually in every package.
Package is APPROVED.
https://bugzilla.redhat.com/show_bug.cgi?id=725733
--- Comment #6 from Vít Ondruch vondruch@redhat.com --- (In reply to Zbigniew Jędrzejewski-Szmek from comment #5)
(In reply to Vít Ondruch from comment #3)
- How to best use the CC. Another option would be to stay closer to upstream and use the "java -jar" call, but that fails with "Closure::Error: no main manifest attribute, in /usr/share/java/closure-compiler/closure-compiler.jar"
error and I was told that this is not supported way how to execute Java stuff on Fedora. So any opinion in this matter?
IIUC, specifying the main class in the manifest onlly works if jar is self-contained and has all the dependencies, or if the classpath is specified through some other means.
I'd like something like this, because that could be probably acceptable by upstream (although who knows :))
- Best way to require CC.
- So far, I used BR: closure-compiler but I might change it to BR: %{_bindir}/closure-compiler, since that is the executable used in the end.
Yeah, for BR either is OK. For R using the path would be annoying because the file list would have to downloaded.
%{_bindir} is always allowed, so I'll go with the R: %{_bindir}
Wouldn't it be easier to use the tarball from github for everything, and ignore the gem? Hm, I see that the guidelines say that the released gem *must* be used. Too bad.
Don't see any substantial benefits in GH tarballs. They have different issues ...
The only issue seems to be the bundling of Lato and jquery. But that seems to be completely standard in rubygems. So please just add Provides: bundled(jquery). At some point we should unbundle jquery from everything, but most likely it's better to do it in some automated fashion instead of manually in every package.
Well, this is pain :/ I don't think this is worth of the provide, since it would not make sense to fill bugs against every rubygem- package in case there is some issue with the bundled jQuery. Unless you fell you want to escalate this ;)
https://bugzilla.redhat.com/show_bug.cgi?id=725733
Vít Ondruch vondruch@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Fixed In Version| |rubygem-closure-compiler-1. | |1.11-1 Resolution|--- |RAWHIDE Last Closed| |2015-09-02 09:31:37
--- Comment #7 from Vít Ondruch vondruch@redhat.com --- Thank you for the review. The package is in Rawhide now.
https://bugzilla.redhat.com/show_bug.cgi?id=725733
Vít Ondruch vondruch@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Whiteboard|NotReady |
package-review@lists.fedoraproject.org