Bug 588474 - Review Request: rubygem-rubyzip - zipfile support in Ruby
Summary: Review Request: rubygem-rubyzip - zipfile support in Ruby
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW buildr
TreeView+ depends on / blocked
 
Reported: 2010-05-03 19:06 UTC by Adam Young
Modified: 2010-12-26 20:30 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-12-26 20:30:10 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Comment 1 Mohammed Imran 2010-05-04 10:17:01 UTC
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-06 01:31:42 UTC
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 05:15:09 UTC
Adam,
yeah,we are using a particular sourceforge mirror,instead of downloading from random mirror.

Comment 4 Matthew Kent 2010-05-06 08:09:16 UTC
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 16:52:35 UTC
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 17:03:04 UTC
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 15:16:24 UTC
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 17:18:32 UTC
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 19:45:10 UTC
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 05:57:14 UTC
(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 18:51:25 UTC
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 18:18:21 UTC
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 14:23:25 UTC
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 17:45:52 UTC
(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 17:00:25 UTC
ping?

Comment 18 Mamoru TASAKA 2010-06-15 16:48:21 UTC
Adam, would you have some time for your review requests?

Comment 19 Mamoru TASAKA 2010-06-21 14:31:57 UTC
Jesus, would you update this bug?

Comment 20 Jesus M. Rodriguez 2010-08-20 12:52:45 UTC
sorry for the delay. I'm working on the items from comment #16

Comment 21 Jesus M. Rodriguez 2010-11-18 14:58:25 UTC
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.


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