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 Review | Assignee: | Vít Ondruch <vondruch> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | d.g.cameron, misc, notting, package-review, tdawson, vondruch |
Target Milestone: | --- | Flags: | vondruch:
fedora-review+
gwync: 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 22:41:28 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | 839064 | ||
Bug Blocks: | 842891, 844013 |
Description
Brenton Leanhardt
2012-07-11 20:06:08 UTC
The latest SRPM can be found here: http://brenton.fedorapeople.org/package_reviews/rubygem-stickshift-controller/rubygem-stickshift-controller-0.14.1-1.src.rpm 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. Thanks for the help. Here are links to the updated artifacts: http://brenton.fedorapeople.org/package_reviews/rubygem-stickshift-controller/0.14.2-1.git.73.1915eff/rubygem-stickshift-controller-0.14.2-1.git.73.1915eff.fc17.noarch.rpm http://brenton.fedorapeople.org/package_reviews/rubygem-stickshift-controller/0.14.2-1.git.73.1915eff/rubygem-stickshift-controller-0.14.2-1.git.73.1915eff.rpmlint.out http://brenton.fedorapeople.org/package_reviews/rubygem-stickshift-controller/0.14.2-1.git.73.1915eff/rubygem-stickshift-controller.spec I put a description of the changes in the changelog. I incorrectly linked to the binary RPM. Here's the SRPM: http://brenton.fedorapeople.org/package_reviews/rubygem-stickshift-controller/0.14.2-1.git.73.1915eff/rubygem-stickshift-controller-0.14.2-1.git.73.1915eff.fc17.src.rpm 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 You were right about needing to escape the macros names in the changelog! Here's the updated artifacts. I changed the Source0 line to use the gem and updated the %prep, %build and %install stanzas accordingly. http://brenton.fedorapeople.org/package_reviews/rubygem-stickshift-controller/201207201705/rubygem-stickshift-controller-0.14.4-1.fc18.src.rpm http://brenton.fedorapeople.org/package_reviews/rubygem-stickshift-controller/201207201705/rubygem-stickshift-controller.rpmlint.out http://brenton.fedorapeople.org/package_reviews/rubygem-stickshift-controller/201207201705/rubygem-stickshift-controller.spec 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. Actually another RPM rubygem-openshift-origin-broker will have the rails dependency. That RPM contains the actual application that uses the controller and common. Here are the latest fixes: fedora-reivew: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-node/201208061658/rubygem-openshift-origin-node-review.txt SRPM: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-node/201208061658/rubygem-openshift-origin-node-0.14.6-2.fc18.src.rpm Spec: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-node/201208061658/rubygem-openshift-origin-node.spec Opps, please ignore those links. They are for rubygem-openshift-origin-node. 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. fedora-review: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/201208071302/rubygem-openshift-origin-controller-review.txt SRPM: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/201208071302/rubygem-openshift-origin-controller-0.14.15-4.fc18.src.rpm Spec: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/201208071302/rubygem-openshift-origin-controller.spec I linked to the pull request for the fix. I'll make sure it gets merged in. This latest update patches the gemspec homepage to match the URL in the spec. SRPM: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/201208101436/rubygem-openshift-origin-controller-0.14.15-5.fc16.src.rpm Spec: http://brenton.fedorapeople.org/package_reviews/rubygem-openshift-origin-controller/201208101436/rubygem-openshift-origin-controller.spec 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> - 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 I'll take it for a review. * 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. 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> - 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 Brenton is a packager now, ( https://bugzilla.redhat.com/show_bug.cgi?id=839064#c40 ), so remove block on fe-needsponsor (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. 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! (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 That makes sense. I really appreciate the explanation. I'll have a new release of the SRPM/Spec shortly. * Mon Aug 20 2012 Brenton Leanhardt <bleanhar> - 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 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. +1 to push common, as this block 5 packages from being built and submitted. rubygem-openshift-origin-common has been built for rawhide and f18 with the requested fix. %changelog * Tue Aug 21 2012 Brenton Leanhardt <bleanhar> - 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). 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. (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. Thanks for the help. I'll patch the gemspec and submit a pull request for upstream. 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: Git done (by process-git-requests). 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 rubygem-openshift-origin-controller-0.14.15-11.fc18 has been pushed to the Fedora 18 testing repository. rubygem-openshift-origin-controller-0.14.15-11.fc18 has been pushed to the Fedora 18 stable repository. |