Bug 225797

Random first notes:
* Release (1.1.1) should be an integer. You may also add %{?dist} if you wish.
* separate the BuildRequires into two lines. It's not very obvious now.
* change BuildRoot to %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
* remove the dot at the end of Summary.
* As /usr/bin is in the execution path, saying /usr/bin/gimptool in the install
section is not necessary. Just use "gimptool", which makes the spec more legible.
* Use %defattr (-, root, root, -) instead of %defattr (-, root, root)
* The license filed says GPL, while there is nothing in the source tarball that
confirms that. The COPYING file is also empty. (BLOCKER)
* You should ship some of the files as %doc (at least AUTHORS, ChangeLog, NEWS,
and README).
* The spec file is not UTF-8. (BLOCKER)
* The install root is not cleaned at the beginning of %install

(In reply to comment #1)
> Random first notes:
> * Release (1.1.1) should be an integer. You may also add %{?dist} if you wish.
> * separate the BuildRequires into two lines. It's not very obvious now.
> * change BuildRoot to
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
> * remove the dot at the end of Summary.
> * As /usr/bin is in the execution path, saying /usr/bin/gimptool in the install
> section is not necessary. Just use "gimptool", which makes the spec more legible.

all fixed in CVS

> * Use %defattr (-, root, root, -) instead of %defattr (-, root, root)

I've used "%defattr (-, root, root, 0755)" . Using "-" as dirmode (instead of
none) doesn't make a difference, both mean "keep mode of the directory as found".

> * The license filed says GPL, while there is nothing in the source tarball that
> confirms that. The COPYING file is also empty. (BLOCKER)

filed upstream at http://bugzilla.gnome.org/show_bug.cgi?id=410387

> * You should ship some of the files as %doc (at least AUTHORS, ChangeLog, NEWS,
> and README).
> * The spec file is not UTF-8. (BLOCKER)
> * The install root is not cleaned at the beginning of %install

all fixed in CVS

Added Adrian on Cc, as he is (was) upstream, at least partly.

Adrian, can you shed some light on the licensing question?

Due to the unclear licensing, the package isn't included in current Rawhide/test
versions. Adrian told me and upstream that his contributions are public domain
(or GPL if preferred) and what he knew about the origins of the other stuff. So
far, upstream hasn't reacted. In that case I'd leave the package out for now, if
upstream comes to a conclusion, we can include it at a later point.

Upstream has resolved the issue, but not issued a sanitized tarball yet. I'll
reopen once this is done.

From http://bugzilla.gnome.org/show_bug.cgi?id=410387:

------- Comment #5 from Sven Neumann  2007-05-07 10:36 UTC -------
OK, I have checked this into trunk then:

2007-05-07  Sven Neumann  <sven>

        * COPYING: GNU General Public License version 2. Fixes bug #410387.

        * brushes/Makefile.am
        * brushes/punch.gbr
        * brushes/reach.gbr
        * brushes/point.gbr
        * brushes/bullethole.gbr: removed "found" art as their license is
        unclear.

We should probably do a release together with GIMP 2.4 then.

*** Bug 251614 has been marked as a duplicate of this bug. ***

gimp-data-extras-2.0.2 is out with the license clarified

The spec file and source RPM are at: 

http://people.fedoraproject.org/~nphilipp/gimp/gimp-data-extras.spec
http://people.fedoraproject.org/~nphilipp/gimp/gimp-data-extras-2.0.2-0.1.fc8.src.rpm

Note that I begun with the spec file of 2.0.1-1.1.1, i.e. before I made the
above changes (didn't find the newer version). I also reverted to using the full
path of gimptool as this is consistent with other RPM macros like e.g. %__id_u
which also use full paths.

These are the changes in the spec file  (I'll use an integer release number
after the review):
* Wed Nov 14 2007 Nils Philippsen <nphilipp> - 2.0.2-0.1
- version 2.0.2
- use RPM macros where appropriate
- use "make DESTDIR=... install"
- merge review (#225797)
  - add dist tag
  - change license tag to GPLv2+
  - sanitize buildroot
  - set default mode 0755 for directories
  - add documentation files
  - separate BuildRequires, add epoch to BR: gimp-devel ...
  - sanitize summary
  - recode SPEC file to UTF-8
  - clean buildroot at beginning of %install

Changing the summary to clarify that this is not about a package already in the
repo.

I'm a bit confused; is this a review ticket for bringing gimp-data-extras back into the distro?  It's lost in the sea of merge reviews so nobody has looked at it in ages.  Koji says that gimp-data-extras has been blocked from the distro since F-7.

If this really does need a review, please let me know.  But I also note that the links in comment 8 above are invalid (which isn't surprising, 13 months later).

(In reply to comment #10)
> I'm a bit confused; is this a review ticket for bringing gimp-data-extras back
> into the distro? 

Yes. Originally it was a merge review ticket, but as of the unclear ownership of some of the files contained in the old versions, the package was dropped. When upstream published a fixed version, I reopened the ticket to get the package get back in.

> It's lost in the sea of merge reviews so nobody has looked at
> it in ages.  Koji says that gimp-data-extras has been blocked from the distro
> since F-7.

Due to legal reasons, but these have been resolved in the meantime.

> If this really does need a review, please let me know.

I'd appreciate a review ;-).

> But I also note that
> the links in comment 8 above are invalid (which isn't surprising, 13 months
> later).

I've rebuilt the source RPM and uploaded the files to:

Spec: http://nphilipp.fedorapeople.org/review/gimp-data-extras.spec
SRPM: http://nphilipp.fedorapeople.org/review/gimp-data-extras-2.0.2-0.1.fc10.src.rpm

I'm not really the best person to do this because I know zilch about gimp, but the package seems simple enough.  Unfortunately, it doesn't build for me in mock, nor in koji:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=1010055

The problem is that gimptool isn't available when the srpm is first evaluated to extract the build dependencies, so the call fails (which is OK) but then that leaves the macro empty and the resulting spec is not syntactically correct.

What we usually do is tack on
  || echo blah
so that the result has something in it, the resulting spec has correct syntax, and the final package, which is built with all of the build dependencies installed, has the correct value.

I can verify that this does indeed build fine with the first line changed to
  %define gimpdatadir %(%{_bindir}/gimptool --gimpdatadir || echo blah)
Everything that follows assumes that a similar change has been made.
However, I guess it's worth asking what that dependency does that the regular dependency on gimp doesn't do.  We really try to avoid file dependencies out of a few specific directories because they require the users to download additional large hunks of metadata.

Why is the release < 1?  It doesn't seem to me that the 2.0.2 tarball upstream is any kind of prerelease.

Unfortunately I can't find any statement of the license version in use.  COPYING is simply v2 of the GPL, which has the usual language about being able to use any version if the program itself doesn't specify one.  That would indicate that GPL+ is the appropriate license tag, but it would be a good idea to clarify with upstream because I don't think that's what they intend.

* source files match upstream.  sha256sum:
  31f9b40822646729be9ff50856e803a59290c119c600a8fdab4b669c4ccf2c1f  
   gimp-data-extras-2.0.2.tar.bz2
X does not meet versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
? license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* rpmlint is silent.
  final provides and requires:
   gimp-data-extras = 2.0.2-0.1.fc11
  =
?  /usr/share/gimp/2.0
   gimp

* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files
* acceptable content

Any response to my review commentary?

Sorry for the delay...

(In reply to comment #13)
> I can verify that this does indeed build fine with the first line changed to
>   %define gimpdatadir %(%{_bindir}/gimptool --gimpdatadir || echo blah)
> Everything that follows assumes that a similar change has been made.

I've added this workaround.

> However, I guess it's worth asking what that dependency does that the regular
> dependency on gimp doesn't do.  We really try to avoid file dependencies out of
> a few specific directories because they require the users to download
> additional large hunks of metadata.

Agreed. The package now requires gimp >= 2:2.0 and I've dropped the dependency on the directory.

> Why is the release < 1?  It doesn't seem to me that the 2.0.2 tarball upstream
> is any kind of prerelease.

Uh, that's because I'm a stickler for eye-pleasing and I wanted to reserve the -1 release for the version to import into Fedora CVS.

> Unfortunately I can't find any statement of the license version in use. 
> COPYING is simply v2 of the GPL, which has the usual language about being able
> to use any version if the program itself doesn't specify one.  That would
> indicate that GPL+ is the appropriate license tag, but it would be a good idea
> to clarify with upstream because I don't think that's what they intend.

I've checked with upstream and they told me I should consider it as GPLv2+. There's a new version in the works which will hopefully clearly mention the license directly in the archive.

> * source files match upstream.  sha256sum:
>   31f9b40822646729be9ff50856e803a59290c119c600a8fdab4b669c4ccf2c1f  
>    gimp-data-extras-2.0.2.tar.bz2
> X does not meet versioning guidelines.

OK if I bump it to -1 prior to importing? That leaves us -0.x for review work....

> * specfile is properly named, is cleanly written and uses macros consistently.
> * summary is OK.
> * description is OK.
> * dist tag is present.
> * build root is OK.
> ? license field matches the actual license.

I can attach an IRC log snippet if necessary.

> * license is open source-compatible.
> * license text included in package.
> * latest version is being packaged.
> * BuildRequires are proper.
> * %clean is present.
> * package builds in mock (rawhide, x86_64).
> * package installs properly.
> * rpmlint is silent.
>   final provides and requires:
>    gimp-data-extras = 2.0.2-0.1.fc11
>   =
> ?  /usr/share/gimp/2.0
>    gimp

Resolved by directly requiring gimp >= 2:2.0 I think...

> * owns the directories it creates.
> * doesn't own any directories it shouldn't.
> * no duplicates in %files.
> * file permissions are appropriate.
> * no generically named files
> * acceptable content

New files:
Spec file: http://nphilipp.fedorapeople.org/review/gimp-data-extras.spec
SRPM file: http://nphilipp.fedorapeople.org/review/gimp-data-extras-2.0.2-0.2.fc10.src.rpm

I thought I had commented on this already, but it seems not.  Drat.

> I've added this workaround.

And the package indeed builds fine.

> Uh, that's because I'm a stickler for eye-pleasing and I wanted to reserve the
> -1 release for the version to import into Fedora CVS.

I admit to not really understanding why anyone would care, but as long as what gets checked in is correct then I don't see a problem.

> I've checked with upstream and they told me I should consider it as GPLv2+.
[...]
> I can attach an IRC log snippet if necessary.

Can you include that in the package?  I checked with spot and he indicated that this is OK as long as you're reasonably certain that the person you were communicating with is the copyright holder.  Obviously it will be academic once a version is out, but until then we have to clarify the license.

(In reply to comment #16)
> > I've checked with upstream and they told me I should consider it as GPLv2+.
> [...]
> > I can attach an IRC log snippet if necessary.
> 
> Can you include that in the package?  I checked with spot and he indicated that
> this is OK as long as you're reasonably certain that the person you were
> communicating with is the copyright holder.  Obviously it will be academic once
> a version is out, but until then we have to clarify the license.

I've included it as "license-clarification.txt" in the package and it gets installed along with the other documentation.

New files:
Spec file: http://nphilipp.fedorapeople.org/review/gimp-data-extras.spec
SRPM file:
http://nphilipp.fedorapeople.org/review/gimp-data-extras-2.0.2-0.3.fc10.src.rpm

Looks good, thanks.

APPROVED; just fix up the release to an integer >= 1 when you check in.

(In reply to comment #18)
> APPROVED; just fix up the release to an integer >= 1 when you check in.

I've changed that in the SRPM I intend to import. Thanks for reviewing!

New Package CVS Request
=======================
Package Name: gimp-data-extras
Short Description: Extra files for GIMP
Owners: nphilipp
Branches: F-9 F-10
InitialCC:

cvs done.