Bug 588428

Summary: Review Request: rubygem-addressable - Improved URI/URL handling
Product: [Fedora] Fedora Reporter: Adam Young <ayoung>
Component: Package ReviewAssignee: Mo Morsi <mmorsi>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: high Docs Contact:
Priority: urgent    
Version: rawhideCC: bkabrda, fedora-package-review, jesusr, jlaska, mastahnke, mmorsi, notting, shawn.starr, tdawson, vondruch
Target Milestone: ---Keywords: Reopened
Target Release: ---Flags: jlaska: fedora-review+
gwync: fedora-cvs+
Hardware: All   
OS: Linux   
Whiteboard: aeolus-dep opennebula-dep
Fixed In Version: rubygem-addressable-2.2.6-3.fc17 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-02-18 01:46:14 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: 588406, 743402    

Description Adam Young 2010-05-03 17:39:33 UTC
Spec URL: http://github.com/admiyo/MySpecs/blob/master/rubygem-addressable.spec
SRPM URL: http://admiyo.fedorapeople.org/buildr-repo/rubygem-addressable-2.1.1-1.young.src.rpm
Description:  Addressable is a replacement for the URI implementation that is part of Ruby's standard library. It more closely conforms to the relevant RFCs and adds support for IRIs and URI templates. 

Dependency required for buildr.

Comment 1 Michael Stahnke 2010-08-30 02:16:51 UTC
builds in mock
rpmlint is clean

%ruby_sitelib should be removed, as it is not used.


You could add a %check

%check
cd %{buildroot}%{geminstdir}
rake spec


There isn't really a reason to do this. You can simply mark the normally installed README file a %doc.  
mkdir -p %{buildroot}/usr/share/doc/%{gemdir}-%{version}
mv  %{buildroot}%{geminstdir}/README %{buildroot}/usr/share/doc/%{gemdir}-%{version}

I'd also encourage you to break this package into a separate -doc package. It would allow someone wishing to use addressable as a dep to not install about 3/4 of the total files.

Comment 2 Jesus M. Rodriguez 2010-11-18 14:52:46 UTC
This initially started as a dependency for rubygem-buildr. It is no longer
needed for that. We can cancel this review.

Comment 3 Shawn Starr 2011-07-19 15:58:23 UTC
I will take this over, needed for OpenNebula 3.0.

Comment 4 Shawn Starr 2011-08-04 04:44:16 UTC
NOW: Part of Dependency packaging for OpenNebula 2.x/3.x

I have updated this to use Rspec for tests

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

Description: Improved URI/URL handling

MOCK: PASS

Comment 5 Mo Morsi 2011-10-07 12:19:20 UTC
This package is needed for rubygem-webmock as well. Taking the review

* package depends on rubygem-idn, review for that in progress

* package builds fine in mock w/ rubygem-idn manually installed

* rpmlint looks good

* Package matches upstream source's md5sum

$ md5sum rpmbuild/SOURCES/addressable-2.2.6.gem Downloads/addressable-2.2.6.gem 
3eec7c544b664f28023ff5d7fb0116ef  rpmbuild/SOURCES/addressable-2.2.6.gem
3eec7c544b664f28023ff5d7fb0116ef  Downloads/addressable-2.2.6.gem


* Can you remove references to ruby_sitearch as this is a noarch gem package

* As mentioned before could you replace the "%doc %{geminstdir}/[A-Z]*" with the doc files explicitly listed, just to prevent unintentional mistakes in the future (if non-doc files are added to the upstream project that match this)

* since %geminstdir is being marked as a %dir in the files section, does the Rakefile need to be marked w/ %exclude? (why exclude the Rakefile but then include the lib subdir?)

* The README file is currently listed twice, first as a doc in the main package (pulled in as part of the aforementioned wildcard expression) and then again in the docs subpackage, either place is acceptable but it should only appear once

http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles

Other than that the package looks good. Thanks for the submissions

Comment 6 Vít Ondruch 2011-10-10 07:46:48 UTC
Also few nits from my side:

* Please replace "Requires: rubygems" with virtual provider "Requires: ruby(rubygems)". The same apply also for BuildRequires.

* It seems that the pushd on line 54 is useless.

* pushd should be paired with popd in %check section

Comment 7 Mo Morsi 2011-10-18 13:28:55 UTC
ping? Am getting regular broken dependencies emails regarding rubygem-webmock.

Again appreciate the submission.

Comment 8 Shawn Starr 2011-10-20 00:19:10 UTC
Pong, let me get to this on weekend or so. I still need to sync with Ruby SIG so I can make sure I'm not getting swallowed up in the transisition to Ruby 1.9.3.

It would be really good if there was a Ruby SIG IRC channel.

Comment 9 James Laska 2011-11-01 20:59:11 UTC
Any updates?  Shawn, while I'm not a ruby expert, I'm happy to help with the review if you like.  Just let me know, and I'll be happy to get my review feedback in.

Comment 10 Shawn Starr 2011-11-03 23:23:17 UTC
I'll fix the comments. But as I've said. Ruby is moving to 1.9.x which is going to complicate things that are not RSpec 2.x ready. I should have this and the latter done by the weekend.

Comment 11 Shawn Starr 2011-11-06 06:41:15 UTC
This is updated from both feedback. It's still not RSpec 2.x though.

I'd appreciate any help/tips in making each of these rubygems compliant for Ruby 1.9.x and RSpec 2.x.

Updates here:

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

Comment 12 Vít Ondruch 2011-11-07 16:57:32 UTC
I did not checked the complete gem content, but the RSpec 2.x seems to be called correctly ATM. But if you call RSpec directly, then it renders Rake, RDoc and Yarv dependencies probably useless and they should be dropped.

Comment 13 Mo Morsi 2011-11-10 18:32:57 UTC
Package looks good. All feedback addressed. rpmlint / mock builds are green (again once rubygem-idn is installed manually).

APPROVED

Comment 14 Shawn Starr 2011-11-22 03:28:50 UTC
Can someone change the flag to + I will submit package as soon as this is done.

Comment 15 Shawn Starr 2011-11-22 03:32:16 UTC
(In reply to comment #14)
> Can someone change the flag to + I will submit package as soon as this is done.

Vit, removed rake/rdoc/yard dependencies also.

Comment 16 Mo Morsi 2011-11-28 12:31:14 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 17 James Laska 2011-11-29 13:50:14 UTC
I've set fedora_review+ as requested

Comment 18 Mo Morsi 2011-12-08 13:04:23 UTC
ping? Can we get this into Fedora / close this out now. Thanks alot.

Comment 19 Shawn Starr 2011-12-08 20:06:25 UTC
Will push to git and build

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

Comment 21 Shawn Starr 2012-01-02 20:06:30 UTC
New Package SCM Request
=======================
Package Name: rubygem-addressable
Short Description: Improved URI/URL handling
Owners: spstarr

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

Comment 23 Shawn Starr 2012-01-12 00:25:25 UTC
ah cvs flag... done

Comment 24 Gwyn Ciesla 2012-01-12 01:34:07 UTC
Git done (by process-git-requests).

Comment 25 Shawn Starr 2012-01-29 20:12:19 UTC
Looks like this fails in rawhide now:

DEBUG: Failures:
DEBUG:   1) Addressable::URI when form encoding a hash should result in correct percent encoded sequence
DEBUG:      Failure/Error: ).should == "%26one=%2F1&%3Dtwo=%3F2&%3Athree=%233"
DEBUG:        expected: "%26one=%2F1&%3Dtwo=%3F2&%3Athree=%233"
DEBUG:             got: "%3Athree=%233&%26one=%2F1&%3Dtwo=%3F2" (using ==)
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-expectations-2.6.0/lib/rspec/expectations/fail_with.rb:29:in `fail_with'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-expectations-2.6.0/lib/rspec/matchers/operator_matcher.rb:48:in `fail_with_message'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-expectations-2.6.0/lib/rspec/matchers/operator_matcher.rb:70:in `__delegate_operator'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-expectations-2.6.0/lib/rspec/matchers/operator_matcher.rb:60:in `eval_match'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-expectations-2.6.0/lib/rspec/matchers/operator_matcher.rb:29:in `=='
DEBUG:      # /builddir/build/BUILD/rubygem-addressable-2.2.6/usr/lib/ruby/gems/1.8/gems/addressable-2.2.6/spec/addressable/uri_spec.rb:4213
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/example.rb:48:in `instance_eval'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/example.rb:48:in `run'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/example.rb:107:in `with_around_hooks'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/example.rb:45:in `run'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/example_group.rb:294:in `run_examples'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/example_group.rb:290:in `map'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/example_group.rb:290:in `run_examples'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/example_group.rb:262:in `run'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/command_line.rb:24:in `run'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/command_line.rb:24:in `map'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/command_line.rb:24:in `run'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/reporter.rb:12:in `report'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/command_line.rb:21:in `run'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/runner.rb:80:in `run_in_process'
DEBUG:      # /usr/lib/ruby/gems/1.8/gems/rspec-core-2.6.4/lib/rspec/core/runner.rb:69:in `run'
DEBUG: Finished in 1.77 seconds
DEBUG: 1019 examples, 1 failure
DEBUG: Failed examples:
DEBUG: rspec ./spec/addressable/uri_spec.rb:4210 # Addressable::URI when form encoding a hash should result in correct percent encoded sequence

Comment 26 Shawn Starr 2012-01-29 21:40:22 UTC
This is now blocking two projects, and I've emailed upstream since this never occurred before.

Comment 27 Bohuslav "Slavek" Kabrda 2012-01-30 06:37:30 UTC
Hi Shawn,
I have run into these problem and here is the explanation:

There was a security problem with hash table collisions in Ruby < 1.9 (see [1] for more info). This was recently solved by introducing Ruby-1.8.7-p357, which deals with this issue by assigning a random seed to each hash (again, look at [1] for the details). Therefore the order of a hash can be different in each interpreter run. The solution is not to test for the exact string, but its contents. BTW, the order of hashes was never guaranteed in Ruby < 1.9, so it's always been a bug, but everything worked well in the simple cases.

In Ruby >= 1.9, the hashes are ordered by default, so the tests won't be failing once we switch to 1.9.3.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=750564

Comment 28 Shawn Starr 2012-02-10 04:07:59 UTC
I have an updated package for review:

* Koji scratch build pass 

SPEC: http://fedorapeople.org/~spstarr/packages/rubygem-addressable.spec

SRPM: http://fedorapeople.org/~spstarr/packages/rubygem-addressable-2.2.6-3.fc17.src.rpm

Comment 29 Shawn Starr 2012-02-14 04:37:28 UTC
ping, can someone review? Some fixes have been made to rubygem-idn which now make no modifications to rubygem-addressable required for package test to pass.

Comment 30 Vít Ondruch 2012-02-14 09:07:37 UTC
Shawn, I took just quick look on the spec file and it looks good. Since the package was already approved, you have requested git repo and it was already prepared, nothing can't prevent you from importing the package into Fedora ;)

Comment 31 Shawn Starr 2012-02-18 01:46:14 UTC
This is pushed into f17 and rawhide now.

Comment 32 Fedora Update System 2012-02-18 16:56:12 UTC
rubygem-addressable-2.2.6-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/rubygem-addressable-2.2.6-3.fc17

Comment 33 Mo Morsi 2012-02-23 20:09:42 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 addressable 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-addressable-2.2.6-4.fc18.src.rpm
http://mo.morsi.org/files/rpms/rubygem-addressable.spec
http://mo.morsi.org/files/rpms/addressable-2.2.6.spec.diff

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

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

F16 branch for rubygem-addressable

Comment 36 Gwyn Ciesla 2012-02-27 15:06:39 UTC
Git done (by process-git-requests).

Comment 37 Fedora Update System 2012-03-06 20:39:17 UTC
rubygem-addressable-2.2.6-3.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 38 Troy Dawson 2014-08-06 16:43:32 UTC
Package Change Request
======================
Package Name: rubygem-addressable
New Branches: epel7
Owners: tdawson

Comment 39 Gwyn Ciesla 2014-08-06 18:43:05 UTC
Git done (by process-git-requests).