Bug 728088 - Review Request: rubygem-idn - Ruby Bindings for the GNU LibIDN library
Summary: Review Request: rubygem-idn - Ruby Bindings for the GNU LibIDN library
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Mo Morsi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-08-04 03:42 UTC by Shawn Starr
Modified: 2014-08-06 18:46 UTC (History)
6 users (show)

Fixed In Version: rubygem-idn-0.0.2-7.fc16
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-01-29 19:48:06 UTC
jlaska: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Shawn Starr 2011-08-04 03:42:04 UTC
Part of Dependency packaging for OpenNebula 2.x/3.x

Spec URL:
http://www.sh0n.net/spstarr/fedora/rubygem-idn/rubygem-idn.spec
SRPM URL:
http://www.sh0n.net/spstarr/fedora/rubygem-idn/rubygem-idn-0.0.2-1.fc17.src.rpm

Description: Ruby Bindings for the GNU LibIDN library

MOCK: PASS

Comment 1 Mo Morsi 2011-10-07 00:04:13 UTC
Taking this one 

* rpmlint looks good

* koji build is green:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3407485

* The license refers to ASL and MIT but I was only able to find ASL in the project, which components are licensed under MIT? The files you pointed out are all licensed under the ASL 

http://www.apache.org/licenses/LICENSE-2.0

* Also the NOTICE file states that 

"This software contains code derived from the Net::XMPP for Ruby project by Yuki Mitsui (http://netxmpp-ruby.jabberstudio.org/)"

This exact url does not seem to be accessible but I found the project's website which indicates that it is licensed under the LGPL:

http://jabberstudio.org/projects/netxmpp/project/view.php

I'm not sure what they mean their code is "derived" from net-xmpp, but it might be good to clarify (I believe licensing the project under ASL v2 AND LGPL would be acceptable)

* rm -rf %{buildroot} is no longer needed at the beginning of the %install section

* since you are packaging this gem for non-gem use, that non-gem subpackage needs to own the ruby_sitelib symlinks according to these guidelines:

"- All the toplevel library files of the Gem must be symlinked into ruby_sitelib.
 - The subpackage must own these symbolic links."

Other than these, package looks good

Comment 2 Shawn Starr 2011-10-07 00:30:59 UTC
I will fix however, Ruby is going to 1.9.3 in Fedora 17 which is going to break a lot of things (compatibility?). I will fix these issues. They also have dropped the non-gem use for rubygems (it's not updated in packaging guidelines!).

Thanks for reviewing further updates to this bug pending.

Comment 3 Mo Morsi 2011-10-07 10:47:01 UTC
Sounds good. Good to know that the packaging rubygems for non-gem use guideline is going away, feel free to remove the non-gem subpackage from this altogether if you want (this was the first package that I've seen do this in a very long time).

Also how definite is it that Ruby 1.9.3 is going into Fedora 17?

Thank you.

Comment 4 Shawn Starr 2011-10-07 19:54:20 UTC
The Ruby SIG team would need to be engaged they have a mailing list. Their changes will cause changes in these packages.

Comment 5 Vít Ondruch 2011-10-10 07:56:16 UTC
(In reply to comment #2)
> They also have dropped the non-gem use for rubygems (it's not updated in
> packaging guidelines!).

I proposed this change, since the non-gem use is bad designed and I can't see any benefit of such packages. However, there was not much done WRT update of packaging guidelines, but I'll try to collect some ideas in [1]. Going to add this proposal right now.


(In reply to comment #3)
> Sounds good. Good to know that the packaging rubygems for non-gem use guideline
> is going away, feel free to remove the non-gem subpackage from this altogether
> if you want (this was the first package that I've seen do this in a very long
> time).
> 
> Also how definite is it that Ruby 1.9.3 is going into Fedora 17?
> 
> Thank you.

I am highly interested in R 1.9.3 in F17 and I did not heard any strong objections, so it is highly probable that I will push them forward. Of course, you can check Ruby-SIG ML as was mentioned by Shawn.


[1] https://fedoraproject.org/wiki/Packaging_talk:Ruby

Comment 6 Mo Morsi 2011-10-26 18:18:46 UTC
ping? could you fix the issues described in my review. rubygem-addressable needs this to work, which in return is required by rubygem-webmock, which in return is required by aeolus-conductor.

Thank you

Comment 7 Shawn Starr 2011-10-27 03:54:31 UTC
Yes, I've just been swamped with RL stuff, I hope to look at this and the other this weekend. These are pre-req's for OpenNebula 3.x also which will depend on how Ruby SIG will have for backwards compatibility or that will kill this.

Comment 8 James Laska 2011-11-01 21:00:33 UTC
Hi Shawn, same as rubygem-addressable ... I'm happy to add review feedback to help this review move forward.  Let me know if I can help.

Comment 9 Shawn Starr 2011-11-06 07:07:17 UTC
Stripped out non-gem, since we're not doing this any more (as mentioned before by Vit, it makes no sense)

Updates can be found here:

http://www.sh0n.net/spstarr/fedora/rubygem-idn/rubygem-idn.spec
http://www.sh0n.net/spstarr/fedora/rubygem-idn/rubygem-idn-0.0.2-2.fc17.src.rpm

Additionally, this does not use RSPec 2.x and as mentioned with rubygem-addressable any help to make these compliant would be greatly appreciated for Ruby 1.9.x and 1.8 compatibility.

Comment 10 Vít Ondruch 2011-11-07 16:55:06 UTC
This gem doesn't use RSpec, so this issue is irrelevant in this case. Please don't worry too much about Ruby 1.9 compatibility. If there will be necessary some changes, it will be just one of 300 gems, so no big deal.

However, you are currently executing the test suite using Rake. I would suggest to replace the "rake test -v --trac" with something like "testrb -Ilib test/tc_*.rb". That allows you to remove the dependency on RDoc and Rake and the patch, which simplifies the maintenance of the library IMO.

Moreover:

* I would suggest to use %exclude macro instead of deleting files in install section, i.e:

%files
%exclude %{geminstdir}/ext
%exclude %{geminstdir}/.yardoc
%exclude %{gemname}-%{version}.gem

* I suggest to use:
%exclude %{gemdir}/cache/%{gemname}-%{version}.gem
since the gem cache has no meaning with RPM.

* I suggest to remove the -devel package, because I doubt that it has some meaning in this case. Will somebody except Ruby users base their work on the gem?

* I am not exactly sure about the meaning of the "cp %{SOURCE0} %{gemname}-%{version}.gem" command when you can simple call the gem install command with the %{SOURCE0} macro.

Comment 11 Mo Morsi 2011-11-10 16:48:41 UTC
OK thanks for the update

- looks like you addressed all my feedback

- rpmlint / koji builds are still green

- as Vit mentioned the cp command and devel packages can probably be removed unless there is good reason to keep those there. I feel it might be useful to keep the cached gem in place though as it's good for reference and in order to keep the files provided by rpm and gem installs fairly consistent. None of this is a big deal either way though

- Vit's other feedback is also good but not blockers as the package is now in compliance w/ Fedora guidelines. Feel free to implement his suggestions pre/post push.

APPROVED

Comment 12 Shawn Starr 2011-11-22 03:45:29 UTC
Vit feedback is added:

using:testrb -Ilib test/tc_*.rb

Removed copy %{SOURCE0}. I originally wanted to keep the .gem in the SRPM, What would happen if the gem upstream was compromised somehow? 

Removed -devel package.

use %exclude for files above + .h file.

Please mark package review + so I can submit package into git.

Comment 13 Vít Ondruch 2011-11-22 08:44:43 UTC
(In reply to comment #12)
> Removed copy %{SOURCE0}. I originally wanted to keep the .gem in the SRPM, What
> would happen if the gem upstream was compromised somehow? 

The %{SROURCE0} is always kept in SRPM. You cannot build the SRPM without the source gem. Moreover there is use --local flag during gem installation. The copying of %{SOURCE0} did not have any meaning, did not prevent anything.

Comment 14 Vít Ondruch 2011-11-22 08:46:46 UTC
(In reply to comment #12)
> Removed copy %{SOURCE0}. I originally wanted to keep the .gem in the SRPM, What
> would happen if the gem upstream was compromised somehow? 

There is even no internet connection on Koji during build, so there is no way how the gem could be accidentally downloaded from some compromised source.

Comment 15 Mo Morsi 2011-11-28 12:31:52 UTC
For some reason the '+' isn't available as an option for the fedora-review flag. Does anyone know the reason for this? It has worked in the past for me.

Comment 16 James Laska 2011-11-29 13:50:04 UTC
I've set fedora_review+ as requested

Comment 17 Shawn Starr 2011-12-20 17:40:59 UTC
I will be pushing this today was blocked due to fedora filer issue and being sick ;/

Comment 18 Shawn Starr 2012-01-02 20:07:26 UTC
New Package SCM Request
=======================
Package Name: rubygem-idn
Short Description: Ruby Bindings for the GNU LibIDN library
Owners: spstarr

Comment 19 Shawn Starr 2012-01-11 16:31:24 UTC
Can I get an update from the SCM team on this approval?

Comment 20 Shawn Starr 2012-01-12 00:26:52 UTC
n/m fedora-cvs is still used ;p

Comment 21 Gwyn Ciesla 2012-01-12 01:35:17 UTC
Git done (by process-git-requests).

Comment 22 Shawn Starr 2012-01-29 19:48:06 UTC
This is now in

Comment 23 Mo Morsi 2012-02-23 20:09:40 UTC
Could this be pushed into Fedora 16 as well. rubygem-webmock will be broken there until it is.

I've attached the following SRPM / SPEC and diff to the idn spec adding ruby 1.8 / Fedora 16 support. I've also requested commit rights to the rawhide so that I can request the new branch / do the push if you don't have the cycles.

Thanks.

http://mo.morsi.org/files/rpms/rubygem-idn-0.0.2-7.fc18.src.rpm
http://mo.morsi.org/files/rpms/rubygem-idn.spec
http://mo.morsi.org/files/rpms/idn-0.0.2.spec.diff

Comment 24 Shawn Starr 2012-02-23 22:22:27 UTC
Please do request commit, never can have enough maintainers :) even for older versions

Comment 25 Mo Morsi 2012-02-27 15:01:14 UTC
Package Change Request
======================
Package Name: rubygem-idn
New Branches: f16
Owners: mmorsi
InitialCC: 

F16 branch for rubygem-idn

Comment 26 Gwyn Ciesla 2012-02-27 15:08:11 UTC
Git done (by process-git-requests).

Comment 27 Fedora Update System 2012-02-27 15:27:42 UTC
rubygem-idn-0.0.2-7.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/rubygem-idn-0.0.2-7.fc16

Comment 28 Fedora Update System 2012-02-28 10:09:18 UTC
rubygem-idn-0.0.2-7.fc16 has been pushed to the Fedora 16 stable repository.

Comment 29 Troy Dawson 2014-08-06 16:58:36 UTC
Package Change Request
======================
Package Name: rubygem-idn
New Branches: epel7
Owners: tdawson

Comment 30 Gwyn Ciesla 2014-08-06 18:46:14 UTC
Git done (by process-git-requests).


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