Bug 1586199 - Review Request: rubygem-mini_magick - Manipulate images with minimal use of memory via ImageMagick
Summary: Review Request: rubygem-mini_magick - Manipulate images with minimal use of m...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jun Aruga
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 1264660 (view as bug list)
Depends On:
Blocks: 1267323
TreeView+ depends on / blocked
 
Reported: 2018-06-05 17:12 UTC by Pavel Valena
Modified: 2018-06-28 09:39 UTC (History)
3 users (show)

Fixed In Version: rubygem-mini_magick-4.8.0-1.fc29
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-06-12 10:46:41 UTC
jaruga: fedora-review+


Attachments (Terms of Use)

Comment 1 Jun Aruga 2018-06-07 13:35:38 UTC
At first 2 things that I want to ask.


1. 

> #-devel
> # BuildRequires: rubygem(posix-spawn)

Why does the only gem: posix-spawn for development is kept as commented line?
While the other gems guard are removed.

2.

> # Test failing with:
> #   expected: "mogrify", got: nil
> sed -i '/^    it "assigns :mogrify by default" do$/,/    end/ s/^/#/g' \
>   spec/lib/mini_magick/configuration_spec.rb

Seeing the source code, this failure happens when mogrify command (ImageMagick package) is not installed. As you are setting ImageMagick as a build dependency, you can remove this sed command line, right?
But after removing the line, you will face a different test failure of ImageMagick unique tests.

```
Failures:

  1) With ImageMagick MiniMagick::Image#details returns a hash of verbose information
     Failure/Error: expect(subject.details["Channel depth"]["Red"]).to eq "8-bit"
     
       expected: "8-bit"
            got: nil
     
       (compared using ==)
     # ./spec/lib/mini_magick/image_spec.rb:423:in `block (5 levels) in <top (required)>'
```

Seeing the source code, the MiniMagick::Image#details is an output of "identify -verbose" command. But maybe the result is invalid. Maybe ImageMagik on Fedora is something wrong. This happens on your environment? Can you dig this or report to the project?

```
<mock-chroot> sh-4.4# rpm -qf /usr/bin/identify
ImageMagick-6.9.9.38-1.fc29.x86_64

<mock-chroot> sh-4.4# identify -verbose
=> The result is empty

<mock-chroot> sh-4.4# identify --help
identify: unable to open image `--help': No such file or directory @ error/blob.c/OpenBlob/2761.
identify: no decode delegate for this image format `' @ error/constitute.c/ReadImage/504.
```

Comment 2 Jun Aruga 2018-06-07 13:38:41 UTC
> <mock-chroot> sh-4.4# identify --help

Sorry I just made mistake. "identify -help" (single "-" is correct).

Comment 3 Jun Aruga 2018-06-07 14:50:42 UTC
Below is a result of fedora-review command. 

[!]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
  The sed command line for "Test failing with" needs a link to upstream, if the sed command is really needed.

Comment 4 Pavel Valena 2018-06-07 18:05:07 UTC
(In reply to Jun Aruga from comment #1)
> > #-devel
> > # BuildRequires: rubygem(posix-spawn)
> 
> Why does the only gem: posix-spawn for development is kept as commented line?
> While the other gems guard are removed.

Sorry, I forgot to remove those as well.

> 2.
> 
> > # Test failing with:
> > #   expected: "mogrify", got: nil
> > sed -i '/^    it "assigns :mogrify by default" do$/,/    end/ s/^/#/g' \
> >   spec/lib/mini_magick/configuration_spec.rb
> 
> Seeing the source code, this failure happens when mogrify command
> (ImageMagick package) is not installed. As you are setting ImageMagick as a
> build dependency, you can remove this sed command line, right?

You're right. I don't know why I did add the `sed`. Removed.

> But after removing the line, you will face a different test failure of
> ImageMagick unique tests.

No, that failure is unrelated, the tests suceed, see bellow.

> 
> ```
> Failures:
> 
>   1) With ImageMagick MiniMagick::Image#details returns a hash of verbose
> information
>      Failure/Error: expect(subject.details["Channel depth"]["Red"]).to eq
> "8-bit"
>      
>        expected: "8-bit"
>             got: nil
>      
>        (compared using ==)
>      # ./spec/lib/mini_magick/image_spec.rb:423:in `block (5 levels) in <top
> (required)>'
> ```

This is actually fixed by Patch0.
  https://github.com/minimagick/minimagick/pull/454/

--

I've updated the srpm and .spec file in Description. New Scratch-build:
  https://koji.fedoraproject.org/koji/taskinfo?taskID=27475124

Thanks for finding those issues!

Comment 5 Jun Aruga 2018-06-08 11:45:10 UTC
okay, I reviewed it again.
It looks good. I would ACCEPT it.

Comment 6 Pavel Valena 2018-06-08 14:12:36 UTC
Fixing assignee.

https://pagure.io/releng/fedora-scm-requests/issue/6946

Comment 7 Jun Aruga 2018-06-08 14:16:25 UTC
Oh thanks for that.

Comment 8 Gwyn Ciesla 2018-06-08 14:17:15 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rubygem-mini_magick

Comment 9 Jun Aruga 2018-06-12 07:58:32 UTC
Hi Pavel,
After the package are prepared, can you close this ticket?
http://fedoraproject.org/wiki/Package_Review_Process

Comment 10 Pavel Valena 2018-06-12 10:46:41 UTC
Jun,

I am familiar with the review process, no need to remind me all the time. Unless, off course, there's some specific detail you want you point out. Which you didn't.

Yes, the package was built 4 days ago, I just didn't get around to close this ticket. It was in my next cycle.

Thanks for the review.

Comment 11 Jun Aruga 2018-06-12 11:30:33 UTC
Pavel,
I believed you are familiear with the review process.
As I saw my assigned BZ is kept not closed for a time, I wanted to remind you.

Comment 12 Pavel Valena 2018-06-28 09:39:52 UTC
*** Bug 1264660 has been marked as a duplicate of this bug. ***


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