Bug 1244760 - Review Request: rubygem-six - Very simple authorization gem
Summary: Review Request: rubygem-six - Very simple authorization gem
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Vít Ondruch
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-07-20 12:48 UTC by Ilya Gradina
Modified: 2015-09-29 05:40 UTC (History)
4 users (show)

Fixed In Version: rubygem-six-0.2.0-4.fc24
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-09-29 05:40:14 UTC
Type: ---
vondruch: fedora-review+


Attachments (Terms of Use)

Description Ilya Gradina 2015-07-20 12:48:11 UTC
Spec URL: http://repo.clanwars.org/gitlab/rubygem-six.spec
SRPM URL: http://repo.clanwars.org/gitlab/rubygem-six-0.2.0-1.fc24.src.rpm
Description: Very simple authorization gem.
Fedora Account System Username:ilgrad, ignatenkobrain

Comment 1 Vít Ondruch 2015-07-20 16:15:23 UTC
Lets do bug 1244764 first ...

Comment 2 Ilya Gradina 2015-08-03 12:46:24 UTC
Spec URL: http://repo.clanwars.org/gitlab/rubygem-six.spec
SRPM URL: http://repo.clanwars.org/gitlab/rubygem-six-0.2.0-2.fc23.src.rpm

changed description, changed summary, add rubygem(rspec) to BRs.

Comment 3 Igor Gnatenko 2015-08-03 13:19:56 UTC
(In reply to Ilya Gradina from comment #2)
> Spec URL: http://repo.clanwars.org/gitlab/rubygem-six.spec
> SRPM URL: http://repo.clanwars.org/gitlab/rubygem-six-0.2.0-2.fc23.src.rpm
> 
> changed description, changed summary, add rubygem(rspec) to BRs.
What you have changed in description? What in summary? if you changed summary -- you should change this bug title.

Comment 4 Vít Ondruch 2015-08-03 14:02:26 UTC
(In reply to Igor Gnatenko from comment #3)
> you should change this bug title.

This would be nice but not super-important. And the upstream summary/description is pretty brief I must say.

* Test suite expansion
  - I would suggest to expand the test suite in %check section. This will avoid
    accidental inclusion of the test suite in resulting RPM.
  - If you prefer to keep it in %prep section, then you should probably use
    %setup macro with -a 1 or -b 1 parameters [1].
  - You don't really need to delete the test suite after its execution, if you
    are using mock (not sure about local build thoug).

* License text
  - The package does not contain any license information. You should contact
    upstream and encourage them to correct this mistake [2, 3].


Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=10587608


Otherwise, the package looks good. Could you please do some informal review so I can sponsor you?




[1] http://www.rpm.org/max-rpm/s1-rpm-inside-macros.html
[2] http://fedoraproject.org/wiki/Packaging:ReviewGuidelines
[3] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#License_Text

Comment 5 Ilya Gradina 2015-08-04 00:01:54 UTC
Hi Vit!
Spec URL: http://repo.clanwars.org/gitlab/rubygem-six.spec
SRPM URL: http://repo.clanwars.org/gitlab/rubygem-six-0.2.0-3.fc23.src.rpm

add license file as SOURCE2, but I don't understand what did you mean by that "to expand the test suite in %check section".

Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=10593755

Comment 6 Vít Ondruch 2015-08-04 06:09:59 UTC
(In reply to Ilya Gradina from comment #5)
> add license file as SOURCE2,

Thanks, although upstream issue or PR would be enough.

BTW if you include some patch or LICENSE file in this case, it is good habit to provide some comment with reference, where you taken it. That way, next time you or anybody else touch this .spec file, will have a chance to check what is the progress of that issue. The patch could be already merged and the license shipped in latest release or it might be time to ping upstream again.

> but I don't understand what did you mean by
> that "to expand the test suite in %check section".

You did "tar xf %{SOURCE1}" to expand the test suite in %prep section, but you don't need this code for anything else except %check section. Hence you could move it into %check section. That way you would be sure it will not propagate into the output RPM by accident.

Nevertheless, that is my preference and somebody else could disagree. So this is just to explain. Your current approach is just fine.

Comment 7 Ilya Gradina 2015-08-04 18:14:16 UTC
Hi Vit!

Spec URL: http://repo.clanwars.org/gitlab/rubygem-six.spec
SRPM URL: http://repo.clanwars.org/gitlab/rubygem-six-0.2.0-4.fc23.src.rpm

I have added link to pull request which adds license in files distribution.

What is my next steps?

Comment 8 Vít Ondruch 2015-08-06 13:00:34 UTC
(In reply to Ilya Gradina from comment #7)
> I have added link to pull request which adds license in files distribution.

Thx
 
> What is my next steps?

I'm still waiting for some informal reviews. So far, I saw only bug 1246974, where you posted just output from fedora-review without any other comment. For example, there appears to be some issues identified by rpmlint, so you should probably pinpoint them and check if they are false positives and probably propose fix for the others.

I hope you understand this is not to discourage you, but to encourage proper packaging ...

Comment 9 Ilya Gradina 2015-09-19 11:45:24 UTC
unofficial review:
https://bugzilla.redhat.com/show_bug.cgi?id=1262965#c1

Comment 10 Vít Ondruch 2015-09-25 11:05:12 UTC
Thanks. I sponsored you and APPROVE this package, please continue with the SCM procedure.

Comment 11 Vít Ondruch 2015-09-29 05:40:14 UTC
This is Rawhide already => closing this bug.


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