Bug 839064

Summary: Review Request: rubygem-openshift-origin-common - OpenShift Origin library
Product: [Fedora] Fedora Reporter: Brenton Leanhardt <bleanhar>
Component: Package ReviewAssignee: Steven Dake <sdake>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: dwalsh, misc, notting, package-review, sdake, tdawson, vondruch
Target Milestone: ---Flags: sdake: fedora‑review+
limburgher: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-09-17 19:17:04 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 839395, 840037, 841641, 842459, 842890, 844817, 845021, 845107    

Description Brenton Leanhardt 2012-07-10 15:49:05 EDT
Spec URL: http://brenton.fedorapeople.org/package_reviews/rubygem-stickshift-common/rubygem-stickshift-common.spec
SRPM URL: http://brenton.fedorapeople.org/package_reviews/rubygem-stickshift-common/rubygem-stickshift-common-0.12.4-1.src.rpm
Description: This packages contains common library code for the stickshift OpenShift subsystem.
Fedora Account System Username: brenton

Hi!  This is my first package for Fedora.  I work for on the OpenShift team and we want to ship Origin as a feature for Fedora 18: https://fedoraproject.org/wiki/Features/OpenShift_Origin

This particular package contains common library code that is used throughout the stickshift subsystem.  I plan also to package stickshift-broker and stickshift-node.  I would also like to mention that I have commit access to the OpenShift codebase.

One interesting thing to note is that there an SELinux module bundled with this package.  The spec is fairly simple right now and we would prefer not to have subpackages unless it helps readability.  OpenShift cannot function without SELinux at least in permissive mode so it didn't make sense to use to allow admins to 'opt' out of installing the SELinux module because it would prevent  OpenShift from working.
Comment 2 Troy Dawson 2012-07-13 18:02:18 EDT
Informal Review:
Don't forget to include rubygem-devel.
Please use the macro's that are included in rubygems-devel.
https://fedoraproject.org/wiki/Packaging/Ruby#Macros

Using the new way of building gems in rpms will help keep rubygem packages more uniform.
https://fedoraproject.org/wiki/Packaging/Ruby#Building_gems

Please attach an rpmlint output.  Here is what I got

$ rpmlint rubygem-stickshift-common.spec
rubygem-stickshift-common.spec: E: specfile-error -e:1: Use RbConfig instead of obsolete and deprecated Config.
rubygem-stickshift-common.spec: E: specfile-error -e:1: Use RbConfig instead of obsolete and deprecated Config.
0 packages and 1 specfiles checked; 2 errors, 0 warnings.

$ rpmlint rubygem-stickshift-common-0.13.1-1.src.rpm
rubygem-stickshift-common.src: W: non-coherent-filename rubygem-stickshift-common-0.13.1-1.src.rpm rubygem-stickshift-common-0.13.1-1.git.45.ecfb399.fc16.src.rpm
rubygem-stickshift-common.src: E: specfile-error -e:1: Use RbConfig instead of obsolete and deprecated Config.
rubygem-stickshift-common.src: E: specfile-error -e:1: Use RbConfig instead of obsolete and deprecated Config.
1 packages and 0 specfiles checked; 2 errors, 1 warnings.

It looks like if you use the rubygems-devel %{ruby_sitelibdir} instead of the one you have in %global, the RBCongif error will go away.

Also, your src.rpm should be named rubygem-stickshift-common-0.13.1-1.git.45.ecfb399.fc16.src.rpm
I'm not sure if you changed the name intentionaly or not, but you should keep the rpm names what they are supposed to be.  It might look better for the moment, but it causes confusion.
Comment 4 Troy Dawson 2012-07-18 12:39:12 EDT
Thank you for the update.
Can you please put the src.rpm up there.  You currently only have the binary (noarch) rpm.
Comment 6 Troy Dawson 2012-07-18 13:04:43 EDT
It's looking better.

First issue - Although you have a URL for the source, the tarball isn't there.

Second issue ... which I don't know if it's your fault or not.
I'm trying to use fedora-review to test this package.  It will build in mock, but then when it starts checking the binary rpm's it fails with the following command.

--------------
Build completed
Run command: rpm -qpl /tmp/common/rubygem-stickshift-common/results/rubygem-stickshift-common-0.13.1-2.git.85.1915eff.fc17.noarch.rpm
Exception down the road...
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/FedoraReview/review_helper.py", line 133, in run
    self.__do_report()
  File "/usr/lib/python2.7/site-packages/FedoraReview/review_helper.py", line 79, in __do_report
    self.__run_checks(self.bug.spec_file, self.bug.srpm_file)
  File "/usr/lib/python2.7/site-packages/FedoraReview/review_helper.py", line 105, in __run_checks
    writedown=not Settings.no_report)
  File "/usr/lib/python2.7/site-packages/FedoraReview/checks_class.py", line 180, in run_checks
    if test.is_applicable():
  File "/usr/lib/python2.7/site-packages/FedoraReview/checks/ccpp.py", line 13, in is_applicable
    self.sources_have_files('*.c') or \
  File "/usr/lib/python2.7/site-packages/FedoraReview/check_base.py", line 105, in sources_have_files
    sources_files = self.sources.get_files_sources()
  File "/usr/lib/python2.7/site-packages/FedoraReview/sources.py", line 86, in get_files_sources
    self.extract_all()
  File "/usr/lib/python2.7/site-packages/FedoraReview/sources.py", line 65, in extract_all
    source.extract()
  File "/usr/lib/python2.7/site-packages/FedoraReview/source.py", line 104, in extract
    if not self.rpmdev_extract(self.filename, self.extract_dir):
AttributeError: 'Source' object has no attribute 'filename'
Exception down the road...
-----------------

This output come from fedora-review-0.2.0-1.fc17 but it also fails on the latest from the git repo as well.
Comment 7 Steven Dake 2012-07-18 13:26:34 EDT
file a bug against fedora review please
Comment 9 Steven Dake 2012-07-21 15:01:20 EDT
Brenton,

I'll sponsor you.

To join the packager group you need to be able to do the following things:
1. provide competent reviews of other people's packages
2. produce high quality packaging that passes the guidelines prior to review
3. help coach packagers on trouble points in their packaging

Read:

http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

A package should follow the packaging guidelines:

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

Since you have submitted a package, I will ask you in the bugzilla to review a
couple other people's packages.  While you are not a packager, you can
still provide reviews to demonstrate you are capable of providing a
review of a new package.  To execute a review, you would follow the
review guidelines:

http://fedoraproject.org/wiki/Packaging:ReviewGuidelines

Some example reviews I have done are here:

https://bugzilla.redhat.com/buglist.cgi?f1=flagtypes.name&list_id=79500&o1=equals&classification=Fedora&emailtype1=substring&query_format=advanced&emailassigned_to1=1&token=1338582948-9534ec43e4e74cdb0393ec72859aedfe&bug_status=NEW&bug_status=ASSIGNED&bug_status=MODIFIED&bug_status=ON_DEV&bug_status=ON_QA&bug_status=VERIFIED&bug_status=RELEASE_PENDING&bug_status=POST&bug_status=CLOSED&email1=sdake%40redhat.com&v1=fedora-review%2B&component=Package%20Review

Once you have given a couple high quality reviews of other's packages,
I'll review your package submission and we will get it beat into
submission for Fedora.

When your ready to review atleast two other packages, find some FE-NEEDSPONSOR packages and use the FedoraReview tool to review those packages.

The FedoraReview tool can be found at: https://fedorahosted.org/FedoraReview/
Comment 10 Michael Scherer 2012-07-21 21:07:03 EDT
Fedora-review fail because the url is 404. This should IMHO either stop the build or be clearer, I have filled https://fedorahosted.org/FedoraReview/ticket/81 .
Comment 11 Brenton Leanhardt 2012-07-26 09:57:26 EDT
Thanks Steven!

I re-read through the guidelines docs yesterday.  I'll find some packages to review today.  Thanks for the help.
Comment 12 Brenton Leanhardt 2012-07-27 08:26:16 EDT
Hi Steven,

Yesterday I had some time so I started reviewing 4 FE-NEEDSPONSOR bugs that hadn't received much attention:

https://bugzilla.redhat.com/show_bug.cgi?id=822959
https://bugzilla.redhat.com/show_bug.cgi?id=818729
https://bugzilla.redhat.com/show_bug.cgi?id=839142
https://bugzilla.redhat.com/show_bug.cgi?id=822283

Let me know what you think.
Comment 13 Michael Scherer 2012-08-02 01:33:42 EDT
Hi Brenton, why does it require rubygem(mongo) since there is no mongo related code in the gem :

/tmp/rubygem-stickshift-common-0.13.3 $ grep -r -i mongo .
./doc/selinux/stickshift.te:#mongo
./doc/selinux/stickshift.te:    type mongod_port_t;
./doc/selinux/stickshift.te:  allow mcollectived_t mongod_port_t:tcp_socket name_connect;
./stickshift-common.spec:Requires:       rubygem(mongo)
./stickshift-common.gemspec:  s.add_dependency("mongo")
./Gemfile.lock:      mongo
./Gemfile.lock:    mongo (1.5.2)

I think this deps should be pushed to the others rpms.
Comment 14 Michael Scherer 2012-08-02 16:05:23 EDT
And by the way, if you rename this package ( like the other ), could you just change the title of the bug instead of reopening a new one :
- easier to follow ( especially when review started )
- no need to redo the whole link between bugs ( especially for this one since everything depend on it )
Comment 15 Brenton Leanhardt 2012-08-06 13:52:39 EDT
SRPM: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-common/201208061331/rubygem-openshift-origin-common-0.13.3-2.fc18.src.rpm

fedora-review: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-common/201208061331/rubygem-openshift-origin-common-review.txt

Spec: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-common/201208061331/rubygem-openshift-origin-common.spec

I fixed a number of issues recorded in the changelog.  I also attached the fedora-review output because I don't understand some of the output:

[!]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.

I'm not sure why this is failing.  The source packages does include it's own license file and it is included in %doc.

[!]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/rpmbuild/rpmbuild/SPECS/openshift-origin-common-0.13.3.gem :
  MD5SUM this package     : e8c3b740b2bf8f6cd2149c4928766d14
  MD5SUM upstream package : d41d8cd98f00b204e9800998ecf8427e

When I download http://mirror.openshift.com/pub/openshift-origin/source/rubygem-openshift-origin-common/openshift-origin-common-0.13.3.gem and check the md5sum it's e8c3b740b2bf8f6cd2149c4928766d14.  That's also the md5sum for ~/rpmbuild/SOURCES/openshift-origin-common-0.13.3.gem for the user that's building this package.  Where is d41d8cd98f00b204e9800998ecf8427e coming from?
Comment 16 Steven Dake 2012-08-06 15:28:40 EDT
Regarding the reviews you have done (Comment #12):

Good first start.  I'd like to see some message exchanges in the bugzilla (ie: users correcting review problems), followed by a final fedora-review paste.
Comment 17 Steven Dake 2012-08-06 15:36:30 EDT
[sdake@bigiron Downloads]$ rpmlint rubygem-openshift-origin-common-0.13.3-2.fc18.src.rpm
rubygem-openshift-origin-common.src:116: W: macro-in-%changelog %defattr
rubygem-openshift-origin-common.src:117: W: macro-in-%changelog %clean
rubygem-openshift-origin-common.src: W: invalid-url Source0: http://mirror.openshift.com/pub/openshift-origin/source/openshift-origin-common-0.13.3.gem HTTP Error 404: Not Found
1 packages and 0 specfiles checked; 0 errors, 3 warnings.


The reason the md5 sums don't match is that the file cannot be downloaded.

Sources URI:
http://mirror.openshift.com/pub/openshift-origin/source/openshift-origin-common-0.13.3.gem

Actual URI:
http://mirror.openshift.com/pub/openshift-origin/source/rubygem-openshift-origin-common/openshift-origin-common-0.13.3.gem

Looks like typo error.
Comment 18 Brenton Leanhardt 2012-08-06 16:07:42 EDT
Yeah, I noticed the URL was wrong but got dragged into a meeting before I could upload the latest fixes.  I have that fixed shortly.
Comment 20 Michael Scherer 2012-08-07 10:38:14 EDT
I am not sure that the common way to ship a change to the selinux policy is to do it like you did, shouldn't it be sent directly to the package instead ?
Comment 21 Brenton Leanhardt 2012-08-07 10:51:10 EDT
Would you mind clarifying "sent directly to the package"?

If you are asking about merging this into the base policy for Fedora that is definitely our long term intention.  However the amount of churn on this code right now would not be appropriate at this point in time (in my opinion).
Comment 22 Steven Dake 2012-08-07 10:58:07 EDT
Typically you would file a bug with the selinux-policy package and if you have specific solutions, attach them (they like that ;).  Fedora puts all policy in one place.  I'll get back to you on best choice for this package.
Comment 23 Daniel Walsh 2012-08-07 13:59:08 EDT
I have no problem with packages shipping their own policy.  There are a few within Fedora right now.  My only concern would be that the policy writers know what they are doing, and work with the selinux-policy maintainers when they have something questionable in their policy.
Comment 24 Brenton Leanhardt 2012-08-07 15:58:48 EDT
(In reply to comment #16)
> Regarding the reviews you have done (Comment #12):
> 
> Good first start.  I'd like to see some message exchanges in the bugzilla
> (ie: users correcting review problems), followed by a final fedora-review
> paste.

Is https://bugzilla.redhat.com/show_bug.cgi?id=839142 sufficient?  The user fixed the issue I described however Matěj Cepl jumped in before I was back from vacation and performed the final fedora-review before approving.
Comment 25 Steven Dake 2012-08-08 21:05:46 EDT
RE Comment #24, its unfortunate that happened, but I need to see a full fedora review run on a package.  The best way to do this is to get the packager to respond to your comments.  Unfortunately the other packages you are reviewing look like the packager is not providing feedback.  I'd recommend taking a look at another couple of packages to try to get some feedback that leads to a full fedora review (atleast 1).

Regards
-steve
Comment 26 Vít Ondruch 2012-08-09 09:37:25 EDT
Hi,

I have several comments regarding this package and they might be applied to other openshift packages as well

* rubygems.org as a source
  - I am wondering why don't you use rubygems.org as every Rubyist would expect
    to be standard source of every gem. This is just for curiosity and to prevent
    possible (although in this case unlikely) name collision

* Macros for gem packaging
  - It was already mentioned by Troy, that rubygems-devel package provides some
    useful macros for gems packaging.
  - For RHEL, you can use [1] as an example and optionally comment on Bug 788001
    to push these macros into RHEL 

* Documentation in -doc subpackage
  - Have you considered to put the documentation into -doc subpackage? It is
    typically substantial part of the rubygem- package payload.

* mongo dependencies
  - You were requested to remove the rubygem(mongo) dependency. However,
    the .gemspec still requires the dependency. This should be fixed upstream
    and temporary patched in .gemspec

    # irb
    irb(main):001:0> require 'stickshift-common'
    Gem::LoadError: Could not find mongo (>= 0) amongst [activemodel-3.2.7,
        activesupport-3.2.7, bigdecimal-1.1.0, builder-3.0.0, i18n-0.6.0,
        io-console-0.3, json-1.6.5, multi_json-1.3.6,
        openshift-origin-common-0.13.3, rdoc-3.12]
            from /usr/share/rubygems/rubygems/dependency.rb:247:in `to_specs'
            from /usr/share/rubygems/rubygems/specification.rb:777:in `block in
                activate_dependencies'
            from /usr/share/rubygems/rubygems/specification.rb:766:in `each'
            from /usr/share/rubygems/rubygems/specification.rb:766:in
                `activate_dependencies'
            from /usr/share/rubygems/rubygems/specification.rb:750:in `activate'
            from /usr/share/rubygems/rubygems.rb:212:in `rescue in try_activate'
            from /usr/share/rubygems/rubygems.rb:209:in `try_activate'
            from /usr/share/rubygems/rubygems/custom_require.rb:59:in `rescue
                in require'
            from /usr/share/rubygems/rubygems/custom_require.rb:35:in `require'
            from (irb):1
            from /usr/bin/irb:12:in `<main>'


* Url does not correspondent with the gems one
  - URL in .spec says openshift.redhat.com while the .gemspec contains line
    s.homepage = "http://www.openshift.com". These two should be aligned IMO

* Is Requires: policycoreutils-python needed?
  - The selinux-policy owns the /usr/share/selinux/packages/ directory, but
    what is the need to require policycoreutils-python?

BTW I am not sure why the line 'require "stickshift-common/exceptions/ss_exception"' is duplicated in lib/stickshift-common.rb, but this is upstream issue.

[1] http://pkgs.fedoraproject.org/cgit/rubygem-hydra.git/tree/rubygem-hydra.spec
Comment 27 Brenton Leanhardt 2012-08-10 15:12:26 EDT
(In reply to comment #26)

> * rubygems.org as a source

We'll consider publishing to rubygems.org however this is not the way we would recommend users install the serverside components of OpenShift Origin at this point.  We'd like to avoid to case where we end up having to support 'gem install' versions of Origin before we're ready.

> * Macros for gem packaging

I added a comment to Bug 788001.

> 
> * Documentation in -doc subpackage

We'll be happy to create separate -doc subpackages in the future.  Right now we think it would be a distraction to making Origin available to the community so we don't plan to do it for Fedora 18.

> 
> * mongo dependencies

The unneeded require was removed and the gemspec was patch.  The fix has been submitted upstream.

> 
> 
> * Url does not correspondent with the gems one

This was fixed upstream as well for all Origin rubygems.  Great catch.

> 
> BTW I am not sure why the line 'require
> "stickshift-common/exceptions/ss_exception"' is duplicated in
> lib/stickshift-common.rb, but this is upstream issue.

That's almost certainly a copy/paste error.  I'll fix that upstream!

Here are the latest artifacts:

SRPM: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-common/201208101436/rubygem-openshift-origin-common-0.13.3-4.fc16.src.rpm

Spec: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-common/201208101436/rubygem-openshift-origin-common.spec
Comment 28 Brenton Leanhardt 2012-08-13 16:37:42 EDT
(In reply to comment #25)
> RE Comment #24, its unfortunate that happened, but I need to see a full
> fedora review run on a package.  The best way to do this is to get the
> packager to respond to your comments.  Unfortunately the other packages you
> are reviewing look like the packager is not providing feedback.  I'd
> recommend taking a look at another couple of packages to try to get some
> feedback that leads to a full fedora review (atleast 1).
> 
> Regards
> -steve

Hi Steve,

Just an update.  I haven't forgot about this.  I'm doing my best to make time to do another review this week.
Comment 29 Brenton Leanhardt 2012-08-14 10:21:53 EDT
Here are two informal reviews I started this morning:

https://bugzilla.redhat.com/show_bug.cgi?id=848043
https://bugzilla.redhat.com/show_bug.cgi?id=847504

Both of them are recent so hopefully the requestors will respond quickly.
Comment 30 Brenton Leanhardt 2012-08-14 11:50:15 EDT
Both requestors have fixed all issues I addressed:

https://bugzilla.redhat.com/show_bug.cgi?id=848043
https://bugzilla.redhat.com/show_bug.cgi?id=847504
Comment 31 Steven Dake 2012-08-14 12:51:10 EDT
Brenton,

Those look like good reviews, but I'd recommend the following workflow.

Report problems you have identified by manually inspecting the tarball and specfile.

When developer has fixed all the problems you have found, provide a complete fedora review.  Your fedora reviews are missing results (because the automated tool can't check them yet/ever).  In this case, you should manually determine if the package passes muster by including a X, or -, or !.

Can you go back to those last two reviews and do that?

After that is done correctly, I'll formally review your package.

Regards
-steve
Comment 32 Brenton Leanhardt 2012-08-14 14:38:01 EDT
(In reply to comment #31)
> Brenton,
> 
> Those look like good reviews, but I'd recommend the following workflow.
> 
> Report problems you have identified by manually inspecting the tarball and
> specfile.
> 
> When developer has fixed all the problems you have found, provide a complete
> fedora review.  Your fedora reviews are missing results (because the
> automated tool can't check them yet/ever).  In this case, you should
> manually determine if the package passes muster by including a X, or -, or !.
> 
> Can you go back to those last two reviews and do that?
> 
> After that is done correctly, I'll formally review your package.
> 
> Regards
> -steve

I've manually checked all items fedora-review could not automate for these packages:

This one has licensing issues:
https://bugzilla.redhat.com/show_bug.cgi?id=848043

I would approve this package:
https://bugzilla.redhat.com/show_bug.cgi?id=847504

My suspicion is that the license issue will take a while to resolve with upstream.  Technically in comment #25 you said I would only need to do this for one package, right? :)  We're a little desperate at this point to get all of the OpenShift Origin packages in for F18 at this point.
Comment 33 Vít Ondruch 2012-08-15 07:32:24 EDT
* It is good habit to bump release each version you submit for a review.
  - This makes easier to track changes you have made.
  - You could use rpmdev-bumpspec tool to bump the release.

* Now, I am a bit confused. The latest .spec you provided is different then the
  .spec in your SRPM and moreover, the changelog does not corresponds with
  Release specified in the header. So what is the most recent version?
  - I am going to assume that the independent .spec file is the most recent one.

* Keep the LICENSE and COPYRIGHT file on their original locations.
  - I.e. reference them as a %{gem_instdir}/LICENSE and
    %{gem_instdir}/COPYRIGHT in the %files section.



(In reply to comment #27)
> > * Macros for gem packaging
> 
> I added a comment to Bug 788001.

I would appreciate if you could follow the example I provided to you and use new Fedora macros by default.
Comment 34 Brenton Leanhardt 2012-08-15 10:30:54 EDT
Apologies for the confusion.  It was certainly my intention to bump the spec.

SRPM: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-common/201208150950/rubygem-openshift-origin-common-0.13.3-6.fc18.src.rpm

Spec: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-common/201208150950/rubygem-openshift-origin-common.spec

To avoid the "files listed twice" warning I actually just removed the LICENSE and COPYRIGHT from the files section.  That way they will get shipped in the gem_instdir.  If that's not what you meant just let me know.

As for the macros provided by rubygems-devel, my main problem with using them is that I'm also planning to maintain this package for EPEL soon and it seems like it would needlessly complicate the specfile for now since those macros aren't available for RHEL yet.
Comment 35 Vít Ondruch 2012-08-16 01:16:24 EDT
(In reply to comment #34)
> Apologies for the confusion.  It was certainly my intention to bump the spec.
> 
> SRPM:
> http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-
> common/201208150950/rubygem-openshift-origin-common-0.13.3-6.fc18.src.rpm
> 
> Spec:
> http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-
> common/201208150950/rubygem-openshift-origin-common.spec
> 
> To avoid the "files listed twice" warning I actually just removed the
> LICENSE and COPYRIGHT from the files section.  That way they will get
> shipped in the gem_instdir.  If that's not what you meant just let me know.

Yes, that better.

Actually, I always suggest to own just the gem directory, such as:

%dir %{gem_instdir}

or in your case:

%dir %{gem_dir}/doc/%{gem_name}-%{version}

This is a bit more work, since you will need to explicitly own the directory content as well, but it will pay of later, because you will be warned about bigger changes in you package.

> As for the macros provided by rubygems-devel, my main problem with using
> them is that I'm also planning to maintain this package for EPEL soon and it
> seems like it would needlessly complicate the specfile for now since those
> macros aren't available for RHEL yet.

I understand your rationale. And that is the beauty of the example I shown you above. Once the macros are available in the RHEL, you would be able to drop just a few macro definitions on the top of the file instead of rewriting every macro scattered around the file.


BTW why do you keep the SELinux directory in the package? Moreover why do you own just its content and not the directory?
Comment 36 Brenton Leanhardt 2012-08-16 12:49:32 EDT
Hi Vít, Let me know if this looks OK:

* Thu Aug 16 2012 Brenton Leanhardt <bleanhar@redhat.com> - 0.13.3-7
- Using rubygem-devel macros and the exclude macro
- Removed SELinux dir from package

SRPM: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-common/201208161241/rubygem-openshift-origin-common-0.13.3-7.fc18.src.rpm

Spec: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-common/201208161241/rubygem-openshift-origin-common.spec

Thanks.
Comment 37 Steven Dake 2012-08-16 13:17:30 EDT
Brenton,

Spec file looks pretty good.  While not forbidden by the packaging guidelines, I generally feel it is bad practice to have conditionals in a spec file for EPEL vs Fedora.  They are separate distributions with separate dependency chains.  By focusing the spec file for "fedora only" all this nonsense disappears:

# Conditionally set required macros for distros without rubygems-devel This can
# be removed once https://bugzilla.redhat.com/show_bug.cgi?id=788001 is
# resolved.
%if 0%{?el6}%{?fc16}
%global rubyabi 1.8
%global gem_dir %(ruby -rubygems -e 'puts Gem::dir' 2>/dev/null)
%global gem_cache %{gem_dir}/cache/%{gem_name}-%{version}.gem
%global gem_docdir %{gem_dir}/doc/%{gem_name}-%{version}
%global gem_instdir %{gem_dir}/gems/%{gem_name}-%{version}
%global gem_libdir %{gem_instdir}/lib
%global gem_spec %{gem_dir}/specifications/%{gem_name}-%{version}.gemspec
%endif

%if 0%{?rhel} == 6
BuildRequires:  rubygems
%else
BuildRequires:  rubygems-devel
%endif

This also prevents the developer from doing something like copying the fedora spec file around to EPEL (because that is easy, vs actually looking at the EPEL spec) which could result in breakage later.  Instead I would recommend maintaining two separate spec files for your sanity, but again, it is up to the packager.

I'll provide full review next.
Comment 38 Steven Dake 2012-08-16 13:35:41 EDT
Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[x]: EXTRA Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: EXTRA Spec file according to URL is the same as in SRPM.
[ ]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[-]: MUST Macros in Summary, %description expandable at SRPM build time.
[-]: MUST Package contains desktop file if it is a GUI application.
[-]: MUST Development files must be in a -devel package
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Package complies to the Packaging Guidelines
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[ ]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Apache (v2.0)" For detailed output of licensecheck see file:
     /home/sdake/839064-rubygem-openshift-origin-common/licensecheck.txt
[x]: MUST Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: MUST Package is named using only allowed ascii characters.
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[-]: MUST If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Package is not relocatable.
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: MUST Package contains systemd file(s) if in need.


[x]: MUST File names are valid UTF-8.
[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: SHOULD Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: SHOULD Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL5 is required
[-]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[-]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[x]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX tarball generation or download is documented.
[!]: SHOULD SourceX / PatchY prefixed with %{name}.
     Note: Source2 (stickshift.if) Source3 (stickshift.te) Source0 (openshift-
     origin-common-0.13.3.gem) Source1 (stickshift.fc) Patch0 (openshift-
     origin-common_gemspec_fixes.patch)
[x]: SHOULD SourceX is a working URL.
[x]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[-]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.


==== Language ====
[x]: MUST Gem package must not define a non-gem subpackage
[x]: MUST Gem package must exclude cached Gem.
[x]: MUST Platform dependent files must all go under %{gem_extdir}, platform
     independent under %{gem_dir}.
[x]: MUST Gem package is named rubygem-%{gem_name}
[x]: MUST Package contains BuildRequires: rubygems-devel.
[x]: MUST Gem package must define %{gem_name} macro.
[x]: MUST Pure Ruby package must be built as noarch
[x]: MUST Package contains Requires: ruby(abi).
[!]: SHOULD Specfile should utilize macros from rubygem-devel package.
     Note: The specfile doesn't use these macros: %{gem_libdir}
[x]: SHOULD Test suite should not be run by rake.
[-]: SHOULD Test suite of the library should be run.

Issues:
See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

Rpmlint
-------
Checking: rubygem-openshift-origin-common-0.13.3-7.fc17.src.rpm
          rubygem-openshift-origin-common-0.13.3-7.fc17.noarch.rpm
rubygem-openshift-origin-common.noarch: W: wrong-file-end-of-line-encoding /usr/share/gems/doc/openshift-origin-common-0.13.3/ri/cache.ri
rubygem-openshift-origin-common.noarch: W: unexpanded-macro /usr/share/gems/doc/openshift-origin-common-0.13.3/ri/StickShift/Group/component_refs%3d-i.ri %3d
rubygem-openshift-origin-common.noarch: W: unexpanded-macro /usr/share/gems/doc/openshift-origin-common-0.13.3/ri/StickShift/Component/subscribes%3d-i.ri %3d
rubygem-openshift-origin-common.noarch: W: unexpanded-macro /usr/share/gems/doc/openshift-origin-common-0.13.3/ri/StickShift/Model/new_record%3f-i.ri %3f
rubygem-openshift-origin-common.noarch: W: unexpanded-macro /usr/share/gems/doc/openshift-origin-common-0.13.3/ri/StickShift/Group/scaling%3d-i.ri %3d
rubygem-openshift-origin-common.noarch: W: unexpanded-macro /usr/share/gems/doc/openshift-origin-common-0.13.3/ri/StickShift/Profile/groups%3d-i.ri %3d
rubygem-openshift-origin-common.noarch: W: unexpanded-macro /usr/share/gems/doc/openshift-origin-common-0.13.3/ri/StickShift/Profile/connections%3d-i.ri %3d
rubygem-openshift-origin-common.noarch: W: unexpanded-macro /usr/share/gems/doc/openshift-origin-common-0.13.3/ri/StickShift/Model/deleted%3f-i.ri %3f
rubygem-openshift-origin-common.noarch: W: unexpanded-macro /usr/share/gems/doc/openshift-origin-common-0.13.3/ri/StickShift/Cartridge/profiles%3d-i.ri %3d
rubygem-openshift-origin-common.noarch: W: unexpanded-macro /usr/share/gems/doc/openshift-origin-common-0.13.3/ri/StickShift/Model/attributes%3d-i.ri %3d
rubygem-openshift-origin-common.noarch: W: unexpanded-macro /usr/share/gems/doc/openshift-origin-common-0.13.3/ri/StickShift/Profile/components%3d-i.ri %3d
rubygem-openshift-origin-common.noarch: W: unexpanded-macro /usr/share/gems/doc/openshift-origin-common-0.13.3/ri/StickShift/Component/publishes%3d-i.ri %3d
rubygem-openshift-origin-common.noarch: W: unexpanded-macro /usr/share/gems/doc/openshift-origin-common-0.13.3/ri/StickShift/Model/persisted%3f-i.ri %3f
2 packages and 0 specfiles checked; 0 errors, 13 warnings.


Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:
Requires
--------
rubygem-openshift-origin-common-0.13.3-7.fc17.noarch.rpm (rpmlib, GLIBC filtered):

    /bin/sh
    ruby(abi) >= 1.8
    rubygem(activemodel)
    rubygem(json)
    rubygems
    selinux-policy

Provides
--------
rubygem-openshift-origin-common-0.13.3-7.fc17.noarch.rpm:

    rubygem(openshift-origin-common) = 0.13.3
    rubygem-openshift-origin-common = 0.13.3-7.fc17

MD5-sum check
-------------
http://mirror.openshift.com/pub/openshift-origin/source/rubygem-openshift-origin-common/openshift-origin-common-0.13.3.gem :
  CHECKSUM(SHA256) this package     : abd7ea533e275518d253ba853c4d15603c3be05b23c52e704af555a5322ca793
  CHECKSUM(SHA256) upstream package : abd7ea533e275518d253ba853c4d15603c3be05b23c52e704af555a5322ca793


Generated by fedora-review 0.2.0 (53cc903) last change: 2012-07-09
Command line :/usr/bin/fedora-review -b 839064
External plugins:
Comment 39 Steven Dake 2012-08-16 13:37:33 EDT
Note:
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[-]: MUST Large documentation files are in a -doc subpackage, if required.

At some point in the future, you may want to evaluate creating a separate doc package.
Comment 40 Steven Dake 2012-08-16 13:39:43 EDT
Brenton,

Nice job!  I am convinced you know how to provide competent reviews and competently package for Fedora.  Welcome to the packagers group!  When you receive notification from the accounts system that you have been added to the packagers group, please submit an SCM request as shown in:

https://fedoraproject.org/wiki/Package_SCM_admin_requests
Comment 41 Steven Dake 2012-08-16 13:40:25 EDT
PACKAGE APPROVED.
Comment 42 Brenton Leanhardt 2012-08-16 13:53:56 EDT
(In reply to comment #39)
> Note:
> [x]: MUST Package is licensed with an open-source compatible license and
> meets
>      other legal requirements as defined in the legal section of Packaging
>      Guidelines.
> [-]: MUST Large documentation files are in a -doc subpackage, if required.
> 
> At some point in the future, you may want to evaluate creating a separate
> doc package.

Agreed.  There are several people packaging OpenShift for Fedora and once the dust settles a little bit we'll do this for all our packages at once.
Comment 43 Brenton Leanhardt 2012-08-16 13:55:43 EDT
(In reply to comment #40)
> Brenton,
> 
> Nice job!  I am convinced you know how to provide competent reviews and
> competently package for Fedora.  Welcome to the packagers group!  When you
> receive notification from the accounts system that you have been added to
> the packagers group, please submit an SCM request as shown in:
> 
> https://fedoraproject.org/wiki/Package_SCM_admin_requests

Seriously, thanks for all the help.
Comment 44 Brenton Leanhardt 2012-08-16 14:53:20 EDT
New Package SCM Request
=======================
Package Name: rubygem-openshift-origin-common
Short Description: Common library code for OpenShift
Owners: brenton tdawson maxamillion
Branches: f17 f18
InitialCC:
Comment 45 Jon Ciesla 2012-08-16 15:36:35 EDT
Git done (by process-git-requests).
Comment 46 Vít Ondruch 2012-08-17 03:20:03 EDT
Brenton,

You probably don't want to %exclude %{gem_libdir}, or do you?

BTW I would mark as a documentation only the files which are documentation.
Comment 47 Michael Scherer 2012-08-19 05:36:25 EDT
Removed the block on FE-NEEDSPONSOR, since brenton is now a packager.
Comment 48 Brenton Leanhardt 2012-08-21 09:45:55 EDT
(In reply to comment #46)
> Brenton,
> 
> You probably don't want to %exclude %{gem_libdir}, or do you?
> 
> BTW I would mark as a documentation only the files which are documentation.

I fixed this in dist-git and built the package.  Thanks for the help!
Comment 49 Vít Ondruch 2012-08-22 02:45:33 EDT
Thank you. However, to get the packages into the Fedora 18, you should do 2 more things:

1) Submit update via Bodhi [1]
2) Setup buildroot override via Bodhi [2]


[1] http://fedoraproject.org/wiki/PackageMaintainers/Join#Submit_Package_as_Update_in_Bodhi
[2] https://fedoraproject.org/wiki/Bodhi/BuildRootOverrides
Comment 50 Fedora Update System 2012-08-22 14:49:31 EDT
rubygem-openshift-origin-common-0.13.3-10.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/rubygem-openshift-origin-common-0.13.3-10.fc18
Comment 51 Fedora Update System 2012-08-23 00:35:29 EDT
rubygem-openshift-origin-common-0.13.3-10.fc18 has been pushed to the Fedora 18 testing repository.
Comment 52 Fedora Update System 2012-09-17 19:17:04 EDT
rubygem-openshift-origin-common-0.13.3-10.fc18 has been pushed to the Fedora 18 stable repository.