Bug 588474
Summary: | Review Request: rubygem-rubyzip - zipfile support in Ruby | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Adam Young <ayoung> |
Component: | Package Review | Assignee: | 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: | rawhide | CC: | 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 20:30:10 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 201449, 588406 |
Description
Adam Young
2010-05-03 19:06:06 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 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? Adam, yeah,we are using a particular sourceforge mirror,instead of downloading from random mirror. 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. 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. Usually we use http://gems.rubyforge.org/gems/%{gemname}-%{version}.gem for sourceURL. Also the latest seems 0.9.4. Updated with License etc. http://github.com/admiyo/MySpecs/blob/5819405848299b9cc794338dafcf788668732310/rubygem-rubyzip.spec http://admiyo.fedorapeople.org/buildr-repo/rubygem-rubyzip-0.9.1-2.young.src.rpm Well, "$ gem list -r rubyzip" returns that the latest is 0.9.4. Would you check what is actually the latest? Updated to 0.9.4 http://admiyo.fedorapeople.org/buildr-repo/rubygem-rubyzip-0.9.4-1.young.src.rpm http://github.com/admiyo/MySpecs/blob/e1146aa55eeb035c092d1c6c427163811c12d3ec/rubygem-rubyzip.spec 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. 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. (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. 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. 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. 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 (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?)
ping? Adam, would you have some time for your review requests? Jesus, would you update this bug? sorry for the delay. I'm working on the items from comment #16 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. |