Bug 225797 - Review Request: gimp-data-extras
Review Request: gimp-data-extras
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
: Reopened
: 251614 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 13:43 EST by Nobody's working on this, feel free to take it
Modified: 2009-02-24 06:15 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-24 06:15:55 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)


External Trackers
Tracker ID Priority Status Summary Last Updated
GNOME Desktop 410387 None None None Never

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 13:43:52 EST
Fedora Merge Review: gimp-data-extras

http://cvs.fedora.redhat.com/viewcvs/devel/gimp-data-extras/
Initial Owner: nphilipp@redhat.com
Comment 1 Roozbeh Pournader 2007-02-06 10:36:10 EST
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
Comment 2 Nils Philippsen 2007-02-21 09:20:41 EST
(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

Comment 3 Nils Philippsen 2007-02-21 09:21:55 EST
Added Adrian on Cc, as he is (was) upstream, at least partly.

Adrian, can you shed some light on the licensing question?
Comment 4 Nils Philippsen 2007-04-02 03:17:37 EDT
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.
Comment 5 Nils Philippsen 2007-06-13 09:44:59 EDT
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@gimp.org>

        * 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.
Comment 6 Nils Philippsen 2007-08-10 04:14:05 EDT
*** Bug 251614 has been marked as a duplicate of this bug. ***
Comment 7 Nils Philippsen 2007-11-14 08:59:02 EST
gimp-data-extras-2.0.2 is out with the license clarified
Comment 8 Nils Philippsen 2007-11-14 09:18:39 EST
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@redhat.com> - 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
Comment 9 Nils Philippsen 2007-12-04 04:53:32 EST
Changing the summary to clarify that this is not about a package already in the
repo.
Comment 10 Jason Tibbitts 2008-12-19 01:35:40 EST
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).
Comment 11 Nils Philippsen 2008-12-19 05:42:48 EST
(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
Comment 12 Jason Tibbitts 2008-12-19 14:58:50 EST
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.
Comment 13 Jason Tibbitts 2008-12-19 17:01:14 EST
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
Comment 14 Jason Tibbitts 2009-02-06 19:24:45 EST
Any response to my review commentary?
Comment 15 Nils Philippsen 2009-02-09 07:09:29 EST
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
Comment 16 Jason Tibbitts 2009-02-12 15:51:30 EST
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.
Comment 17 Nils Philippsen 2009-02-13 10:11:50 EST
(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
Comment 18 Jason Tibbitts 2009-02-13 12:34:29 EST
Looks good, thanks.

APPROVED; just fix up the release to an integer >= 1 when you check in.
Comment 19 Nils Philippsen 2009-02-16 05:37:52 EST
(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:
Comment 20 Kevin Fenzi 2009-02-16 16:29:15 EST
cvs done.

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