Bug 725733 - Review Request: rubygem-closure-compiler - Ruby Wrapper for the Google Closure Compiler
Summary: Review Request: rubygem-closure-compiler - Ruby Wrapper for the Google Closur...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 725739 1023848
Blocks: 705513
TreeView+ depends on / blocked
 
Reported: 2011-07-26 12:25 UTC by Vít Ondruch
Modified: 2015-09-02 13:32 UTC (History)
4 users (show)

Fixed In Version: rubygem-closure-compiler-1.1.11-1
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-09-02 13:31:37 UTC
Type: ---
zbyszek: fedora-review+


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Bugzilla 1090187 None None None Never

Internal Links: 1090187

Description Vít Ondruch 2011-07-26 12:25:11 UTC
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.rpm
Description: A Ruby Wrapper for the Google Closure Compiler

Comment 1 Zbigniew Jędrzejewski-Szmek 2015-05-29 18:37:13 UTC
Oh, this is rather old. If you still want this reviewed, it should be updated to depend on closure-compiler.

Comment 2 Vít Ondruch 2015-06-08 12:34:44 UTC
Well, there were always issues with bundling. Have to check this package again, since it seems it might be useful for recent rubygem-sprockets.

Comment 3 Vít Ondruch 2015-08-25 08:50:17 UTC
Spec URL: https://fedorapeople.org/cgit/vondruch/public_git/rubygem-closure-compiler.git/tree/rubygem-closure-compiler.spec?id=db1e3875060985324ffc58e90fc4d1c82090f9cd
SRPM URL: http://people.redhat.com/vondruch/rubygem-closure-compiler-1.1.11-1.fc24.src.rpm

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

Comment 4 Vít Ondruch 2015-08-25 09:00:19 UTC
Seem that similarly, nodejs-package went with closure-compiler executable ...


[1] http://pkgs.fedoraproject.org/cgit/nodejs-closure-compiler.git/tree/

Comment 5 Zbigniew Jędrzejewski-Szmek 2015-08-25 12:17:52 UTC
(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.

Comment 6 Vít Ondruch 2015-08-25 13:01:30 UTC
(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 ;)

Comment 7 Vít Ondruch 2015-09-02 13:31:37 UTC
Thank you for the review. The package is in Rawhide now.


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