Bug 1992914 - Review Request: rubygem-middleware - Generalized implementation of the middleware abstraction for Ruby
Summary: Review Request: rubygem-middleware - Generalized implementation of the middle...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1987945
TreeView+ depends on / blocked
 
Reported: 2021-08-12 00:28 UTC by Pavel Valena
Modified: 2021-08-25 14:33 UTC (History)
1 user (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2021-08-25 14:33:14 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+


Attachments (Terms of Use)

Description Pavel Valena 2021-08-12 00:28:12 UTC
Spec URL: https://raw.githubusercontent.com/fedora-distgit/rubygem-middleware/rawhide/rubygem-middleware.spec
SRPM URL: https://github.com/fedora-distgit/rubygem-middleware/raw/rawhide/rubygem-middleware-0.1.0-9.fc35.src.rpm
Description: Generalized implementation of the middleware abstraction for Ruby.
Fedora Account System Username: pvalena

Additional tests: https://git.io/JR1uj

This is a retired package, for more than 8 weeks, therefore re-review is needed.

Comment 1 Mamoru TASAKA 2021-08-12 05:09:21 UTC
Taking. Would you instead review my review request? https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=1992931

Some comments:

* release number and last %changelog entry mismatch. (%changelog in spec file in .src.rpm ends with -10)
  (New %changelog date format (including time and time difference) does not seem to be supported on
   rpmlint, also this would cause error / warning when building this when doing 'fedpkg build' after
   importing this to fedora dist-git. I suggest to use old date format.)

* Check if files in %doc are really needed.
  Especially, spec/ Rakefile Gemfile should not be needed. I also suggest to remove %{gem_instdir}/%{gem_name}.gemspec .


* Note (not a blocker):
  rspec -I rspec spec now causes warnings. Please consider to check
  https://github.com/mitchellh/middleware/pull/15/commits/f559ee1a5fbc625b3a629f64d03f4ce249ea2fa0

Comment 2 Pavel Valena 2021-08-12 09:12:44 UTC
(In reply to Mamoru TASAKA from comment #1)
> Taking. Would you instead review my review request?
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=1992931

Thanks!

I did submit this, at it was blocking a colleague, and was long overdue. I'll not be able to do the review in the near time, but I'll ask the colleague to take it instead.

> 
> Some comments:
> 
> * release number and last %changelog entry mismatch. (%changelog in spec
> file in .src.rpm ends with -10)

Sorry, I've fixed this in the attached .spec file, but forgot to do so in the SRPM [1].

>   (New %changelog date format (including time and time difference) does not
> seem to be supported on
>    rpmlint, also this would cause error / warning when building this when
> doing 'fedpkg build' after
>    importing this to fedora dist-git. I suggest to use old date format.)

I'm not getting any rpmlint error [2]. But I'm fine with the old format.

> 
> * Check if files in %doc are really needed.
>   Especially, spec/ Rakefile Gemfile should not be needed. I also suggest to
> remove %{gem_instdir}/%{gem_name}.gemspec .
> 

This seems like a divergence from upstream to me. I'd prefer leave it in a subpackage for inspection purposes, as it came with the .gem. There's no harm done to have it, I think.

> 
> * Note (not a blocker):
>   rspec -I rspec spec now causes warnings. Please consider to check
>  
> https://github.com/mitchellh/middleware/pull/15/commits/
> f559ee1a5fbc625b3a629f64d03f4ce249ea2fa0

That's the only PR I've not checked. Thanks! I've applied the patch, and updated the SRPM+Spec.

https://koji.fedoraproject.org/koji/taskinfo?taskID=73716696

[1] https://github.com/fedora-distgit/rubygem-middleware/blob/rawhide/rubygem-middleware.spec#L70
[2] https://git.io/JRDca

Comment 3 Pavel Valena 2021-08-16 10:14:26 UTC
Actually, I missed part of the patch, also had to modify it:

https://koji.fedoraproject.org/koji/taskinfo?taskID=73717105

Comment 4 Mamoru TASAKA 2021-08-17 13:32:56 UTC
Ah, well, do you know how is this package from : https://github.com/Ibsciss/ruby-middleware  ?

Looking at the git commit log, I guess that Ibsciss/ruby-middleware is forked from  mitchellh/middleware
(  https://github.com/mitchellh/middleware/commit/23d4741d15c5999d81493ec43448f72032119bee seems the same as
   https://github.com/Ibsciss/ruby-middleware/commit/23d4741d15c5999d81493ec43448f72032119bee )
and Ibsciss/ruby-middleware seems "newer".

Comment 5 Pavel Valena 2021-08-17 17:50:58 UTC
It's from the same `.gem` file as it was before retiring, AFAIK there was no release since "March 16, 2012".

You're right, there was some activity in the fork 2015 / 2017. If the middleware gem needs those changes for rubygem-cucumber update (which is the reason we're un-retiring it) we can apply them. At this point that doesn't seem to be the case.

Comment 6 Mamoru TASAKA 2021-08-18 12:41:48 UTC
Okay, looks good

* spec file clean
* license acceptable (MIT)
* buildable on all archs: https://koji.fedoraproject.org/koji/taskinfo?taskID=74008041
* installable
* rpmlint clean (for srpm, binary rpms, installed rpm)
* example seems to work: https://github.com/mitchellh/middleware/blob/master/README.md
  works as written

=================================================================
  This package (rubygem-middleware) is APPROVED by
  mtasaka
=================================================================

Comment 7 Pavel Valena 2021-08-19 15:00:29 UTC
Thank you!

Comment 8 Mamoru TASAKA 2021-08-25 14:33:14 UTC
rubygem-middleware-0.1.0-9.fc36 is already built, closing.


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