Bug 1065685 - 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: ASSIGNED
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Achilleas Pipinellis
Fedora Extras Quality Assurance
:
: 786636 1268354 (view as bug list)
Depends On: 1096996
Blocks:
  Show dependency treegraph
 
Reported: 2014-02-15 15:43 EST by Ken Dreyer
Modified: 2016-04-29 20:23 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
axilleas: fedora‑review?


Attachments (Terms of Use)

  None (edit)
Description Ken Dreyer 2014-02-15 15:43:33 EST
Spec URL: http://ktdreyer.fedorapeople.org/reviews/rubygem-unicorn.spec
SRPM URL: http://ktdreyer.fedorapeople.org/reviews/rubygem-unicorn-4.8.2-2.fc21.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.
Fedora Account System Username: ktdreyer

F21 scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6533905
Comment 1 Ken Dreyer 2014-02-15 15:43:51 EST
*** Bug 786636 has been marked as a duplicate of this bug. ***
Comment 2 Achilleas Pipinellis 2014-03-01 07:02:57 EST
Ok, here are some comments.

- Use `cp -pa` to preserve timestamps, currently you do `cp -a`.

- rpmlint complains about: `No known owner of /usr/share/gems/gems/unicorn-4.8.2/bin, /usr/lib64/gems/ruby/unicorn-4.8.2`. Could it be that you should also include %{gem_instdir}/bin/ and %{gem_extdir_mri}/lib under %files as well? Actually, I haven't really understood how the dir ownership is defined. If I put a folder under %files it includes all its subfolders as well, right?

- You don't exclude %{gem_cache}.
 
- rpmlint complains with `W: no-soname /usr/lib64/gems/ruby/unicorn-4.8.2/lib/unicorn_http.so`. I have no idea what this means and `rpmlint -I` doesn't give any info. Safe to ignore?

- Minor but fixable :) `W: mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 97)`

- E: specfile-error -e:1:in `<main>': Use RbConfig instead of obsolete and deprecated Config. Epel6 uses ruby1.8 which I assume uses Config so it's safe to ignore?
Comment 3 Achilleas Pipinellis 2014-03-01 07:15:21 EST
About my second query the answer is here [0]. So you must include the top level directory which will include the entire tree below it. That addresses to `%{gem_extdir_mri}/*`. 

In my previous comment I missed that you don't include `%dir %{gem_instdir}`. That would solve the `No known owner of /usr/share/gems/gems/unicorn-4.8.2/bin` issue. Then i guess you could also omit the `%{gem_instdir}/bin/unicorn` and
`%{gem_instdir}/bin/unicorn_rails` macros.

Finally, you include Rakefile in -doc. I've seen that you usually remove it, so I just point that in case you missed it.


[0] https://fedoraproject.org/wiki/Packaging:UnownedDirectories#Wildcarding_Files_inside_a_Created_Directory
Comment 4 Vít Ondruch 2014-03-12 10:08:49 EDT
(In reply to Achilleas Pipinellis from comment #3)
> About my second query the answer is here [0]. So you must include the top
> level directory which will include the entire tree below it. That addresses
> to `%{gem_extdir_mri}/*`. 

Actually, it should be %{gem_extdir_mri} to own the directory and its content. By specifying %{gem_extdir_mri}/* you says you own just the content, but not the directory itself.

> In my previous comment I missed that you don't include `%dir
> %{gem_instdir}`. That would solve the `No known owner of
> /usr/share/gems/gems/unicorn-4.8.2/bin` issue. Then i guess you could also
> omit the `%{gem_instdir}/bin/unicorn` and
> `%{gem_instdir}/bin/unicorn_rails` macros.

No, that is wrong. The %dir directive says that only the specified directory is owned, not its content. If you check the files section closely, only the files under %{gem_instdir}/bin/ are explicitly owned, but the directory itself is not.

To solve this, either add "%dir %{gem_instdir}/bin/" or remove the explicitly specified content of bin dir. The former is more secure, since you have better control over the bindir content.
Comment 5 Achilleas Pipinellis 2014-03-12 10:58:31 EDT
(In reply to Vít Ondruch from comment #4)
> (In reply to Achilleas Pipinellis from comment #3)
> > About my second query the answer is here [0]. So you must include the top
> > level directory which will include the entire tree below it. That addresses
> > to `%{gem_extdir_mri}/*`. 
> 
> Actually, it should be %{gem_extdir_mri} to own the directory and its
> content. By specifying %{gem_extdir_mri}/* you says you own just the
> content, but not the directory itself.
> 

Oops, I see where you got confused. When I wrote "That addresses
to `%{gem_extdir_mri}/*`" I was referring to the existing macro in the spec. Not what it should be.

> > In my previous comment I missed that you don't include `%dir
> > %{gem_instdir}`. That would solve the `No known owner of
> > /usr/share/gems/gems/unicorn-4.8.2/bin` issue. Then i guess you could also
> > omit the `%{gem_instdir}/bin/unicorn` and
> > `%{gem_instdir}/bin/unicorn_rails` macros.
> 
> No, that is wrong. The %dir directive says that only the specified directory
> is owned, not its content. If you check the files section closely, only the
> files under %{gem_instdir}/bin/ are explicitly owned, but the directory
> itself is not.
> 
> To solve this, either add "%dir %{gem_instdir}/bin/" or remove the
> explicitly specified content of bin dir. The former is more secure, since
> you have better control over the bindir content.

Thanks for the clarification. If I understand correctly, the right entry would be 

%dir %{gem_instdir}/bin/
%{gem_instdir}/bin/unicorn
%{gem_instdir}/bin/unicorn_rails

or equally

%{gem_instdir}/bin/

right?
Comment 6 Vít Ondruch 2014-03-12 11:03:44 EDT
(In reply to Achilleas Pipinellis from comment #5)
> (In reply to Vít Ondruch from comment #4)
> > (In reply to Achilleas Pipinellis from comment #3)
> > > About my second query the answer is here [0]. So you must include the top
> > > level directory which will include the entire tree below it. That addresses
> > > to `%{gem_extdir_mri}/*`. 
> > 
> > Actually, it should be %{gem_extdir_mri} to own the directory and its
> > content. By specifying %{gem_extdir_mri}/* you says you own just the
> > content, but not the directory itself.
> > 
> 
> Oops, I see where you got confused. When I wrote "That addresses
> to `%{gem_extdir_mri}/*`" I was referring to the existing macro in the spec.
> Not what it should be.
> 

Hopefully Ken get the point ;)

> > > In my previous comment I missed that you don't include `%dir
> > > %{gem_instdir}`. That would solve the `No known owner of
> > > /usr/share/gems/gems/unicorn-4.8.2/bin` issue. Then i guess you could also
> > > omit the `%{gem_instdir}/bin/unicorn` and
> > > `%{gem_instdir}/bin/unicorn_rails` macros.
> > 
> > No, that is wrong. The %dir directive says that only the specified directory
> > is owned, not its content. If you check the files section closely, only the
> > files under %{gem_instdir}/bin/ are explicitly owned, but the directory
> > itself is not.
> > 
> > To solve this, either add "%dir %{gem_instdir}/bin/" or remove the
> > explicitly specified content of bin dir. The former is more secure, since
> > you have better control over the bindir content.
> 
> Thanks for the clarification. If I understand correctly, the right entry
> would be 
> 
> %dir %{gem_instdir}/bin/
> %{gem_instdir}/bin/unicorn
> %{gem_instdir}/bin/unicorn_rails
> 
> or equally
> 
> %{gem_instdir}/bin/
> 
> right?

Indeed
Comment 7 Ken Dreyer 2014-03-13 18:46:25 EDT
Thanks very much for the review!

(In reply to Achilleas Pipinellis from comment #2)
> - Use `cp -pa` to preserve timestamps, currently you do `cp -a`.

It's my understanding that "-a" implies "-p".

> - rpmlint complains about: `No known owner of
> /usr/share/gems/gems/unicorn-4.8.2/bin, /usr/lib64/gems/ruby/unicorn-4.8.2`.

Good catch, thanks. I've gone with "%{gem_instdir}/bin" in -3.

I've also removed the asterisk after gem_extdir_mri, so the package ships /usr/lib64/gems/ruby/unicorn-4.8.2 now.

> - You don't exclude %{gem_cache}.

Good catch, thanks. Fixed in -3.

> - rpmlint complains with `W: no-soname
> /usr/lib64/gems/ruby/unicorn-4.8.2/lib/unicorn_http.so`. I have no idea what
> this means and `rpmlint -I` doesn't give any info. Safe to ignore?

I've seen this on all the C-based Ruby extensions, so I think it's safe to ignore.

> - Minor but fixable :) `W: mixed-use-of-spaces-and-tabs (spaces: line 4,
> tab: line 97)`

Good catch, thanks. I've replaced the tabs with spaces. Fixed in -3.

> - E: specfile-error -e:1:in `<main>': Use RbConfig instead of obsolete and
> deprecated Config. Epel6 uses ruby1.8 which I assume uses Config so it's
> safe to ignore?

IMHO it's ok to ignore this since that macro only gets used on EL6.

Here is the new version of rubygem-unicorn:

Spec URL: http://ktdreyer.fedorapeople.org/reviews/rubygem-unicorn.spec
SRPM URL: http://ktdreyer.fedorapeople.org/reviews/rubygem-unicorn-4.8.2-3.fc21.src.rpm

Exact changes in Git: http://fedorapeople.org/cgit/ktdreyer/public_git/rubygem-unicorn.git/commit/?id=fe2d3423797a54250ed065b1870adb8e585693d7

F21 scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6631139
Comment 8 Vít Ondruch 2014-03-14 07:52:58 EDT
(In reply to Ken Dreyer from comment #7)
> > - E: specfile-error -e:1:in `<main>': Use RbConfig instead of obsolete and
> > deprecated Config. Epel6 uses ruby1.8 which I assume uses Config so it's
> > safe to ignore?
> 
> IMHO it's ok to ignore this since that macro only gets used on EL6.

1) You should use RbConfig, not sure why you should go with Config
2) You should not declare these macros at all, unless you are going to support EL5, since all Fedora macros should work on EL6.5+. Even the %{gem_install} should be supported now. Remember, it was you who pushed this forward ;)
Comment 9 Vít Ondruch 2014-03-14 07:54:48 EDT
In other words, the only difference should be the ruby(abi) vs ruby(release).


BTW why "ExcludeArch: ppc ppc64"?
Comment 10 Vít Ondruch 2014-03-14 08:06:29 EDT
>> - You don't exclude %{gem_cache}.
>
> Good catch, thanks. Fixed in -3.

Can't confirm.

* You should not mark tests as a %doc
  - And there are probably also other directories which should not be marked.

* seds in %prep
  - I would move the seds (especially for the test suite) into %check and kept
    the originals untouched.
  - The rubygems seds are unnecessary IMO. And if they are needed, then just on
    EL6. There are different means to require files for test suite if needed.

* "# Require rubygems during binary invocation"
  - There should be generated bin stubs by rubygems, hence these seds are not
    requires
  - The subsequent copies of binaries in %install sections are wrong from the
    same reasons.
  - Please check with the example the guidelines [1].

* Fix wrong shebang
  - The seds can be modified into oneline version, which would be more intuitive
    IMO.


[1] https://fedoraproject.org/wiki/Packaging:Ruby#Building_gems
Comment 11 Ken Dreyer 2014-05-12 21:27:57 EDT
I'm continuing to work to address the issues Vít pointed out in the review.

I've posted Minitest 5 support upstream, and I'm working with upstream to get that merged. http://bogomips.org/unicorn-public/m/20140513011353.GA9713%40dcvr.yhbt.net.html

Currently this package depends on a working rubygem-kgio, so I've set this to block bug 1096996. In bug 1006808 I've posted a new kgio version that builds for Rawhide.
Comment 12 Vít Ondruch 2015-10-20 07:38:53 EDT
*** Bug 1268354 has been marked as a duplicate of this bug. ***
Comment 13 Upstream Release Monitoring 2015-11-12 02:19:02 EST
soulen's scratch build of rack-1.0.1-0.src.rpm for f23 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11800617
Comment 14 Upstream Release Monitoring 2015-11-12 02:27:16 EST
soulen's scratch build of rack-1.0.1-0.src.rpm for f23 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11800663
Comment 15 Upstream Release Monitoring 2015-11-12 02:32:27 EST
soulen's scratch build of rack-1.0.1-0.src.rpm for f22-candidate failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11800684
Comment 16 Upstream Release Monitoring 2015-11-12 02:35:37 EST
soulen's scratch build of rack-1.0.1-0.src.rpm for f23-candidate failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11800694
Comment 17 Upstream Release Monitoring 2015-11-12 02:43:45 EST
soulen's scratch build of rack-1.0.1-0.src.rpm for f23 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11800718
Comment 18 Upstream Release Monitoring 2015-11-13 14:51:40 EST
soulen's scratch build of rack-1.0.1-0.src.rpm for f24 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11823665

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