Bug 967396 - Review Request: rubygem-chosen-rails - Integrate Chosen JavaScript library with Rails asset pipeline
Review Request: rubygem-chosen-rails - Integrate Chosen JavaScript library wi...
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Vít Ondruch
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2013-05-26 23:14 EDT by Anuj More
Modified: 2016-01-04 04:01 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-01-04 04:01:19 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Anuj More 2013-05-26 23:14:22 EDT
Spec URL: http://anujmore.fedorapeople.org/pkgs/rubygem-chosen-rails/rubygem-chosen-rails.spec
SRPM URL: http://anujmore.fedorapeople.org/pkgs/rubygem-chosen-rails/rubygem-chosen-rails-0.9.14-1.fc19.src.rpm
Description: Integrate Chosen JavaScript library with Rails asset pipeline
Fedora Account System Username: anujmore
Comment 1 Anuj More 2013-05-26 23:15:43 EDT
Mock builds successfully: https://raw.github.com/execat/Packages/master/rpmspecs/rubygem-chosen-rails/mock
rpmlint complains (but barely ;-)):
rubygem-chosen-rails.noarch: W: spelling-error %description -l en_US jQuery -> j Query, query, equerry
rubygem-chosen-rails.src: W: spelling-error %description -l en_US jQuery -> j Query, query, equerry
3 packages and 0 specfiles checked; 0 errors, 2 warnings.
Comment 2 Josef Stribny 2013-05-28 09:56:52 EDT
* LICENSE should be marked as %doc
* Gemfile and Rakefile should be in the -doc sub-package rather than excluded
* README.md should be moved to the -doc sub-package as it's not require during run-time

Other than that I tested it with a simple generated app and everything seems to be fine. Please fix the issues above and I will approve.
Comment 3 Anuj More 2013-05-29 02:09:57 EDT
Hi Josef,

Thanks for reviewing the package.

I am following these guidelines (set by myself after looking at example specs and talking to vondruch): https://github.com/execat/Packages/blob/master/rpmspecs/packaging_rules

Changing these would mean changing *ALL* the specfiles I have submitted and am currently working on (There are 20 of those, so that could take some time).

About Gemfile and Rakefile, vondruch specifically asked me to remove them all unless something in the Rakefile really doesn't let package to function properly.

(Applies to https://bugzilla.redhat.com/show_bug.cgi?id=967333 as well)
Comment 4 Vít Ondruch 2013-05-29 05:52:08 EDT
(In reply to Anuj More from comment #3)
> Hi Josef,
> 
> Thanks for reviewing the package.
> 
> I am following these guidelines (set by myself after looking at example
> specs and talking to vondruch):
> https://github.com/execat/Packages/blob/master/rpmspecs/packaging_rules

cp -pr spec/ %{buildroot}%{gem_instdir}
cp -pr spec/ %{buildroot}%{gem_instdir}

I would suggest to replace the %{buildroot} with '.', i.e. wit the current directory.

Actually, I am not sure why exactly we wrote the guidelines that way, since unpacking the test suite in %check section would do the same (something like in activemodel) on one line. We will try to clarify this with FPC.

> About Gemfile and Rakefile, vondruch specifically asked me to remove them
> all unless something in the Rakefile really doesn't let package to function
> properly.

I agree with jstribny. I am not sure I would said that anytime. I may said, that some maintainer might have different opinion, but I typically try to keep everything what comes in gem packaged. If I want to remove something, then I ask upstream to do that.

[1] http://pkgs.fedoraproject.org/cgit/rubygem-activemodel.git/tree/rubygem-activemodel.spec#n66
Comment 6 Vít Ondruch 2016-01-04 04:01:19 EST
Closing this stalled review.

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