[Bug 1256435] New: Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Bug ID: 1256435
           Summary: Review Request: metapixel - photomosaic generator
           Product: Fedora
           Version: rawhide
         Component: Package Review
          Severity: medium
          Priority: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]

Spec URL: http://people.redhat.com/nhorman/metapixel.spec
SRPM URL: http://people.redhat.com/nhorman/metapixel-1.0.2-1.src.rpm
Description: metapixel is a simple photomosaic and collage generator
Fedora Account System Username: nhorman

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Miroslav Suchý  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |[e-mail]
           Assignee|[e-mail]    |[e-mail]
              Flags|                            |fedora-review?

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

--- Comment #1 from Miroslav Suchý  ---
> %define _bindir /usr/bin
> %define _mandir /usr/share/man

Why? Those macros are used even on RHEL5.
> rm -rf $RPM_BUILD_ROOT

a) this should be at the beggining of %install and not prep, b) not necessary
unless you target EPEL

Use macros consitently. I.e. %{buildroot} instead of $RPM_BUILD_ROOT.
> gzip $RPM_BUILD_ROOT/%{_mandir}/man1/metapixel.1

Not necessary. rpm will compress it automatically. Actually rpm can use even
different compress format e.g. xz
Therefore
  %files
  %{_mandir}/man1/metapixel.1.gz
should be rather
  %files
  %{_mandir}/man1/metapixel.1*
> rm -rf %{buildroot}
> %defattr(-, root, root)

Both not needed unless you target EPEL

- Dist tag must be present.

- If (and only if) the source package includes the text of the license(s)
  in its own file, then that file, containing the text of the license(s)
  for the package is included in %license.
  Note: License file COPYING is marked as %doc instead of %license
  See:
  http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

Side note, this package already exist in dist-git, but is retired, so you would
not need new_package request, just change request.

Please check the license tag so it correctly follow reality:
GPL (v2 or later) (with incorrect FSF address)
----------------------------------------------
metapixel-1.0.2/convert.c
metapixel-1.0.2/getopt.c
metapixel-1.0.2/getopt.h
metapixel-1.0.2/getopt1.c
metapixel-1.0.2/imagesize.c
metapixel-1.0.2/metapixel.c
metapixel-1.0.2/metapixel.h
metapixel-1.0.2/pools.c
metapixel-1.0.2/pools.h
metapixel-1.0.2/rwimg/readimage.c
metapixel-1.0.2/rwimg/readimage.h
metapixel-1.0.2/rwimg/rwgif.c
metapixel-1.0.2/rwimg/rwgif.h
metapixel-1.0.2/rwimg/rwjpeg.c
metapixel-1.0.2/rwimg/rwjpeg.h
metapixel-1.0.2/rwimg/rwpng.c
metapixel-1.0.2/rwimg/rwpng.h
metapixel-1.0.2/rwimg/writeimage.c
metapixel-1.0.2/rwimg/writeimage.h
metapixel-1.0.2/vector.c
metapixel-1.0.2/vector.h
metapixel-1.0.2/zoom.c
metapixel-1.0.2/zoom.h

LGPL (v2 or later) (with incorrect FSF address)
-----------------------------------------------
metapixel-1.0.2/allocator.c
metapixel-1.0.2/allocator.h
metapixel-1.0.2/lispreader.c
metapixel-1.0.2/lispreader.h

You may even wont to contact upstream so they update their license file.

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

--- Comment #2 from Miroslav Suchý  ---
BTW http://www.complang.tuwien.ac.at/schani/metapixel/metapixel-1.0.2.tar.gz
produce 404 Not Found.

metapixel.x86_64: W: name-repeated-in-summary C Metapixel
You should not repeat the name in summary.

metapixel.x86_64: W: incoherent-version-in-changelog 1.0.2 ['1.0.2-1',
'1.0.2-1']
The version in changelog should include release number as well.

metapixel.x86_64: W: spurious-executable-perm
/usr/share/man/man1/metapixel.1.gz

metapixel.src:32: W: setup-not-quiet
You should pass -q to %setup macro (without -q it should be used only for
debugging).

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Neil Horman  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|---                         |NOTABUG
        Last Closed|                            |2015-08-26 15:39:23

--- Comment #3 from Neil Horman  ---
sorry about that, I didn't even think to check dist-git.  I'll just unretire
it.  Thanks

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Neil Horman  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|fedora-review?              |fedora-cvs?

--- Comment #4 from Neil Horman  ---
Package Change Request
======================
Package Name: metapixel
New Branches: master
Owners: nhorman

please unretire this package, I have fixes to allow it to build again.

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

--- Comment #5 from Miroslav Suchý  ---
I'm afraid that you need to go through review process anyway because this
package is retired for long time. See
https://fedoraproject.org/wiki/Orphaned_package_that_need_new_maintainers#Cl...

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

--- Comment #6 from Jon Ciesla  ---
Clearing cvs flag.

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Jon Ciesla  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|fedora-cvs?                 |

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Neil Horman  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|NOTABUG                     |CURRENTRELEASE

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Neil Horman  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|CLOSED                      |ASSIGNED
         Resolution|CURRENTRELEASE              |---
           Keywords|                            |Reopened

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Neil Horman  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |needinfo?([e-mail]
                   |                            |)

--- Comment #7 from Neil Horman  ---
Spec URL: http://people.redhat.com/nhorman/metapixel.spec
SRPM URL: http://people.redhat.com/nhorman/metapixel-1.0.2-1.src.rpm

Ok, new packages/spec available for review

Change notes:

*rpmlint cleanups.  Should be clean save for a false positive spelling issue
(photomosaic isn't a recognized word)

* License Address changes to properly reflect current FSF address (I'm
preparing an upstream patch for this as well)

* Removed needless macros

* Enforced consistent use of RPM_BUILD_ROOT

* Fixed man page permission and compression

* Added dist tag

* Fixed spec file to list COPYING as %license

* Fixed spec file license tag to reflect reality (LGPLv2+ and GPLv2+), though
that should be fixed upstream too, given that this build produces no DSO's,
making the LGPL largely moot.

I've also sent a note to devel announcing my intent to unretire this package,
so pending your review, we should be good to start with the admin cvs requests.

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Miroslav Suchý  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|needinfo?([e-mail] |
                   |)                           |

--- Comment #8 from Miroslav Suchý  ---
SPEC in #7 differs from SPEC file included in SRPM in #7.
> %{_mandir}/man1/metapixel.1

This should be:
%{_mandir}/man1/metapixel.1*
> Requires: libpng
> Requires: libjpeg
> Requires: giflib

This is not needed. Rpm automatically guess this dependency and generate e.g
for libjpeg those deps:
  libjpeg.so.62()(64bit)
  libjpeg.so.62(LIBJPEG_6.2)(64bit)
which is more precise than listing libjpeg explicitely.
> %changelog
> Wed Aug 26 2015 Neil Horman  - 1.0.2

As I said, you should user release in changelog entry as well. Therefore:
* Wed Aug 26 2015 Neil Horman  - 1.0.2-1

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Neil Horman  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]
              Flags|                            |needinfo?([e-mail]
                   |                            |m)

--- Comment #9 from Neil Horman  ---
Spec URL: http://people.redhat.com/nhorman/metapixel.spec
SRPM URL: http://people.redhat.com/nhorman/metapixel-1.0.2-1.src.rpm

All fixed, new version available, thanks!

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Ralf Corsepius  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #10 from Ralf Corsepius  ---
(In reply to Neil Horman from comment #9)
> All fixed, new version available, thanks!

Well,
- this package doesn't honor CFLAGS:
...
gcc -I/sw/include -I/usr/X11R6/include -I/usr/X11R6/include/X11 -I. -Irwimg
-Wall -O2   -DMETAPIXEL_VERSION="1.0.2" -DRWIMG_JPEG -DRWIMG_PNG -DRWIMG_GIF
-c metapixel.c
...

- this package doesn't honor ans installation paths. 
This is the origin why "make install" appears broken to you and likely is why
you resorted to manual installation.

- this package's "release" lacks %{?dist}

- there still are superflous deps:
# rpmlint metapixel-1.0.2-1.x86_64.rpm 
metapixel.x86_64: E: explicit-lib-dependency giflib
metapixel.x86_64: E: explicit-lib-dependency libjpeg
metapixel.x86_64: E: explicit-lib-dependency libpng

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

--- Comment #11 from Neil Horman  ---
Ralf, you're two revisions behind in your reviews.  The deps have been fixed
already

Though you're correct about the missed cflags and install paths.  The makefile
honors the paths, but the spec file was taken from the upstream project, and I
neglected to fix that up. will do so

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Neil Horman  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|needinfo?([e-mail] |needinfo?([e-mail]
                   |m)                          |)

--- Comment #12 from Neil Horman  ---
Spec URL: http://people.redhat.com/nhorman/metapixel.spec
SRPM URL: http://people.redhat.com/nhorman/metapixel-1.0.2-1.src.rpm

There, cflags and install paths fixed.

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

--- Comment #13 from Ralf Corsepius  ---
(In reply to Neil Horman from comment #11)
> Ralf, you're two revisions behind in your reviews.

It's what I found in http://people.redhat.com/nhorman/metapixel-1.0.2-1.src.rpm
shortly after comment #9 arrived by email.

As you did not increment %release, it's impossible for to tell if the version I
downloaded is the right one => Please increment %release each time you change
something in your spec. Not doing so adds avoidable complications to reviews.

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

--- Comment #14 from Ralf Corsepius  ---
(In reply to Neil Horman from comment #12)
> Spec URL: http://people.redhat.com/nhorman/metapixel.spec
> SRPM URL: http://people.redhat.com/nhorman/metapixel-1.0.2-1.src.rpm
>
> There, cflags and install paths fixed.

Well, you seem to have updated http://people.redhat.com/nhorman/metapixel.spec 
but not http://people.redhat.com/nhorman/metapixel-1.0.2-1.src.rpm

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

--- Comment #15 from Ralf Corsepius  ---
- Impossible to review this package, because *.spec does not match the srpm.
=>  metapixel-copyright.patch and metapixel-install.patch are missing.

- If metapixel-copyright.patch is what I presume it is (Changing the FSF
address) then it's superfluous, because it's legally irrelevant.

- MUSTFIX: Add %{?dist} to release

- make PREFIX=... is pretty much useless.
You need to add BINDIR and MANDIR as well.

- Requires: perl also seems superfluous. Rpm automatically adds appropriate R:
to packages containing perl scripts

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

--- Comment #16 from Neil Horman  ---
sorry, typoed the srpm address

Spec URL: http://people.redhat.com/nhorman/metapixel.spec
SRPM URL: http://people.redhat.com/nhorman/metapixel-1.0.2-2.fc21.src.rpm

even bumped the release for you :)

Regarding the copyright patch, it is what you presume, and while I agree its
legally irrelevant, its also easy to do, so instead of arguing about it, I just
updated it.

%{dist} tag has been fixed for some time, above was a typo.

BINDIR and MANDIR are set appropriately in the makefile, no need to do that in
the spec file.

Regarding perl, it may be superfulous, but its also not getting a complaint
from rpmlint on the subject, nor are the packaging guidelines explicit on the
subject.  I'd just as soon leave it in place

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Miroslav Suchý  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|needinfo?([e-mail] |fedora-review+
                   |)                           |

--- Comment #17 from Miroslav Suchý  ---
metapixel-copyright.patch is not necessary. It is just enough to contact
upstream and notify them. However it is neither blocker for review.

Everything else was addressed (I agree with Neil on BINDIR and MANDIR and perl
issues). Therefore:

APPROVED

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Neil Horman  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |fedora-cvs?

--- Comment #18 from Neil Horman  ---
Package Change Request
======================
Package Name: metapixel
New Branches: master f23
Owners: nhorman

please unretire the metapixel package and assign me as the owner

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Jon Ciesla  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|fedora-cvs?                 |fedora-cvs+

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

--- Comment #19 from Jon Ciesla  ---
Git done (by process-git-requests).

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Neil Horman  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |MODIFIED

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

--- Comment #20 from Ralf Corsepius  ---
(In reply to Miroslav Suchý from comment #17)
> Everything else was addressed (I agree with Neil on BINDIR and MANDIR and
> perl issues). Therefore:

I do not agree. Though issues are minor, they will very likely hit you in
future.
> APPROVED

Well, you don't want to know what I think of this.

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

--- Comment #21 from Fedora Update System  ---
metapixel-1.0.2-2.fc23 has been submitted as an update to Fedora 23.
https://bodhi.fedoraproject.org/updates/FEDORA-2015-15527

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|MODIFIED                    |ON_QA

--- Comment #22 from Fedora Update System  ---
metapixel-1.0.2-2.fc23 has been pushed to the Fedora 23 testing repository. If
problems still persist, please make note of it in this bug report.\nIf you want
to test the update, you can install it with \n su -c 'yum
--enablerepo=updates-testing update metapixel'. You can provide feedback for
this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-15527

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

--- Comment #23 from Fedora Update System  ---
metapixel-1.0.2-2.fc23 has been pushed to the Fedora 23 stable repository. If
problems still persist, please make note of it in this bug report.

[Bug 1256435] Review Request: metapixel - photomosaic generator

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1256435

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ON_QA                       |CLOSED
         Resolution|---                         |ERRATA
        Last Closed|2015-08-26 15:39:23         |2015-10-31 22:49:26