Bug 798248 - Review Request: rubygem-ruby-dbus - Ruby module for interaction with D-Bus
Summary: Review Request: rubygem-ruby-dbus - Ruby module for interaction with D-Bus
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Vít Ondruch
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-02-28 12:13 UTC by Bohuslav "Slavek" Kabrda
Modified: 2012-03-16 21:20 UTC (History)
3 users (show)

Fixed In Version: rubygem-ruby-dbus-0.7.0-2.fc17
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-03-16 21:20:00 UTC
Type: ---
Embargoed:
vondruch: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Bohuslav "Slavek" Kabrda 2012-02-28 12:13:51 UTC
Spec URL: http://bkabrda.fedorapeople.org/pkgs/ruby-dbus/rubygem-ruby-dbus.spec
SRPM URL: http://bkabrda.fedorapeople.org/pkgs/ruby-dbus/rubygem-ruby-dbus-0.7.0-1.fc17.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=3827402
Description: Ruby D-Bus provides an implementation of the D-Bus protocol such that the D-Bus system can be used in the Ruby programming language.


Please note, that this package will obsolete ruby-dbus.

Comment 1 Vít Ondruch 2012-02-28 12:19:23 UTC
I'll take this for a review.

Comment 2 Vít Ondruch 2012-02-28 14:22:27 UTC
* Test suite execution
  - The test command you are using is somehow cryptic as well as the result is
    not comprehensive. Is this really the best approach? Wouldn't be better to
    fallback to rake in this particular case?

* Bundled libraries
  - the lib/dbus/core_ext directory contains files copied from active support.
    Not sure if we should replace them by dependency on AS. If not, the license
    of the package should be adjusted at least. The best would be if upstream
    will consider to remove this bundled files, but I can imagine it will
    be hard :/

* Rename
  - The package should obsolete and provide "oldpackagename", however you are
    obsoleting and providing virtual provide. Is that intentional?

Please fix the above mentioned issues before the package could be approved. Thank you.

Comment 3 Bohuslav "Slavek" Kabrda 2012-02-28 14:29:43 UTC
(In reply to comment #2)
> * Test suite execution
>   - The test command you are using is somehow cryptic as well as the result is
>     not comprehensive. Is this really the best approach? Wouldn't be better to
>     fallback to rake in this particular case?

I found out an easier way:
RUBYOPT="-I../lib" ./test_env testrb *_test.rb t[0-9]*.rb
I think that this is quite straightforward and I would really not like to get to rake. Would this be ok for you?

> * Bundled libraries
>   - the lib/dbus/core_ext directory contains files copied from active support.
>     Not sure if we should replace them by dependency on AS. If not, the license
>     of the package should be adjusted at least. The best would be if upstream
>     will consider to remove this bundled files, but I can imagine it will
>     be hard :/
> 

I will investigate and try to discuss it with upstream.

> * Rename
>   - The package should obsolete and provide "oldpackagename", however you are
>     obsoleting and providing virtual provide. Is that intentional?
> 

Hmm, I am not sure and the guidelines [1] don't say anything about virtual provides. Maybe both package name and its virtual provides should be obsoleted?

> Please fix the above mentioned issues before the package could be approved.
> Thank you.

When I solve points 2 and 3, I'll post new SPEC and SRPM.

[1] http://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages

Comment 4 Bohuslav "Slavek" Kabrda 2012-03-09 06:53:53 UTC
- So, I have created a patch that unbundles the libraries and upstream has accepted it [1], so this solves the bundling issue.
- As for the provides/obsoletes of virtual provides, I have discussed with two proven packagers and they agreed, that the proper way is to obsolete and provide both the package name and its virtual provide (which was ruby(dbus) = %{version}). So the proper tags for this package should be (already in the updated spec):

Provides: ruby(dbus) = %{version}
Provides: ruby-dbus = %{version}-%{release}
Obsoletes: ruby(dbus) < 0.3.0
Obsoletes: ruby-dbus < 0.3.0-5

SPEC: http://bkabrda.fedorapeople.org/pkgs/ruby-dbus/rubygem-ruby-dbus.spec
SRPM: http://bkabrda.fedorapeople.org/pkgs/ruby-dbus/rubygem-ruby-dbus-0.7.0-2.fc17.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=3871637


[1] https://github.com/mvidner/ruby-dbus/pull/18

Comment 5 Vít Ondruch 2012-03-09 10:46:43 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > * Test suite execution
> >   - The test command you are using is somehow cryptic as well as the result is
> >     not comprehensive. Is this really the best approach? Wouldn't be better to
> >     fallback to rake in this particular case?
> 
> I found out an easier way:
> RUBYOPT="-I../lib" ./test_env testrb *_test.rb t[0-9]*.rb
> I think that this is quite straightforward and I would really not like to get
> to rake. Would this be ok for you?

Nice!

(In reply to comment #4)
> - So, I have created a patch that unbundles the libraries and upstream has
> accepted it [1], so this solves the bundling issue.

Sweet!

> - As for the provides/obsoletes of virtual provides, I have discussed with two
> proven packagers and they agreed, that the proper way is to obsolete and
> provide both the package name and its virtual provide (which was ruby(dbus) =
> %{version}). So the proper tags for this package should be (already in the
> updated spec):
> 
> Provides: ruby(dbus) = %{version}
> Provides: ruby-dbus = %{version}-%{release}
> Obsoletes: ruby(dbus) < 0.3.0
> Obsoletes: ruby-dbus < 0.3.0-5

There should be used "Obsoletes: ruby(dbus) <= 0.3.0", since you want to obsolete ruby(dbus) includeing 0.3.0 version. The ruby-dbus obsolete should be fine, since the version is higher that any version which was released.

* %{gem_instdir}/doc should be marked as %doc, also the examples might be considered as a doc? I leave it up to you.

Otherwise the package looks good => APPROVED.

Please fix the two minor issues mentioned above prior committing and don't forget to retire the ruby-dbus package properly.

Comment 6 Bohuslav "Slavek" Kabrda 2012-03-09 10:53:43 UTC
(In reply to comment #5)
> > - As for the provides/obsoletes of virtual provides, I have discussed with two
> > proven packagers and they agreed, that the proper way is to obsolete and
> > provide both the package name and its virtual provide (which was ruby(dbus) =
> > %{version}). So the proper tags for this package should be (already in the
> > updated spec):
> > 
> > Provides: ruby(dbus) = %{version}
> > Provides: ruby-dbus = %{version}-%{release}
> > Obsoletes: ruby(dbus) < 0.3.0
> > Obsoletes: ruby-dbus < 0.3.0-5
> 
> There should be used "Obsoletes: ruby(dbus) <= 0.3.0", since you want to
> obsolete ruby(dbus) includeing 0.3.0 version. The ruby-dbus obsolete should be
> fine, since the version is higher that any version which was released.
> 

You are absolutely right, my mistake.

> * %{gem_instdir}/doc should be marked as %doc, also the examples might be
> considered as a doc? I leave it up to you.

Good point. Will do.

> 
> Otherwise the package looks good => APPROVED.
> 
> Please fix the two minor issues mentioned above prior committing and don't
> forget to retire the ruby-dbus package properly.

Thank you for your review!

Comment 7 Bohuslav "Slavek" Kabrda 2012-03-09 10:54:48 UTC
New Package SCM Request
=======================
Package Name: rubygem-ruby-dbus
Short Description: Ruby module for interaction with D-Bus
Owners: bkabrda
Branches: f17
InitialCC:

Comment 8 Gwyn Ciesla 2012-03-09 13:45:06 UTC
Git done (by process-git-requests).

Comment 9 Fedora Update System 2012-03-09 14:02:15 UTC
rubygem-ruby-dbus-0.7.0-2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/rubygem-ruby-dbus-0.7.0-2.fc17

Comment 10 Fedora Update System 2012-03-10 16:15:01 UTC
rubygem-ruby-dbus-0.7.0-2.fc17 has been pushed to the Fedora 17 testing repository.

Comment 11 Bohuslav "Slavek" Kabrda 2012-03-14 08:01:13 UTC
Package Change Request
======================
Package Name: rubygem-ruby-dbus
New Branches: f16
Owners: bkabrda

Comment 12 Gwyn Ciesla 2012-03-14 12:27:20 UTC
Git done (by process-git-requests).

Comment 13 Fedora Update System 2012-03-16 21:20:00 UTC
rubygem-ruby-dbus-0.7.0-2.fc17 has been pushed to the Fedora 17 stable repository.


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