Bug 738744

Summary: Review Request: rubygem-execjs - ExecJS lets you run JavaScript code from Ruby
Product: [Fedora] Fedora Reporter: Fotios Lindiakos <fotios>
Component: Package ReviewAssignee: Vít Ondruch <vondruch>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: bkabrda, jkeck, notting, package-review, vondruch
Target Milestone: ---Flags: vondruch: fedora‑review+
limburgher: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-07-17 03:09:58 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On: 719908, 738721, 832886    
Bug Blocks: 738742, 840422    

Comment 1 Vít Ondruch 2012-01-30 07:35:13 EST
I'll review it, however I am not sponsor.
Comment 2 Vít Ondruch 2012-01-30 07:52:18 EST
* Please update the library to upstream version
  - It seems that there is available 1.3.0 version already.

* Duplicated files
  - README.md is duplicated in main package and -doc subpackage. This should
    be avoided. Instead of including whole %{geminstdir} in the main package,
    you should include just the LICENSE file there.

* Please consider excluding the cached .gem file
  - The file has no meaning in Fedora, it is just waste-load.

* Test suite
  - Although the test suite is not embedded in the gem, it is available
    upstream. It would be nice if you could run it during build time following
    the guidelines [2]

* Please consider updating the package for Ruby 1.9.3
  - Since we are in the process of rebuild for Ruby 1.9.3, it would be nice
    if you can prepare this package according to the new guidelines [1]

[1] https://fedoraproject.org/wiki/PackagingDrafts/Ruby
[2] https://fedoraproject.org/wiki/PackagingDrafts/Ruby#Test_suite_not_included_in_package
Comment 3 Fotios Lindiakos 2012-01-30 10:11:38 EST
Thanks for the feedback, this was one of my first attempts at creating an RPM from a gem. I'll update the library and in the process perform your other suggestions as well. I'll update it to 1.9.3 as well.
Comment 5 Vít Ondruch 2012-03-02 08:01:57 EST
* F17/Rawhide
  - I see that you are already using ruby(abi) = 1.9.1, i.e. you are building
    for Ruby 1.9.3 in F17/Rawhide, in that case, you should follow the new
    guidelines [1]. BTW I placed .spec file updated according to the new
    guidelines here [2].

* Tests do not run
  - There is reported "test/test_execjs.rb: cannot load such file -- multi_json"
    error. It should be fixed by adding:

      BuildRequires: rubygem(multi_json) => 1.0
      BuildRequires: rubygem(multi_json) < 2
  - After adding the BR, it fails anyway with message: "Could not find a
    JavaScript runtime. See https://github.com/sstephenson/execjs for a list
    of available runtimes." So there should be BR some supported JS engine
    (spidermonkey?) or better all engines available in Fedora.

[1] https://fedoraproject.org/wiki/PackagingDrafts/Ruby
[2] http://people.redhat.com/vondruch/rubygem-execjs.spec
Comment 6 Bohuslav "Slavek" Kabrda 2012-05-11 04:33:20 EDT
Hi Fotios, any progress on this one? If you are short on time, would you mind me taking the package and finishing it?
Comment 7 Bohuslav "Slavek" Kabrda 2012-06-13 07:42:57 EDT
Since Fotios seems to be unreachable, I'll take over and let's finish this review.
Comment 8 Bohuslav "Slavek" Kabrda 2012-06-13 09:37:11 EDT
Starting from scratch here:

SPEC: http://bkabrda.fedorapeople.org/pkgs/execjs/rubygem-execjs.spec
SRPM: http://bkabrda.fedorapeople.org/pkgs/execjs/rubygem-execjs-1.4.0-1.fc17.src.rpm

ExecJS lets you run JavaScript code from Ruby. It automatically picks the
best runtime available to evaluate your JavaScript program, then returns
the result to you as a Ruby object.

Note, that the build fails because of two tests, I am currently investigating, reported here: https://github.com/sstephenson/execjs/issues/98
Comment 9 Vít Ondruch 2012-06-14 06:09:30 EDT
Hi Slavek,

Thank you for picking this up. I have few notes:

* Would be nice to reference the deprecated JS runtime with the upstream report
  - https://github.com/sstephenson/execjs/issues/96

* Duplicated "git checkout" in Source1 comment ;)

Otherwise the package looks good. If you clarify the JS engine support and the test issues, I will happily approve the package.
Comment 10 Bohuslav "Slavek" Kabrda 2012-06-20 08:51:26 EDT
Hi Vit,
thanks for the comments.
To sum up the current state here, I have the package ready and waiting, but I first need to get a functioning and supported JavaScript runtime engine into Fedora. The only sane (from packager's point of view) is currently therubyracer [1]. For that, updated V8 will be needed. I already requested that at [2]. When that's finished and therubyracer makes it into Fedora, this review can be finished. I'll post the updated SPEC/SRPM then.

[1] https://rubygems.org/gems/therubyracer
[2] https://bugzilla.redhat.com/show_bug.cgi?id=832886
Comment 11 Vít Ondruch 2012-06-21 02:02:57 EDT
(In reply to comment #10)
Thank you for update.
Comment 13 Vít Ondruch 2012-07-16 05:46:04 EDT
Thank you. The package looks good => APPROVED
Comment 14 Bohuslav "Slavek" Kabrda 2012-07-16 06:32:38 EDT
New Package SCM Request
Package Name: rubygem-execjs
Short Description: ExecJS lets you run JavaScript code from Ruby
Owners: bkabrda
Comment 15 Gwyn Ciesla 2012-07-16 11:03:03 EDT
Git done (by process-git-requests).