Bug 1065685 - Review Request: rubygem-unicorn - Rack HTTP server for fast clients and Unix
Summary: Review Request: rubygem-unicorn - Rack HTTP server for fast clients and Unix
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 786636 1268354 (view as bug list)
Depends On: 1096996
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2014-02-15 20:43 UTC by Ken Dreyer
Modified: 2021-08-11 00:45 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-08-11 00:45:28 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Ken Dreyer 2014-02-15 20:43:33 UTC
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 20:43:51 UTC
*** Bug 786636 has been marked as a duplicate of this bug. ***

Comment 2 Achilleas Pipinellis 2014-03-01 12:02:57 UTC
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 12:15:21 UTC
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 14:08:49 UTC
(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 14:58:31 UTC
(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 15:03:44 UTC
(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 22:46:25 UTC
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 11:52:58 UTC
(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 11:54:48 UTC
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 12:06:29 UTC
>> - 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-13 01:27:57 UTC
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 11:38:53 UTC
*** Bug 1268354 has been marked as a duplicate of this bug. ***

Comment 13 Upstream Release Monitoring 2015-11-12 07:19:02 UTC
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 07:27:16 UTC
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 07:32:27 UTC
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 07:35:37 UTC
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 07:43:45 UTC
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 19:51:40 UTC
soulen's scratch build of rack-1.0.1-0.src.rpm for f24 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11823665

Comment 19 Package Review 2020-07-10 00:49:14 UTC
This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

Comment 20 Package Review 2020-11-13 00:46:00 UTC
This is an automatic action taken by review-stats script.

The ticket reviewer failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we reset the status and the assignee of this ticket.

Comment 21 Otto Liljalaakso 2021-07-11 06:45:54 UTC
This review request is very old. Do you still intend to complete it? If so, I can review. If not, please close the issue and mark it as FE-DEADREVIEW, or do nothing, in which case automation will close it in one month.

Things to fix: Specfile and srpm links are broken, new version available, items listed in earlier comments.

Comment 22 Package Review 2021-08-11 00:45:28 UTC
This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.


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