This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours

Bug 588474

Summary: Review Request: rubygem-rubyzip - zipfile support in Ruby
Product: [Fedora] Fedora Reporter: Adam Young <ayoung>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, imranceh, jesusr, mkent, mtasaka, notting
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-12-26 15:30:10 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 201449, 588406    

Comment 1 Mohammed Imran 2010-05-04 06:17:01 EDT
Just a note

Source0 should a URL to the upstream download link
and for sourceforge.net hosted projects Source URL have to be like

Source0: http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net
Comment 2 Adam Young 2010-05-05 21:31:42 EDT
Mohammed,  In general, yes, but we have a slew of packages that are all done as gem2rpm, and that use the gem file as the source.  This is an accepted practice according to

http://fedoraproject.org/wiki/Packaging/Ruby

So I can chenge this one to:

http://downloads.sourceforge.net/project/rubyzip/rubyzip/0.9.1/rubyzip-0.9.1.gem

OK?
Comment 3 Mohammed Imran 2010-05-06 01:15:09 EDT
Adam,
yeah,we are using a particular sourceforge mirror,instead of downloading from random mirror.
Comment 4 Matthew Kent 2010-05-06 04:09:16 EDT
Wow, lots of rubygem submissions. This applies to all of yours:

This isn't a full review but have you read

https://fedoraproject.org/wiki/Packaging/Ruby

you'll need at least the rubyabi added, see 

http://magoazul.com/wip/SPECS/rubygem-amqp.spec

for a reference (accepted yesterday).

Also give http://fedoraproject.org/wiki/PackagingGuidelines#.25global_preferred_over_.25define a look.

The ruby_sitelib macro is unnecessary for anything without a compiled extension.

Finally have you verified the License for each? Bug #588473 for example is MIT licensed.
Comment 5 Adam Young 2010-05-06 12:52:35 EDT
Mathew,
THanks.  I've read those, and I'm, going through and scrubbing now.  Got it on review from a nother package.  All of these are required to get buildr, which is the real goal.  

Mohammad, so

Source0: http://downloads.sourceforge.net/%{name}/%{name}-%{version}.gem should be OK.  Thanks.
Comment 6 Mamoru TASAKA 2010-05-06 13:03:04 EDT
Usually we use
http://gems.rubyforge.org/gems/%{gemname}-%{version}.gem
for sourceURL. Also the latest seems 0.9.4.
Comment 8 Mamoru TASAKA 2010-05-12 11:16:24 EDT
Well, "$ gem list -r rubyzip" returns that the latest is 0.9.4.
Would you check what is actually the latest?
Comment 10 Mamoru TASAKA 2010-05-12 13:18:32 EDT
Some notes

* %ruby_sitelib macro seems to be used nowhere.

* When the source code says the license is "the same as Ruby",
  it means "GPLv2 or Ruby" license tag on Fedora.

* Please write the full URL for Source0.

* BuildRoot is no longer needed on Fedora
  - rpmlint may complain, however you can ignore it
  - If you want to import this package also into EPEL, buildroot is
    needed on EPEL.

* Please properly mark some files and directories as %doc
  - %geminstdir/[A-Z]*
  - %geminstdir/install.rb (is this needed?)
  - %geminstdir/samples/
  - %geminstdir/test/

  ! Also please consider to split out these document files (and
    also rdoc and ri files) into -doc subpackge.

* Please enable %check section and execute some test program
  there.
  ! Note
    "require 'net/sftp'" line in Rakefile doesn't seem to be needed.

* Please fix rpmlint complaints
  * W: summary-not-capitalized C rubyzip is a ruby module for reading and writing zip files
    - Repeating "rubyzip is a" in Summary is redundant and should be removed.

  * Please fix lots of
    "E: non-standard-executable-perm" or "script-without-shebang" rpmlint
    complaints.
    ! For scripts
      - scripts with shebang should usually be with 0755 permission.
      - scripts without shebang should usually be with 0644 permission.

* Please update %changelog entry.
Comment 11 Adam Young 2010-05-12 15:45:10 EDT
Thanks for the review.  Very helpful.  I'll make sure to deal with the RPM lint issues for this and all of the buildr dependencies.

Going to roll this back to 0.9.1.  We need this for buildr, and 0.9.4 introduces a bug.  We'll upgrade once we fix that.

We are going to build this for EPEL as well as Fedora, so we're planning on leaving the BuildRoot logic in for now.  Once we get the word that we don't have to support RHEL, we'll drop the BuildRoot pieces. 

I'm reluctant to split the package as then the gem will calim to be installed, but won't provide the same set of files as the upstream gem.
Comment 12 Matthew Kent 2010-05-13 01:57:14 EDT
(In reply to comment #11)
> I'm reluctant to split the package as then the gem will calim to be installed,
> but won't provide the same set of files as the upstream gem.    

While this is true I think there is merit to splitting them up:

1) rdoc/ri - some of my rubygem -doc packages are 2x the size of the main page due to all the rdoc/ri generation. There are far more files as well - rubygem-merb-core has 128 files while the -doc has 2320! And I don't find much use for rdoc content on every server I manage.

2) cruft - I can't count the number of gems that I've looked at that ship Rakefiles with long lists of development dependencies, random scripts, files used for development, entire copies of the projects website, etc. Cruft that might get removed anyway if this was a standard package installation. Of course it's not being removed - just tucked away in a subpackage.

3) functionality - With splitting them up users can at least be assured that what's shipped in the primary package should function, while getting the random Rakefiles, examples, test suites and such working would require many more dependencies to package/track.

Plus I don't think it's to much to say "yum install rubygem-rubyzip*" gives you everything the gem install would.
Comment 13 Adam Young 2010-05-13 14:51:25 EDT
OK, split out into two RPMs, and all the RPM lint errors are gone, although some wanrings persist.  I'm waiting on guidance from the developer as far as how to address those.


http://github.com/admiyo/MySpecs/blob/46f558a62b20a4669dd8ee0db7c5f9f263f1958f/rubygem-rubyzip.spec

http://admiyo.fedorapeople.org/buildr-repo/rubygem-rubyzip-0.9.4-2.young.src.rpm


I've also gone back to the 0.9.4 codebase, but with a patch to make buildr work.
Comment 14 Mamoru TASAKA 2010-05-15 14:18:21 EDT
Well, for -2:

* License
> * When the source code says the license is "the same as Ruby",
>   it means "GPLv2 or Ruby" license tag on Fedora.

* SourceURL
  - "Source0: " is written twice for unknown reason.

* rpmlint issue
  - There are still some complaints about "non-executable-script"
    and "non-standard-executable-perm".
----------------------------------------------------------------
rubygem-rubyzip-doc.noarch: E: non-executable-script /usr/lib/ruby/gems/1.8/gems/rubyzip-0.9.4/test/data/file2.txt 0664L /usr/bin/env
rubygem-rubyzip-doc.noarch: E: non-standard-executable-perm /usr/lib/ruby/gems/1.8/gems/rubyzip-0.9.4/test/gentestfiles.rb 0775L
rubygem-rubyzip-doc.noarch: E: non-standard-executable-perm /usr/lib/ruby/gems/1.8/gems/rubyzip-0.9.4/test/ziprequiretest.rb 0775L
rubygem-rubyzip-doc.noarch: E: non-standard-executable-perm /usr/lib/ruby/gems/1.8/gems/rubyzip-0.9.4/test/stdrubyexttest.rb 0775L
rubygem-rubyzip-doc.noarch: E: non-standard-executable-perm /usr/lib/ruby/gems/1.8/gems/rubyzip-0.9.4/test/alltests.rb 0775L
rubygem-rubyzip-doc.noarch: E: non-standard-executable-perm /usr/lib/ruby/gems/1.8/gems/rubyzip-0.9.4/test/ioextrastest.rb 0775L
rubygem-rubyzip-doc.noarch: E: non-standard-executable-perm /usr/lib/ruby/gems/1.8/gems/rubyzip-0.9.4/test/data/notzippedruby.rb 0775L
----------------------------------------------------------------
    - 0775 is unusual, which should be 0755.
    - scripts with shebags should have 0755 permission.

    Please change the permission of these scripts in %install
    explicitly.

* Enabling test
> * Please enable %check section and execute some test program
>   there.
>   ! Note
>     "require 'net/sftp'" line in Rakefile doesn't seem to be needed.
Comment 15 Adam Young 2010-05-17 10:23:25 EDT
Much cleaned up from previous reviews:  


http://github.com/admiyo/MySpecs/blob/c1f91849426aed4747472ebe7291e2209293c786/rubygem-rubyzip.spec

http://admiyo.fedorapeople.org/buildr-repo/rubygem-rubyzip-0.9.4-2.young.src.rpm


I went back to the 0.9.4 version.  It was the latest, it had a lot cleaned up from 0.9.1, and the issue that caused the breakage could be handled by a patch to buildr.

The lint errors are fixed.  The lint warnings about template strings in the installed files are still there, but that is an artifact of the ruby info build, which has not yet been addressed by the developers.

License is back to GPL V2 or Ruby.
Permissions are fixed
Comment 16 Mamoru TASAKA 2010-05-17 13:45:52 EDT
(From next time, please change the release number when you modify your
 spec file to avoid confusion).

For (second) -2:
* rpmlint
  - non-standard-executable-perm rpmlint errors still exist
-------------------------------------------------------------------------
rubygem-rubyzip-doc.noarch: E: non-executable-script /usr/lib/ruby/gems/1.8/gems/rubyzip-0.9.4/test/data/file2.txt 0664L /usr/bin/env
rubygem-rubyzip-doc.noarch: E: non-standard-executable-perm /usr/lib/ruby/gems/1.8/gems/rubyzip-0.9.4/test/gentestfiles.rb 0775L
rubygem-rubyzip-doc.noarch: E: non-standard-executable-perm /usr/lib/ruby/gems/1.8/gems/rubyzip-0.9.4/test/ziprequiretest.rb 0775L
rubygem-rubyzip-doc.noarch: E: non-standard-executable-perm /usr/lib/ruby/gems/1.8/gems/rubyzip-0.9.4/test/stdrubyexttest.rb 0775L
rubygem-rubyzip-doc.noarch: E: non-standard-executable-perm /usr/lib/ruby/gems/1.8/gems/rubyzip-0.9.4/test/alltests.rb 0775L
rubygem-rubyzip-doc.noarch: E: non-standard-executable-perm /usr/lib/ruby/gems/1.8/gems/rubyzip-0.9.4/test/ioextrastest.rb 0775L
rubygem-rubyzip-doc.noarch: E: non-standard-executable-perm /usr/lib/ruby/gems/1.8/gems/rubyzip-0.9.4/test/data/notzippedruby.rb 0775L
-------------------------------------------------------------------------
   Again, permissions should usually be 0644 or 0755.

* license tag
  - is still "GPLv2+ or Ruby".

* Consistent macro usage
  - Please use %geminstdir when possible (please check %check section)

* Test program
  - Please enable test program (also please check my previous comment)
>  ! Note
>    "require 'net/sftp'" line in Rakefile doesn't seem to be needed.

* Document files
  - I guess sample/ directory should be marked as %doc (and are there
    any reason you want to put sample/ file in main binary rpm?)
Comment 17 Mamoru TASAKA 2010-06-06 13:00:25 EDT
ping?
Comment 18 Mamoru TASAKA 2010-06-15 12:48:21 EDT
Adam, would you have some time for your review requests?
Comment 19 Mamoru TASAKA 2010-06-21 10:31:57 EDT
Jesus, would you update this bug?
Comment 20 Jesus M. Rodriguez 2010-08-20 08:52:45 EDT
sorry for the delay. I'm working on the items from comment #16
Comment 21 Jesus M. Rodriguez 2010-11-18 09:58:25 EST
I will be taking this over but I have to downgrade the version to 0.9.1 as 0.9.4 has some issues with it that cause buildr pain. And buildr is the real reason we need this packaged. I'll post an updated spec and srpm.