This service will be undergoing maintenance at 00:00 UTC, 2017-10-23 It is expected to last about 30 minutes

Bug 738465

Summary: Review Request: rubygem-barista - Simple, integrated support for CoffeeScript in Rack and Rails applications
Product: [Fedora] Fedora Reporter: Fotios Lindiakos <fotios>
Component: Package ReviewAssignee: Vít Ondruch <vondruch>
Status: CLOSED DEFERRED QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: bkabrda, jkeck, msuchy, package-review, rpms, ved.harish3, vondruch
Target Milestone: ---Flags: vondruch: fedora‑review?
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2015-07-21 09:16:16 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On: 738742    
Bug Blocks: 201449    

Description Fotios Lindiakos 2011-09-14 17:20:13 EDT
Spec URL: https://github.com/fotioslindiakos/rubygem-barista/raw/master/SPECS/rubygem-barista.spec

SRPM URL: https://github.com/fotioslindiakos/rubygem-barista/raw/master/SRPMS/rubygem-barista-1.2.1-1.fc15.src.rpm

Description: This gem allows for automated building of Coffeescripts in a rack environment. More information is available here: https://github.com/Sutto/barista. I used gem2rpm and then filled out missing license, contact, and files information.

This is my first package, please be gentle.
Comment 1 Fotios Lindiakos 2011-09-14 17:49:19 EDT
Apologies, but I renamed my Github repo, so these are the new links:

Spec URL: https://raw.github.com/fotioslindiakos/rubygem-all/master/SPECS/rubygem-barista.spec

SRPM URL: https://raw.github.com/fotioslindiakos/rubygem-all/raw/master/SRPMS/rubygem-barista-1.2.1-1.fc15.src.rpm
Comment 2 Jonas Courteau 2012-05-28 00:11:50 EDT
Hi!  I can only provide an unofficial review, but here's a few comments:

I'd recommend going over the spec file and bringing it more in line with the guidelines here: http://fedoraproject.org/wiki/Packaging:Ruby. Specifically, you'll want to have a BuildRequires against rubygems-devel, and use the provided macros (gem_dir,gem_instdir, and so on). There's also new guidelines in there for the %prep/%build/%install sections.

As suggested in those guidelines, you may want to add a %check section.  It looks like the gem includes its tests, so:
%check
pushd .%{gem_instdir}
rspec -Ilib spec/
rm -rf spec/
popd
should work. (it's also recommended that you don't package the tests unless you need to)

Also, to package on fedora 17 or rawhide, the ruby(abi) requirement needs to be updated to 1.9.1

There's also a bunch of whitespace in front of the changelog entry which should be removed.
---

Hopefully that helps!  Once these things are resolved, I'll run through the full review checklist as well.

If anyone is wanting to do other Rubygem-based package reviews, take a look at some of mine - see https://bugzilla.redhat.com/show_bug.cgi?id=823352 and its dependencies.  I still need a sponsor too.
Comment 3 Bohuslav "Slavek" Kabrda 2012-08-24 01:49:24 EDT
It seems that Fotios has been unresponsive for quite some time now. If anyone needs this gem, please fix the spec and srpm (see Comment 2) and take it over, I will review it if needed.
Comment 4 Harish Ved 2013-03-19 13:31:56 EDT
Updated SPEC URL : https://github.com/vedharish/rubygem-gems/blob/master/SPECS/rubygem-barista.spec

Updated SRPM URL : https://github.com/vedharish/rubygem-gems/blob/master/SRPMS/rubygem-barista-1.3.0-1.fc18.src.rpm

The tests under spec/ folder are not packaged.
This is my first package too, so it might need extensive reviewing.
Comment 5 Vít Ondruch 2013-03-20 03:28:20 EDT
Hi Harish,

(In reply to comment #4)
> The tests under spec/ folder are not packaged.

Please consider to run the test suite. You can find help in Ruby guidelines [1].

Also, please fix the gem to be compatible with Rawhide guidelines. You can use gem2rpm (from RubyGems.org, the packaged gem2rpm is a bit older version :/) or this [2] migration script.

Thank you.


[1] https://fedoraproject.org/wiki/Packaging:Ruby#Test_suites_not_included_in_the_package
[2] https://github.com/voxik/fermig/blob/master/f19.rb
Comment 6 Harish Ved 2013-04-21 02:25:20 EDT
The links have again been updated for fedora 19 and the tests are run.

Updated SPEC URL : https://github.com/vedharish/rubygem-gems/blob/master/SPECS/rubygem-barista.spec

Updated SRPM URL : https://github.com/vedharish/rubygem-gems/blob/master/SRPMS/rubygem-barista-1.3.0-1.fc19.src.rpm

Please review.
Comment 7 Vít Ondruch 2013-04-22 03:24:00 EDT
(In reply to comment #6)
It does not look like f19 .spec file. Could you please use recent gem2rpm and possibly with "-t fedora-19-rawhide" option, which will use the appropriate template.

BTW I would suggest to unpack the test suite in %check section. In that case, you will not need to include it in resulting package.
Comment 8 Harish Ved 2013-04-27 18:18:16 EDT
The links have again been updated.
Please review.

Updated SPEC URL : https://github.com/vedharish/rubygem-gems/blob/master/SPECS/rubygem-barista.spec

Updated SRPM URL : https://github.com/vedharish/rubygem-gems/blob/master/SRPMS/rubygem-barista-1.3.0-1.fc19.src.rpm
Comment 9 Vít Ondruch 2013-04-30 08:13:55 EDT
Thanks for the update.

* LICENSE file must be included in the main package.
  - See [1] for more information

* Include test suite as a SOURCE
  - I am afraid, that build of your package fails on Koji. You cannot
    donwload/checkout anything from the internet. Please follow the guidelines,
    which gives example how to properly include test suite [2]

* Run the test suite from .%{gem_instdir}
  - If you run the test suite from .%{gem_instdir}, you will save some hassle
    with moving and deleting the test suite.

* Move into -doc subpackage non-essential files

  %{gem_instdir}/DESCRIPTION
  %{gem_instdir}/Gemfile
  %{gem_instdir}/Gemfile.lock
  %{gem_instdir}/Rakefile
  %{gem_instdir}/barista.gemspec

  - The files above should do to -doc subpackage, since they are not needed for
    runtime.

* Exclude the hidden files

  %{gem_instdir}/.document
  %{gem_instdir}/.rspec
  %{gem_instdir}/.rvmrc.example

  - The above files should be exclude (I guess rpmlint would warn about them).
    I suggest to use "%exclude %{gem_instdir}/.*" to exclude them all in once.

* The package is not buildable
  - Apparently, you are not BR: git, which you are using later. But since you
    are going to redo the %check section it should not be issue, just something
    to be aware.
  - In the future, please use mock [3] for local testing or Koji [4] (which is
    for bonus points ;)


[1] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text
[2] https://fedoraproject.org/wiki/Packaging:Ruby#Test_suites_not_included_in_the_package
[3] https://fedoraproject.org/wiki/Using_Mock_to_test_package_builds
[4] https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Create_Your_Review_Request
Comment 10 Harish Ved 2013-06-27 10:13:20 EDT
Spec URL: https://github.com/vedharish/rubygem-gems/raw/master/SPECS/rubygem-barista.spec

SRPM URL: https://github.com/vedharish/rubygem-gems/raw/master/SRPMS/rubygem-barista-1.3.0-1.fc20.src.rpm

Koji build report: https://koji.fedoraproject.org/koji/taskinfo?taskID=5550470

Description: This gem allows for automated building of Coffeescripts in a rack environment. More information is available here: https://github.com/Sutto/barista.

Fedora Account System Username: something
Comment 11 Vít Ondruch 2013-07-30 07:48:28 EDT
* Upstream source differs from the one in SRPM
  - It seems that the source .gem you have in your SRPM does not match the
    upstream on. Could you please fix this issue?

* LICENSE file must be included in the main package.
  - This does not seems to be resolved.
  - See [1] for more information


[1] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text
Comment 13 Miroslav Suchý 2015-07-21 09:16:16 EDT
Closing due long inactivity. And based on #12 it does not seem this review will continue.