Bug 711893 - Review Request: rubygem-dnsruby - Ruby DNS(SEC) implementation
Summary: Review Request: rubygem-dnsruby - Ruby DNS(SEC) implementation
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Gwyn Ciesla
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-06-08 21:59 UTC by Paul Wouters
Modified: 2011-11-25 02:12 UTC (History)
4 users (show)

Fixed In Version: rubygem-dnsruby-1.52-3.fc15
Clone Of:
Environment:
Last Closed: 2011-11-25 02:07:16 UTC
Type: ---
Embargoed:
gwync: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Paul Wouters 2011-06-08 21:59:49 UTC
Spec URL: ftp://ftp.xelerance.com/opendnssec/rubygem-dnsruby.spec
SRPM URL: ftp://ftp.xelerance.com/opendnssec/rubygem-dnsruby-1.52-1.fc14.src.rpm
Description: Ruby DNS(SEC) implementation

Note there are a bunch of warnings about macros in the doc files that I'm still checking out from rpmlint.

rubygem-dnsruby.noarch: W: unexpanded-macro /usr/lib/ruby/gems/1.8/doc/dnsruby-1.52/ri/Dnsruby/Dnssec/no_keys%3f-c.yaml %3f

Othen then that, rpmlint is quiet.

This package is a requirement for opendnssec

Comment 1 Gwyn Ciesla 2011-07-18 14:37:41 UTC
Good:

- rpmlint checks return:

Just the macro warnings.  Any update on this?

- package meets naming guidelines
- package meets packaging guidelines
- license ( GPLv2+ or Ruby ) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file 

Otherwise it looks good, running a Mock build to double-check BRs.

Comment 2 Gwyn Ciesla 2011-07-18 14:41:33 UTC
BRs are good, so it's just macros.

Comment 3 Vít Ondruch 2011-07-19 11:43:24 UTC
According to [1] and other two files I randomly checked, I would say that the license should be ASL 2.0

It would be nice to execute the test suite in %check section.

[1] http://dnsruby.rubyforge.org/svn/trunk/lib/dnsruby.rb

Comment 4 Paul Wouters 2011-09-27 00:35:45 UTC
- The man page issues are still there, upstream did not get back to me. I'll have to look at it myself

- I am not sure how I could run the test suite, as the "build" target is empty, and we only have something outside of the .gem file after the install phase. I believe the check phase runs before install?

Comment 5 Gwyn Ciesla 2011-09-27 01:13:46 UTC
Does the test not take a path?  If not, just fix the macros and license.

Comment 6 Vít Ondruch 2011-09-27 07:28:57 UTC
(In reply to comment #4)
> - The man page issues are still there, upstream did not get back to me. I'll
> have to look at it myself
> 
> - I am not sure how I could run the test suite, as the "build" target is empty,
> and we only have something outside of the .gem file after the install phase. I
> believe the check phase runs before install?

No, the %check is executed after %install section, so you happily execute the test suite there. Also, for newer packages, I suggest to install the gem in %prep section and move it later to the final position in %install section. You can take a look at [1] for example.

Also note, that you can use latest rubygem-gem2rpm to generate the spec file which reflects this suggestion.

[1] http://pkgs.fedoraproject.org/gitweb/?p=rubygem-bson.git;a=blob;f=rubygem-bson.spec;h=a39e38d8c6dc91562e0043a559b4a91a091e29ac;hb=HEAD

Comment 7 Paul Wouters 2011-10-04 16:05:33 UTC
Thanks for the comments.
I upgraded using the latest rubygem-gem2rpm, though I needed to make a few changes.

- Fixed licence tag to ASL 2.0
- I left the %check commented because it requires network connectivity, contains errors and missing check targets, and actually hangs.
- I install using --no-ri to avoid the above mentioned file name issues
- I added this to the %files (do these belong in a rubygem package?)
  %{geminstdir}/test
  %{geminstdir}/demo
  %{geminstdir}/Rakefile
- Added %doc %{geminstdir}/README to base package (as well as doc package)
- convert two DOS format files (README and DNSSEC) to unix

new rpmlint output:

3 packages and 0 specfiles checked; 0 errors, 0 warnings.

Comment 8 Paul Wouters 2011-10-04 16:07:17 UTC
oops. and the new spec file and packages:

Spec URL: ftp://ftp.xelerance.com/opendnssec/rubygem-dnsruby.spec
SRPM URL:
ftp://ftp.xelerance.com/opendnssec/rubygem-dnsruby-1.52-2.fc14.src.rpm

Comment 9 Gwyn Ciesla 2011-10-04 16:52:48 UTC
Looks good, but in the future it would be good to preserve the old changelog entries.

APPROVED.

Comment 10 Vít Ondruch 2011-10-05 06:01:41 UTC
(In reply to comment #7)
> - I left the %check commented because it requires network connectivity,
> contains errors and missing check targets, and actually hangs.

I agree that test suite could be left disabled, when it requires some network traffic, but if you are testing locally and it hangs, then it is bad sign and should be better evaluated. You are preventing problems not just yourself but also to your users.

> - I install using --no-ri to avoid the above mentioned file name issues

Please no, don't do that. This is how RI works. Just ignore the output, that's it. Rpmlint provides just hints, rpmlints with 0 warnings is not must.


> - I added this to the %files (do these belong in a rubygem package?)
>   %{geminstdir}/test
>   %{geminstdir}/demo
>   %{geminstdir}/Rakefile

These should be typically contained in -doc subpackage.

> - Added %doc %{geminstdir}/README to base package (as well as doc package)

There is no need to include the README into both packages, since the -doc subpackage has explicit require for base package.

Comment 11 Paul Wouters 2011-10-05 21:03:56 UTC
I'll move the test/demo to the doc package, remove the readme from the docs package, and enable the RI.

Thanks for reviewing!

Comment 12 Gwyn Ciesla 2011-10-06 00:50:33 UTC
No SCM request?

Comment 13 Gwyn Ciesla 2011-10-07 17:27:23 UTC
Still no SCM request.

https://fedoraproject.org/wiki/Package_SCM_admin_requests

Comment 14 Paul Wouters 2011-10-11 22:13:15 UTC
I thought i had set it before, but it got eaten? I've set it again.

Comment 15 Gwyn Ciesla 2011-10-12 01:19:38 UTC
Include an SCM request when setting the fedora=cvs flag, please.  Thanks!

Comment 16 Vít Ondruch 2011-10-12 07:36:31 UTC
(In reply to comment #14)
> I thought i had set it before, but it got eaten? I've set it again.

Paul, if you are setting the CVS flag, then you have to also provide more specifications about repository and branches you request. Jon already provided a link above where you can read more. Specifically, you should read the section [1]. You can take also look on other reviews [2] how does it look in reality.


[1] https://fedoraproject.org/wiki/Package_SCM_admin_requests#New_Packages
[2] https://bugzilla.redhat.com/show_bug.cgi?id=742935

Comment 17 Paul Wouters 2011-10-13 22:26:23 UTC
ooh oops. i totally forgot about that....

New Package SCM Request
=======================
Package Name: rubygem-dnsruby
Short Description: Ruby DNS(SEC) implementation
Owners: pwouters
Branches: f15 f16 el5 el6
InitialCC: pwouters

Comment 18 Gwyn Ciesla 2011-10-14 12:12:23 UTC
Git done (by process-git-requests).

Comment 19 Fedora Update System 2011-10-17 16:17:22 UTC
rubygem-dnsruby-1.52-3.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/rubygem-dnsruby-1.52-3.fc15

Comment 20 Fedora Update System 2011-10-17 16:18:08 UTC
rubygem-dnsruby-1.52-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/rubygem-dnsruby-1.52-3.fc16

Comment 21 Fedora Update System 2011-10-17 16:54:07 UTC
rubygem-dnsruby-1.52-3.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/rubygem-dnsruby-1.52-3.el5

Comment 22 Fedora Update System 2011-10-18 07:17:45 UTC
rubygem-dnsruby-1.52-3.fc16 has been pushed to the Fedora 16 testing repository.

Comment 23 Fedora Update System 2011-11-25 02:07:16 UTC
rubygem-dnsruby-1.52-3.fc16 has been pushed to the Fedora 16 stable repository.

Comment 24 Fedora Update System 2011-11-25 02:08:48 UTC
rubygem-dnsruby-1.52-3.el5 has been pushed to the Fedora EPEL 5 stable repository.

Comment 25 Fedora Update System 2011-11-25 02:12:48 UTC
rubygem-dnsruby-1.52-3.fc15 has been pushed to the Fedora 15 stable repository.


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