Bug 588428 - Review Request: rubygem-addressable - Improved URI/URL handling
Review Request: rubygem-addressable - Improved URI/URL handling
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
urgent Severity high
: ---
: ---
Assigned To: Mo Morsi
Fedora Extras Quality Assurance
aeolus-dep opennebula-dep
: Reopened
Depends On:
Blocks: buildr 743402
  Show dependency treegraph
 
Reported: 2010-05-03 13:39 EDT by Adam Young
Modified: 2014-08-06 14:43 EDT (History)
10 users (show)

See Also:
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-17 20:46:14 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
jlaska: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Adam Young 2010-05-03 13:39:33 EDT
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-29 22:16:51 EDT
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 09:52:46 EST
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 11:58:23 EDT
I will take this over, needed for OpenNebula 3.0.
Comment 4 Shawn Starr 2011-08-04 00:44:16 EDT
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 08:19:20 EDT
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 03:46:48 EDT
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 09:28:55 EDT
ping? Am getting regular broken dependencies emails regarding rubygem-webmock.

Again appreciate the submission.
Comment 8 Shawn Starr 2011-10-19 20:19:10 EDT
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 16:59:11 EDT
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 19:23:17 EDT
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 01:41:15 EST
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 11:57:32 EST
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 13:32:57 EST
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-21 22:28:50 EST
Can someone change the flag to + I will submit package as soon as this is done.
Comment 15 Shawn Starr 2011-11-21 22:32:16 EST
(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 07:31:14 EST
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 08:50:14 EST
I've set fedora_review+ as requested
Comment 18 Mo Morsi 2011-12-08 08:04:23 EST
ping? Can we get this into Fedora / close this out now. Thanks alot.
Comment 19 Shawn Starr 2011-12-08 15:06:25 EST
Will push to git and build
Comment 20 Shawn Starr 2011-12-20 12:40:32 EST
I will be pushing this today was blocked due to fedora filer issue and being sick ;/
Comment 21 Shawn Starr 2012-01-02 15:06:30 EST
New Package SCM Request
=======================
Package Name: rubygem-addressable
Short Description: Improved URI/URL handling
Owners: spstarr
Comment 22 Shawn Starr 2012-01-11 11:31:44 EST
Can I get an update from the SCM team on this approval?
Comment 23 Shawn Starr 2012-01-11 19:25:25 EST
ah cvs flag... done
Comment 24 Gwyn Ciesla 2012-01-11 20:34:07 EST
Git done (by process-git-requests).
Comment 25 Shawn Starr 2012-01-29 15:12:19 EST
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 16:40:22 EST
This is now blocking two projects, and I've emailed upstream since this never occurred before.
Comment 27 Bohuslav "Slavek" Kabrda 2012-01-30 01:37:30 EST
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-09 23:07:59 EST
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-13 23:37:28 EST
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 04:07:37 EST
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-17 20:46:14 EST
This is pushed into f17 and rawhide now.
Comment 32 Fedora Update System 2012-02-18 11:56:12 EST
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 15:09:42 EST
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 17:22:02 EST
Please do request commit, never can have enough maintainers :) even for older versions
Comment 35 Mo Morsi 2012-02-27 10:02:01 EST
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 10:06:39 EST
Git done (by process-git-requests).
Comment 37 Fedora Update System 2012-03-06 15:39:17 EST
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 12:43:32 EDT
Package Change Request
======================
Package Name: rubygem-addressable
New Branches: epel7
Owners: tdawson
Comment 39 Gwyn Ciesla 2014-08-06 14:43:05 EDT
Git done (by process-git-requests).

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