Spec URL: http://niteshnarayan.fedorapeople.org/SPECS/rubygem-amq-protocol.spec
SRPM URL: http://niteshnarayan.fedorapeople.org/SRPMS/rubygem-amq-protocol-1.9.2-1.fc19.src.rpm

Taking this one. Note Nitesh needs sponsorship.

Overall package looks good, some nits

- Please update this bug summary in include the package summary not the package description

- Also for your other submissions the bug summary needs to include the full package name, eg 'rubygem-foo' not just 'foo' (this one is fine)

- The %check section should appear after the %install section and before the %files section

- Consider adding the following to the %install section

> pushd %{buildroot}%{gem_instdir}/
> rm -rf Rakefile amq-protocol.gemspec benchmarks \
>        .gitignore .travis.yml .rspec codegen profiling \
>        generate.rb .gitmodules
> popd


and removing the following from the %files sections

< %exclude %{gem_instdir}/Rakefile
< %exclude %{gem_instdir}/amq-protocol.gemspec
< %exclude %{gem_instdir}/benchmarks/*
< %exclude %{gem_instdir}/.gitignore
< %exclude %{gem_instdir}/.travis.yml
< %exclude %{gem_instdir}/.rspec

< %{gem_instdir}/codegen/*
< %{gem_instdir}/profiling/*
< %{gem_instdir}/generate.rb
< %{gem_instdir}/.gitmodules


- Currently the spec suite pulls in bundler and effin_utf8, both of which are unecessary deps (and in the later case not in fedora). Please add to the %check section right after the pushd but before the rspec

> sed -i /effin_utf8/d Gemfile
> sed -i /effin_utf8/d spec/spec_helper.rb
> sed -i /bundler/d spec/spec_helper.rb


- The %{gem_cache} file needs to be marked as %exclude

- The LICENSE file should be in the main package (not the 'doc' subpackage and _not_ marked as %doc)

- Please add spacing in your changelog entry (eg between your name and email and between the '-' and 'initial package'). See other specs for the correct format. (may seem small but there are often tools that parse this stuff)


Other than that package looks good and was able to koji it up. Once you make these changes and post the updated spec and srpm (make sure to bump the release and add a new changelog entry) I'll do the official review using fedora-review.

http://koji.fedoraproject.org/koji/taskinfo?taskID=6532053

(In reply to Mo Morsi from comment #2)
> - The LICENSE file should be in the main package (not the 'doc' subpackage
> and _not_ marked as %doc)
> 

Sorry for hijacking, but I noticed you wrote the license file should not marked as %doc. 

Currently, this is how I do it and how many others as well (from a quick look in rubygems at dist-git). Also, this is not documented anywhere I looked in packaging guidelines and all the packaging examples in the wiki put license under %doc. 

After reading the maximum rpm [0] though, I have to say that you have a point.

[0] http://www.rpm.org/max-rpm/s1-rpm-inside-files-list-directives.html

Apologies, I misremembered and  got it backwards, it should be included as %doc:

https://fedoraproject.org/wiki/Packaging:ReviewGuidelines#Things_To_Check_On_Review (see the line about the license file)

Good catch

Oh yeah, you're right I missed that page :)

SPEC:http://niteshnarayan.fedorapeople.org/SPECS/rubygem-amq-protocol-1.9.2-2.spec
SRPM: http://niteshnarayan.fedorapeople.org/SRPMS/rubygem-amq-protocol-1.9.2-1.fc19.src.rpm
Thanks for the comments, I had made the changes accordingly.

(In reply to Nitesh Narayan Lal from comment #6)
> SPEC:http://niteshnarayan.fedorapeople.org/SPECS/rubygem-amq-protocol-1.9.2-
> 2.spec

Hmm, wrong SPEC url. I see that the correct one is http://niteshnarayan.fedorapeople.org/SPECS/rubygem-amq-protocol.spec

You may want to resubmit both the SPEC/SRPM in order for the fedora-review tool to pick them ;)

Also, as Mo suggested

> - Please update this bug summary in include the package summary not the package > description

I changed the bug summary for you to include the rubygem's Summary.

Hey Nitesh, just getting back to this. Some more feedback.

- You added a new changelog entry for the release but didn't bump the release near the top. When you do so the SRPM that will be generated will be named "rubygem-amq-protocol-1.9.2-2.srpm"

- In the "%files doc" list there should be list line "%{gem_instdir}/spec/" instead of "%{gem_instdir}/spec/*". The former includes the directory and all files in it, the later just includes all files in it

- "Updated as per the comments" in your changelog entry is a bit ambiguous for future contributors / maintainers. Perhaps some like "updated from Fedora review feedback" would be more direct (people can search bugzilla from there)

Make sure to bump the release to '3' and add a new changelog entry after the above changes. Also like Achilleas mentioned, the links to the 'rubygem-amq-protocol.spec' and 'rubygem-amq-protocol-1.9.2-3.sprm' files should be in the comment so fedora-review will pick them up

https://fedorahosted.org/FedoraReview/

Thanks

I won't be able to work on this due to time deficiency, sorry for the delay.