This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours

Bug 839395

Summary: Review Request: rubygem-openshift-origin-controller - Rails engine for the OpenShift Broker API
Product: [Fedora] Fedora Reporter: Brenton Leanhardt <bleanhar>
Component: Package ReviewAssignee: Vít Ondruch <vondruch>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: d.g.cameron, misc, notting, package-review, tdawson, vondruch
Target Milestone: ---Flags: vondruch: 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 18:41:28 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On: 839064    
Bug Blocks: 842891, 844013    

Description Brenton Leanhardt 2012-07-11 16:06:08 EDT
Spec URL: http://brenton.fedorapeople.org/package_reviews/rubygem-stickshift-controller/rubygem-stickshift-controller.spec
SRPM URL: http://brenton.fedorapeople.org/package_reviews/rubygem-stickshift-controller/rubygem-stickshift-controller-0.13.13-1.src.rpm
Description: This packages contains a Rails engine that provides the majority of the OpenShift Broker API.
Fedora Account System Username: brenton

Hi, this is another package to help get OpenShift Origin into Fedora 18: https://fedoraproject.org/wiki/Features/OpenShift_Origin.

This package containts a Rails engine that exposes the Broker API.  It's worth noting that I am a developer on the OpenShift team and can help make changes if needed.
Comment 2 David Cameron 2012-07-18 07:21:47 EDT
Hi,

Here is a first (informal) review:

1. The %globals in the spec file expect ruby to be already installed. Please use the pre-defined macros instead:

 http://fedoraproject.org/wiki/Packaging:Ruby#Macros

This leads to the following rpmlint errors:

rpmlint rubygem-stickshift-controller-0.14.1-1.git.26.d123083.fc18.src.rpm

rubygem-stickshift-controller.src: E: specfile-error sh: ruby: command not found
rubygem-stickshift-controller.src: E: specfile-error sh: ruby: command not found
rubygem-stickshift-controller.src: E: specfile-error sh: ruby: command not found
1 packages and 0 specfiles checked; 3 errors, 0 warnings.

2. The changelog needs to match the release:

 rubygem-stickshift-controller.noarch: W: incoherent-version-in-changelog 0.14.1-1 ['0.14.1-1.git.26.d123083.fc1
8', '0.14.1-1.git.26.d123083']

3. Other rpmlint errors:

rubygem-stickshift-controller.noarch: W: no-documentation
rubygem-stickshift-controller.noarch: E: script-without-shebang /builddir/.gem/ruby/1.9.1/gems/stickshift-controller-0.14.1/lib/stickshift-controller/app/models/application.rb
rubygem-stickshift-controller.noarch: E: script-without-shebang /builddir/.gem/ruby/1.9.1/gems/stickshift-controller-0.14.1/lib/stickshift-controller/app/models/gear.rb
rubygem-stickshift-controller.noarch: E: non-executable-script /builddir/.gem/ruby/1.9.1/gems/stickshift-controller-0.14.1/test/ddns/named_service.rb 0644L /usr/bin/ruby

4. The spec file linked in the original description is different from the spec in the 0.14.1-1 SRPM.
Comment 5 David Cameron 2012-07-19 06:26:22 EDT
It looks like problems 1 and 3 are fixed, but the changelog version warning is still there, and the spec file inside the new srpm is different from the link in comment 3 - the Source0 tag gives a warning

rubygem-stickshift-controller.src: W: invalid-url Source0: rubygem-stickshift-controller-git-73.1915eff.tar.gz

(the URL is ok in the standalone spec).

The new entries in the changelog also throw up new warnings:

rubygem-stickshift-controller.src:70: W: macro-in-%changelog %gemname
rubygem-stickshift-controller.src:70: W: macro-in-%changelog %gem_name
rubygem-stickshift-controller.src:70: W: macro-in-%changelog %geminstdir

I guess you want to escape those with %%.

As for the unexpanded macro warnings, this seems to be a feature related to ruby packages and they are safe to ignore.

One more thing - the LICENSE file should be included in %doc

http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
Comment 7 Michael Scherer 2012-08-02 17:16:58 EDT
Is this package intended to go to EPEL 5 ?
If not, there is a serie of item to clean ( %defattr, %clean, rm -Rf %buildroot in %install, BuildRoot tag ).

Fedora review also complain about :

- [!]: MUST Gem package must exclude cached Gem.
See: http://fedoraproject.org/wiki/Packaging:Ruby

but I do not see where this is written. I think that's this :
%{gem_dir}/cache/%{gem_name}-%{version}.gem

and indeed, if the code is already shipped outside of the gem, no need to bundle it a 2nd time.

- Regarding the various requirement :
rubygem(highline) 
rubygem(json_pure)
rubygem(parseconfig)

are not needed, since they do not seems to be used in the code. 

- I think rubygem(rails) is needed, since common do not requires it, and the controller code use it.
Comment 9 Brenton Leanhardt 2012-08-06 17:20:38 EDT
Opps, please ignore those links.  They are for rubygem-openshift-origin-node.
Comment 10 Brenton Leanhardt 2012-08-06 17:28:46 EDT
Here are the correct links:

fedora-review: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/201208061717/rubygem-openshift-origin-controller-review.txt

SRPM: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/201208061717/rubygem-openshift-origin-controller-0.14.15-3.fc18.src.rpm

Spec: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/201208061717/rubygem-openshift-origin-controller.spec

Right now there is a 'script without shebang' error reported from rpmlint.  I don't have direct control over the tarball source that is hosted on the OpenShift mirror so I'll ask for that to be fixed tomorrow.  The same goes for the 'non-executable-script' error.
Comment 13 Brenton Leanhardt 2012-08-16 16:23:30 EDT
I went ahead and updated this package to the latest standards that were approved in my other two packages.

* Thu Aug 16 2012 Brenton Leanhardt <bleanhar@redhat.com> - 0.14.15-6
- Using rubygem-devel macros
- Removed open4, and json requirement and patched gemspec.  These were used
  only for tests.
- Remove unneeded activemodel require

SRPM: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/2012080161533/rubygem-openshift-origin-controller-0.14.15-6.fc18.src.rpm

Spec: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/2012080161533/rubygem-openshift-origin-controller.spec
Comment 14 Vít Ondruch 2012-08-17 04:04:55 EDT
I'll take it for a review.
Comment 15 Vít Ondruch 2012-08-17 04:10:34 EDT
* ruby-devel
  - You probably don't need this dependency, since this is pure Ruby gem.

* Patching after gem build
  - I am not sure I understand the purpose of this patch, or better why to
    patch after you build the gem?

* Use gem packaging macros
  - In %install section, please replace
    %{gem_dir}/gems/%{gem_name}-%{version}/lib => %{gem_libdir}
    %{gem_dir}/gems/%{gem_name}-%{version} => %{gem_instdir}

* Do not %exclude %{gem_libdir}

* Mark only documentation as a %doc
  - i.e. Gemfile, Rakefile, %{gem_name}.gemspec should not be marked as a %doc

* Please consider splitting documentation into -doc subpackage.
  - BTW are you aware of gem2rpm tool? This would create the -doc subpackage
    for you and much more.

* Please consider execution of test suite

* Please keep the documentation in the package
  - There is no reason to remove the test suite IMO.
  - In case you created the -doc subpackage, the test suite should be moved
    there.

* CONFIGURE_ARGS is not needed
  - This is needed just for binary gems.
Comment 16 Brenton Leanhardt 2012-08-17 10:27:01 EDT
Regarding %patch1, that was another mistake.  I had incorrectly ported a change that was recommended in http://fedoraproject.org/wiki/Packaging:Ruby.  There the following is used in prep:

gem spec %{SOURCE0} -l --ruby > %{gem_name}.gemspec

I believe that command creates whitespace differences that causes gemspec patches to fail (if they were was created against the original source).  That meant if I needed to patch the gemspec I had to patch it after that command.

If that line isn't absolutely necessary I would like to avoid it. Currently I believe (fixed) prep section is much more readable:

gem unpack %{SOURCE0}
%setup -q -D -T -n  %{gem_name}-%{version}
%patch0 -p1
%patch1 -p1

Right now there are unfortunately no real unit tests for this package (even though they exist in test/unit).  They are all integration style tests that have external dependencies.

I would like to create -doc subpackages for all OpenShift libraries once the dust settles a little and the packagers involved have time to standardize on decisions like this.

Here are the updated artifacts:

* Fri Aug 17 2012 Brenton Leanhardt <bleanhar@redhat.com> - 0.14.15-7
- Fixed prep and build sections
- Removed ruby-devel BuildRequire
- Removed useless CONFIGURE_ARGS variable
- Proper usage of gem_libdir and gem_instdir in install section
- Correctly incorrect usage of doc directive and fixed incorrect gem_libdir
  exclusion

SRPM: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/201208170939/rubygem-openshift-origin-controller-0.14.15-7.fc18.src.rpm

Spec: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/201208170939/rubygem-openshift-origin-controller.spec
Comment 17 Michael Scherer 2012-08-19 05:33:36 EDT
Brenton is a packager now, ( https://bugzilla.redhat.com/show_bug.cgi?id=839064#c40 ), so remove block on fe-needsponsor
Comment 18 Vít Ondruch 2012-08-20 08:00:38 EDT
(In reply to comment #16)
> Regarding %patch1, that was another mistake.  I had incorrectly ported a
> change that was recommended in http://fedoraproject.org/wiki/Packaging:Ruby.
> There the following is used in prep:
> 
> gem spec %{SOURCE0} -l --ruby > %{gem_name}.gemspec
> 
> I believe that command creates whitespace differences that causes gemspec
> patches to fail (if they were was created against the original source). 
> That meant if I needed to patch the gemspec I had to patch it after that
> command.
> 
> If that line isn't absolutely necessary I would like to avoid it. Currently
> I believe (fixed) prep section is much more readable:
> 
> gem unpack %{SOURCE0}
> %setup -q -D -T -n  %{gem_name}-%{version}
> %patch0 -p1
> %patch1 -p1

You took dangerous path. Please don't recreate the gem using upstream .gemspec file and use the one created by "gem spec" command. If you compare these two, you'll be surprised how different they look.

Moreover, looking into the patches:

* I would drop the Patch0 entirely, since I don't see any real benefit to carry it along.
* I would drop the Patch1, since it changes (1) development dependencies (2) they are not satisfied in Fedora anyway.
Comment 19 Brenton Leanhardt 2012-08-20 09:40:24 EDT
I'm not sure I completely understand your stance on patch1.  Are you saying that no changes to the gemspec are needed?  Currently the Gemfile pulls all gem dependencies from the gemspec.  If the gemspec isn't patched to label the test dependencies as development only I believe the library will fail to load.

I'm fine with dropping patch0 though.  It was a recommendation by another reviewer and I've already resolved the issue upstream.

I am curious about the dangers of using the upstream gemspec.  If there is any documentation on this I would be happy to read through it!
Comment 20 Vít Ondruch 2012-08-20 10:48:29 EDT
(In reply to comment #19)
> I'm not sure I completely understand your stance on patch1.  Are you saying
> that no changes to the gemspec are needed?  Currently the Gemfile pulls all
> gem dependencies from the gemspec.  If the gemspec isn't patched to label
> the test dependencies as development only I believe the library will fail to
> load.

Ah, sorry ... I overlooked that. You are right.

> I am curious about the dangers of using the upstream gemspec.  If there is
> any documentation on this I would be happy to read through it!

Well, there is no documentation, except my argument with FPC [1, 2]. But gaining more experiences with this approach, I am still firmly convinced (unless [3] unveils something unexpected) that repackaging of gems is wrong. 

My biggest concern with regards of you package is, that you actually cannot know what files will appear in the resulting .gem.

For example, if you do "rpmbuild -bb your.spec" followed by "rpmbuild -bp your.spec", you'll see, that the BUILD directory contains bunch of files comming from the gem and among them, there is "usr" directory for example. But there might be other files as well.

If you take the .gempsec produced by the "gem spec" command, there is at least already expanded list of all files which were contained in the original gem,  (in contrary to upstream's .gemspc, where is used some globbing) therefore lower chance, that some arbitrary file will endup in the repackaged gem.

Somebody, who will take you example my fail, because upstream used "git ls-files" to populate the files list, but the gem does not contain the git metadata.

Another problem might be, that the upstream's .gemspec contains some arbitrary code. On the other hand, the .gemspec produced by "gem spec" command was undergone transformation into YAML, so there cannot be everything.

So to conclude, it might work in your hands, but it can be "disaster" in others hands. Therefore I discourage this practice.

[1] https://fedorahosted.org/fpc/ticket/134
[2] http://lists.fedoraproject.org/pipermail/packaging/2012-February/008192.html
[3] http://lists.fedoraproject.org/pipermail/ruby-sig/2012-August/001088.html
Comment 21 Brenton Leanhardt 2012-08-20 10:51:27 EDT
That makes sense.  I really appreciate the explanation.  I'll have a new release of the SRPM/Spec shortly.
Comment 22 Brenton Leanhardt 2012-08-20 11:25:58 EDT
* Mon Aug 20 2012 Brenton Leanhardt <bleanhar@redhat.com> - 0.14.15-8
- Properly recreate the gemspec with 'gem spec'
- Dropped upstreamed openshift-origin-controller_gemspec_fixes.patch

SRPM: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/201208201121/rubygem-openshift-origin-controller-0.14.15-8.fc18.src.rpm

Spec: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/201208201121/rubygem-openshift-origin-controller.spec
Comment 23 Vít Ondruch 2012-08-21 02:50:44 EDT
Although the package looks good, it does not work:

$ irb
irb(main):001:0> require 'stickshift-controller'
LoadError: cannot load such file -- openshift-origin-common
	from /usr/share/rubygems/rubygems/custom_require.rb:36:in `require'
	from /usr/share/rubygems/rubygems/custom_require.rb:36:in `require'
	from /usr/share/gems/gems/openshift-origin-controller-0.14.15/lib/stickshift-controller.rb:1:in `<top (required)>'
	from /usr/share/rubygems/rubygems/custom_require.rb:60:in `require'
	from /usr/share/rubygems/rubygems/custom_require.rb:60: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>'

$ gem list

*** LOCAL GEMS ***

activemodel (3.2.8)
activesupport (3.2.8)
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)
openshift-origin-controller (0.14.15)
rdoc (3.12)
state_machine (1.1.2)

May be upstream should finish the rename at first place. It is not the best practice to "require 'stickshift-controller'" when you want to use openshift-origin-controller gem.

BTW I used openshift-origin-common with fix I requested in https://bugzilla.redhat.com/show_bug.cgi?id=839064#c46

Also, it would be nice to import and build openshift-origin-common for Fedora first, which would make the testing easier.
Comment 24 Michael Scherer 2012-08-21 04:16:42 EDT
+1 to push common, as this block 5 packages from being built and submitted.
Comment 25 Brenton Leanhardt 2012-08-21 09:46:26 EDT
rubygem-openshift-origin-common has been built for rawhide and f18 with the requested fix.
Comment 26 Brenton Leanhardt 2012-08-21 12:01:58 EDT
%changelog
* Tue Aug 21 2012 Brenton Leanhardt <bleanhar@redhat.com> - 0.14.15-9
- Adding rubygem-mongo require
- Patching openshift-origin-common load

SRPM: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/201208211154/rubygem-openshift-origin-controller-0.14.15-9.fc18.src.rpm

Spec: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/201208211154/rubygem-openshift-origin-controller.spec

I included a comment in the spec stating that this patch is Fedora specific right now since we're in the middle of the rename.  The actual rename will be done upstream once all the packages are accepted in Fedora (since Fedora suggested the rename).
Comment 27 Troy Dawson 2012-08-21 13:49:48 EDT
Vit,
I was the one who did the upstream renaming wrong, but I believe I have a solution.
Can you test the following and see if they pass your tests?

http://tdawson.fedorapeople.org/openshift-origin-fix/rubygem-openshift-origin-common-0.13.3-10.fc18.noarch.rpm
http://tdawson.fedorapeople.org/openshift-origin-fix/rubygem-openshift-origin-controller-0.14.15-10.fc18.noarch.rpm

http://tdawson.fedorapeople.org/openshift-origin-fix/

I have renamed the ruby libraries to their new openshift-origin names  (openshift-origin-common and openshift-origin-controller).  But I have kept the paths the same.  Changing the paths at this point will break too many things.

I have done some basic tests and it works for me.  But I am not a ruby expert and I might have missed some things.

If these work, I'll work on getting the other openshift rubygems fixed up.
Comment 28 Vít Ondruch 2012-08-22 07:32:44 EDT
(In reply to comment #27)
Well that was my suggestion towards upstream. I don't think we should take any action in Fedora in that direction and we should wait until new upstream gem release will be available.

It is probably not good idea to have in Fedora gem, which claims it is 0.14.15 version, but it differs in behavior from upstream gem.

(In reply to comment #26)
Brenton,

* I see that you add a Require: rubygem(mongo)? Shouldn't it be referenced in the .gemspec file as well? Shouldn't it be reported upstream? The .gemspec dependencies should be aligned with .spec requires, if there is no special reason.


Nevertheless, this is minor issue and the package looks good now => APPROVE. Brenton, please clarify with upstream the mongo requirements before import.
Comment 29 Brenton Leanhardt 2012-08-22 08:31:27 EDT
Thanks for the help.  I'll patch the gemspec and submit a pull request for upstream.
Comment 30 Brenton Leanhardt 2012-08-22 08:34:34 EDT
New Package SCM Request
=======================
Package Name: rubygem-openshift-origin-controller
Short Description: Rails engine for the OpenShift Broker API
Owners: brenton tdawson maxamillion
Branches: f17 f18
InitialCC:
Comment 31 Jon Ciesla 2012-08-22 09:04:14 EDT
Git done (by process-git-requests).
Comment 32 Fedora Update System 2012-08-22 14:51:18 EDT
rubygem-openshift-origin-controller-0.14.15-11.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/rubygem-openshift-origin-controller-0.14.15-11.fc18
Comment 33 Fedora Update System 2012-08-23 00:35:47 EDT
rubygem-openshift-origin-controller-0.14.15-11.fc18 has been pushed to the Fedora 18 testing repository.
Comment 34 Fedora Update System 2012-09-17 18:41:28 EDT
rubygem-openshift-origin-controller-0.14.15-11.fc18 has been pushed to the Fedora 18 stable repository.