Bug 786636 - Review Request: rubygem-unicorn - Rack HTTP server for fast clients and Unix
Review Request: rubygem-unicorn - Rack HTTP server for fast clients and Unix
Status: CLOSED DUPLICATE of bug 1065685
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-01 20:03 EST by Guillermo Gómez
Modified: 2014-02-24 14:51 EST (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-02-15 15:43:51 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Guillermo Gómez 2012-02-01 20:03:07 EST
Spec URL: http://gomix.fedorapeople.org/rubygem-unicorn/rubygem-unicorn.spec
SRPM URL: http://gomix.fedorapeople.org/rubygem-unicorn/rubygem-unicorn-4.2.0-1.fc16.src.rpm
Description:

Unicorn is an HTTP server for Rack applications designed to only serve fast
clients on low-latency, high-bandwidth connections and take advantage of
features in Unix/Unix-like kernels. Slow clients should only be served by
placing a reverse proxy capable of fully buffering both the the request and
response in between Unicorn and slow clients

$ rpmlint -v SPECS/rubygem-unicorn.spec 
SPECS/rubygem-unicorn.spec: I: checking-url http://rubygems.org/downloads/unicorn-4.2.0.gem (timeout 10 seconds)
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint -v /var/lib/mock/fedora-16-x86_64/result/rubygem-unicorn-doc-4.2.0-1.fc16.x86_64.rpm 
rubygem-unicorn-doc.x86_64: I: checking
rubygem-unicorn-doc.x86_64: I: checking-url http://unicorn.bogomips.org/ (timeout 10 seconds)
rubygem-unicorn-doc.x86_64: W: unexpanded-macro /usr/lib/ruby/gems/1.8/doc/unicorn-4.2.0/ri/Unicorn/TeeInput/client_body_buffer_size%3d-c.yaml %3d
rubygem-unicorn-doc.x86_64: W: doc-file-dependency /usr/lib/ruby/gems/1.8/gems/unicorn-4.2.0/test/aggregate.rb /usr/bin/ruby
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

$ rpmlint -v /var/lib/mock/fedora-16-x86_64/result/rubygem-unicorn-4.2.0-1.fc16.x86_64.rpm 
rubygem-unicorn.x86_64: I: checking
rubygem-unicorn.x86_64: I: checking-url http://unicorn.bogomips.org/ (timeout 10 seconds)
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
Comment 1 Michael Scherer 2012-02-11 18:52:37 EST
The package doesn't seems to build in rawhide :
ruby(abi) = 1.8 
cannot be found.
Comment 2 Julian C. Dunn 2012-12-27 00:21:22 EST
Guillermo: Not sure if you're still working on this package or not, but I made a few changes to clean it up and upgrade to 4.5.0.

http://jdunn.fedorapeople.org/rubygem-unicorn/rubygem-unicorn.spec

Unfortunately, tests are failing at the moment. I've contacted upstream; if you can think of any reason why this would not work, let me know. The build log is at

http://jdunn.fedorapeople.org/rubygem-unicorn/build.log
Comment 3 Guillermo Gómez 2012-12-27 19:20:58 EST
(In reply to comment #2)
> Guillermo: Not sure if you're still working on this package or not, but I
> made a few changes to clean it up and upgrade to 4.5.0.
> 
> http://jdunn.fedorapeople.org/rubygem-unicorn/rubygem-unicorn.spec
> 
> Unfortunately, tests are failing at the moment. I've contacted upstream; if
> you can think of any reason why this would not work, let me know. The build
> log is at
> 
> http://jdunn.fedorapeople.org/rubygem-unicorn/build.log

Thanks Julian, im planning to keep working with it, just looking for the time.

I'll take a look at your spec, again thanks. But mostly as i remember, i need to clear the deps.
Comment 4 Julian C. Dunn 2012-12-27 21:54:32 EST
Cool, no problem.

I managed to work around the build problem. It's a namespace conflict when running under testrb, so I applied a patch to rename the test class. This SRPM and spec build cleanly in a Fedora 19 mock:

http://jdunn.fedorapeople.org/rubygem-unicorn/rubygem-unicorn.spec
http://jdunn.fedorapeople.org/rubygem-unicorn/rubygem-unicorn-4.5.0-2.fc19.src.rpm

Let me know what you think.
Comment 5 Vít Ondruch 2013-01-07 08:55:40 EST
(In reply to comment #4)
Not sure what is the issue with the test suite, but if you run the tests independently, there is no point to execute them using testrb. You should be able to run them by ruby itself.

Also, you can always omit testrb by using something like:

  $ ruby -e "Dir.glob('./test/**/*_test.rb').each {|t| require t}"

to execute the whole test suite at once.
Comment 6 Ken Dreyer 2013-02-20 19:28:26 EST
Thank you for carrying on this review. I'll need Unicorn for Gitorious.

Unfortunately, Ruby on RHEL 6 does not load binary gems from (what is defined on Fedora as) %{gem_extdir}, so we have to use %{ruby_sitearch} instead. Here's what I use for RHEL 6 compatibility in my arch-specific rubygem packages.

At the top, I have a list of macros like you have, and I add ruby_sitearch to the bottom of the list:

%{!?ruby_sitearch: %global ruby_sitearch %(ruby -rrbconfig -e 'puts Config::CONFIG["sitearchdir"]')}

Then, in %install, instead of copying the shared lib to gem_extdir, I use a dist tag conditional:

%if 0%{?el6}
# gem_extdir doesn't really work on EL 6
mkdir -p %{buildroot}%{ruby_sitearch}
# move the extension to ruby_sitearch
mv %{buildroot}%{gem_libdir}/unicorn_http.so %{buildroot}%{ruby_sitearch}
%else
mkdir -p %{buildroot}%{gem_extdir}/lib
# move the extension to gem_extdir
mv %{buildroot}%{gem_libdir}/unicorn_http.so %{buildroot}%{gem_extdir}/lib/
%endif

And lastly, in %files, I use a second dist tag conditional:

%if 0%{?el6}
%{ruby_sitearch}/unicorn_http.so
%else
%{gem_extdir}
%endif
Comment 7 Achilleas Pipinellis 2013-08-06 02:59:40 EDT
Hey Guillermo, are you still on to this? If not I'd like to take over, thanks.
Comment 8 Guillermo Gómez 2013-08-06 08:17:39 EDT
Actually i was working on it yesterday (casually). Im planning to upload new f19 spec/srpm today.
Comment 9 Guillermo Gómez 2013-08-06 08:52:00 EDT
Spec URL: http://gomix.fedorapeople.org/rubygem-unicorn/rubygem-unicorn.spec
SRPM URL: http://gomix.fedorapeople.org/rubygem-unicorn/rubygem-unicorn-4.6.3-1.fc19.src.rpm

$ rpmlint -v SPECS/rubygem-unicorn.spec 
SPECS/rubygem-unicorn.spec: I: checking-url http://rubygems.org/downloads/unicorn-4.6.3.gem (timeout 10 seconds)
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint -v /var/lib/mock/fedora-19-x86_64/result/rubygem-unicorn-4.6.3-1.fc19.x86_64.rpm 
rubygem-unicorn.x86_64: I: checking
rubygem-unicorn.x86_64: I: checking-url http://unicorn.bogomips.org/ (timeout 10 seconds)
rubygem-unicorn.x86_64: W: no-soname /usr/lib64/gems/ruby/unicorn-4.6.3/lib/unicorn_http.so
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

no-soname can be safely ignored afaik.

$ rpmlint -v /var/lib/mock/fedora-19-x86_64/result/rubygem-unicorn-doc-4.6.3-1.fc19.x86_64.rpm 
rubygem-unicorn-doc.x86_64: I: checking
rubygem-unicorn-doc.x86_64: W: spelling-error Summary(en_US) rdoc -> doc, r doc, Doctor
rubygem-unicorn-doc.x86_64: I: checking-url http://unicorn.bogomips.org/ (timeout 10 seconds)
rubygem-unicorn-doc.x86_64: W: doc-file-dependency /usr/share/gems/gems/unicorn-4.6.3/test/aggregate.rb /usr/bin/ruby
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

We can safely ignore those two warnings too.
Comment 10 Ken Dreyer 2013-11-25 21:02:53 EST
Hi Guillermo,I've made some adjustments to the spec file and pushed the changes to a Git repo, here:
http://fedorapeople.org/cgit/ktdreyer/public_git/rubygem-unicorn.git/log/

I've broken each change into a separate Git commit so you can review each one. I'll also try to explain each suggested change below.

1. use HTTPS in URL: This change changes the URL from using plaintext HTTP to secured HTTPS.

2. rm BuildRoot, %clean, and default defattr. There were a couple of older bits in the RPM that are not needed on EL6 and above.

3. adjust dist conditionals post F17. Since Fedora 17 is EOL, this commit removes references to it in the dist conditionals.

4. adjust whitespace. The Requires: ruby(release) field wasn't properly indented to align with the others, so this commit fixes that.

5. remove trailing whitespace. There was a lot of trailing whitespace in the spec file, so this commit fixes that.

6. update to 4.7.0. The upstream Unicorn project has released a new version this month, so I've updated the spec for this.

7. rm spurious dist tag conditional. There was an empty Fedora 18 dist tag conditional, and this commit removes it.

8. fix tests. This commit is a big one, and it might be a little controversial. It has two main changes: I've moved the test modifications to %prep, and I've changed the test invocation in %check to be simply "make test".

9. translate comment to english. This commit simply translates the comment about Unix permissions to english.

10. add comment about upstream fix for doc permissions. As it turns out the permissions problem was a bug in the wrongdoc gem. It was fixed upstream, so I contacted the Unicorn author and asked him to update to the lastest wrongdoc version. As it turns out, Eric was happy to do so, and the next version of Unicorn shouldn't have this permissions problem. I've added a link to the mailinglist discussion so that future maintainers will know "maybe this is fixed upstream and I can remove the chmod statement."

11. require rubygems in test helper. Since each of the tests load test_helper, it's simpler to require rubygems in the test helper rather than adding the rubygems line to each test file individually.

12. use simpler sed commands to fix shebang. So, this one might be a little controversial as well, but I think that it's simpler/cleaner to avoid grepping for upstream's strange shebang line and just use these sed functions instead.

13. build doc subpackage as noarch. The -doc subpackage should be set to "noarch" since there's no architecture-specific files within it.

14. filter test/doc files from auto-requires. If you look at the --requires on the -doc subpackage, you'll see that some weird things like /usr/bin/ruby, /bin/sh, /usr/bin/env get set. When we use the __requires_exclude_from macro, we can skip scanning the files that we ship in -doc.


What do you think?
Comment 11 Ken Dreyer 2013-11-26 19:32:18 EST
Hi Guillermo, I've pushed a couple more patches to Git:

15. use gem_docdir macro. This commit uses the gem_docdir macro in the %files list instead of manually specifying the same thing.

16. add missing dependency on rack. When testing this on an EL6 box, I found that I needed to have rubygem-rack installed. This commit adds the rack gem to Requires.

17. require rubygems during binary invocation. I found that I needed to have "require 'rubygems'" to make unicorn work properly on EL6.

18. add macros for el6. On RHEL 6 prior to 6.5, many of the modern Ruby and Gem RPM macros are not available. This commit conditionally defines these macros if they happen to be missing.

19. add EL6 conditionals. This commit allows the RPM to be built for EL6. It uses the ruby_sitearch technique I described in comment 6. With this commit, I can successfully run /usr/bin/unicorn on EL6.

Git repo:
http://fedorapeople.org/cgit/ktdreyer/public_git/rubygem-unicorn.git/log/
Comment 12 Guillermo Gómez 2013-12-05 06:38:10 EST
Thanks Ken for your contributions :), i will make changes an update my spec for f20 and look for a reviewer (probably today). Package yet needs to be approved.
Comment 13 Guillermo Gómez 2013-12-05 07:21:10 EST
(In reply to Vít Ondruch from comment #5)
> (In reply to comment #4)
> Not sure what is the issue with the test suite, but if you run the tests
> independently, there is no point to execute them using testrb. You should be
> able to run them by ruby itself.
> 
> Also, you can always omit testrb by using something like:
> 
>   $ ruby -e "Dir.glob('./test/**/*_test.rb').each {|t| require t}"
> 
> to execute the whole test suite at once.

Well yes, that's ideal, but the point of running them independently is to be able to avoid a particular test and make the packaging succeed, no need sometimes to force every single test to pass for packaging needs, perhaps there are non sense test for our environments. This is also a result instead of failing tests previously, so also worked for me to isolate things. BTW thanks for the great tips :)

I keep using testrb just as a good practice and consistency and readability, hope this is not a blocker.
Comment 14 Vít Ondruch 2013-12-05 07:56:32 EST
Well, have many repeating lines has nothing to do with readability. This applies to the sed lines as well as the testrb lines.

If you want to disable single file, it is better to disable the one file then run them separately.

Moreover, since you run them one by one, you might miss newly introduced file when package gets updated.

Actually, I am afraid that although the first test may fail, the build will successful, which is another unexpected result of running the tests file by file.
Comment 15 Guillermo Gómez 2013-12-06 07:40:21 EST
Well, i'm not just repeating exactly the same line over and over (just for people not viewing the code in which case that would just stupid, basically the reason for me to comment) , that's not repeating. Using a block like you suggested is great for repeating the same action over many files, and i'm working on improving the actual spec.

And yes, thanks, i'm also aware about the weakness of the actual testing scenario im using. Again, will improve asap (im on it).

I'm reviewing the make test option Ken mentioned and merging many changes he has proposed. I'm not planning on EPEL releases yet.
Comment 16 Ken Dreyer 2013-12-11 15:26:57 EST
Hi Guillermo,

I noticed something else that can be changed in the packaging today.

20. skip .gitkeep removals. This commit removes the "find" + "rm" command that deletes the .gitkeep files. Since upstream no longer ships .gitkeep files in 4.7.0, there's no need to search and remove them in the RPM packaging.

Git repo:
http://fedorapeople.org/cgit/ktdreyer/public_git/rubygem-unicorn.git/log/
Comment 17 Ken Dreyer 2014-02-10 17:58:16 EST
Hi Guillermo, just checking to see if you've had a chance to review those patches and merge them into your unicorn package? It would be great to get unicorn into Fedora.

I've updated for the latest Unicorn release (4.8.2):

http://fedorapeople.org/cgit/ktdreyer/public_git/rubygem-unicorn.git/commit/?id=480d0473ce4b40bca0137d50f50df9170f6f1abf
Comment 18 Ken Dreyer 2014-02-15 15:43:51 EST
I've updated the package to drop the Fedora 18 conditionals, since that release is EOL now.

http://fedorapeople.org/cgit/ktdreyer/public_git/rubygem-unicorn.git/commit/?id=a9edc7c4b789758834f3c0fd285fab438b720ea5

New package review request at bug 1065685

*** This bug has been marked as a duplicate of bug 1065685 ***
Comment 19 Guillermo Gómez 2014-02-19 06:55:12 EST
I was about today to work on this. Nobody reviewed the bug during the history of it, it was always an open discussion. Closing it this way is just rude. That's not the Fedora way to do things.

If Ken was so interested on getting Unicorn on Fedora, he could just take it for review and work with me for getting it done.

I will pass on it and let it go, but please, Ken, just take note that's not proper way to proceed.
Comment 20 Guillermo Gómez 2014-02-24 14:51:31 EST
Please be this comment just to note that im ok with the new review and Ken and I talked about it off line and we are ok and in good terms. :)

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