Bug 526651 - Review Request: xpaint - An X Window System image editing or paint program
Review Request: xpaint - An X Window System image editing or paint program
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Gwyn Ciesla
Fedora Extras Quality Assurance
https://sourceforge.net/projects/sf-x...
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-01 06:11 EDT by Paulo Roma Cavalcanti
Modified: 2011-01-08 13:27 EST (History)
8 users (show)

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


Attachments (Terms of Use)

  None (edit)
Description Paulo Roma Cavalcanti 2009-10-01 06:11:51 EDT
Description of problem:

SRPM URL: http://orion.lcg.ufrj.br/RPMS/src/xpaint-2.8.2-5.fc10.src.rpm

Spec URL: http://orion.lcg.ufrj.br/RPMS/SPECS/xpaint.spec


XPaint is an X Window System color image bitmap editing program. It does
not try to compete with more advanced tools such as 'gimp' but aims at
being very easy to use. XPaint however supports most standard paint program
operations. It also supports more advanced features, such as image processing
algorithms and scripting.  XPaint allows the edition of multiple images
simultaneously and supports a wide variety of image formats, including:
GIF, JPG, PNG, PPM, TIFF, XBM, XPM, etc.

Install xpaint if you need an easy to use paint program for X.

Xpaint now uses the Xaw3d widget set for a bit nicer look. The package
includes several examples of C scripts: filters, image scripts
adding some new editing features including user filters. Some example
filter code is included.



rpmlint is clean.

need to recheck license.


Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1.
2.
3.
  
Actual results:


Expected results:


Additional info:
Comment 2 Paulo Roma Cavalcanti 2009-10-02 08:53:55 EDT
Fixed.Thanks.
Comment 3 Thomas Janssen 2009-10-03 04:40:53 EDT
Ok, i cant get the source from that URL in the spec, 404 is given. There's not even a downloadlink on the page that comes up with the URL: from the spec.

A page with a downloadlink comes up with: 
https://sourceforge.net/projects/sf-xpaint

A working downloadlink is:
http://downloads.sourceforge.net/project/sf-xpaint/sf-xpaint/%{name}-%{version}/%{name}-%{version}.tar.bz2

MUST: rpmlint must be run on every package. The output should be posted in the review.

Where did you find the MIT license? There's no license file in the tarball and the project homepage says: License: "Other license" which is a link to a page with other projects.

MUST: The package must be licensed with a Fedora approved license and meet the http://fedoraproject.org/wiki/Packaging/LicensingGuidelines
MUST: The License field in the package spec file must match the actual license.

Upstream has released 2.8.3 by the way.

MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.

Can you post the 'Task Info' link to a koji scratch build here, thanks.

Icon tag in Desktop Files, the icon tag can be specified in two ways:
Full path to specific icon file or shortname without file extension.

You use shortname with file extension.

Missing: BuildRequires: desktop-file-utils

desktop-file-install MUST be used if the package does not install the file or there are changes desired to the .desktop file (such as add/removing categories, etc).
http://fedoraproject.org/wiki/Packaging/Guidelines#desktop

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 4 Thomas Janssen 2009-10-03 04:51:42 EDT
If this is your first package (i cant find your email in FAS) you need to:

add FE-NEEDSPONSOR to the bugs being blocked by your review request.

There's also: https://bugzilla.redhat.com/bugzilla/enter_bug.cgi?product=Fedora&format=extras-review
for reviews. Maybe next time ;)

I added NotReady to the whiteboard due to the license question. Clear the whiteboard when it's fixed.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 5 Paulo Roma Cavalcanti 2009-10-03 06:59:10 EDT
I fixed the spec, but still have to check the license:

SRPM URL: http://orion.lcg.ufrj.br/RPMS/src/xpaint-2.8.3-5.fc10.src.rpm

Spec URL: http://orion.lcg.ufrj.br/RPMS/SPECS/xpaint.spec
Comment 6 Paulo Roma Cavalcanti 2009-10-03 07:43:15 EDT
In the source there is a file debian/copyright:

---------------------------------------------------------

This package was debianized by Mark W. Eichin eichin@kitten.gen.ma.us on
Wed, 26 Feb 1997 12:37:58 -0500.

The current Debian maintainer is Hugo Vanwoerkom <hugovanwoerkom@yahoo.com>.

The sourcecode can be downloaded from
https://sourceforge.net/projects/sf-xpaint/

Note that parts of this are GPL and parts are redistributable as-is.

Copyright:

Copyright (C) 1990, 1991, 1992, 1993, David Koblas
Copyright (C) 1995, 1996, 1997, 1998, Torsten Martinsen
Copyright (C) 1996-2003, Greg Roelofs
Copyright (C) 1997, Scott D. Nelson
Copyright (C) 1995, Tor Lillqvist

............

It is not clear for me, reading the whole file, if I can use
just GPLv2 or stick with MIT:

The link for the debian, ubuntu, and alt linux packages are these:

http://packages.debian.org/unstable/graphics/xpaint

http://packages.ubuntu.com/karmic/xpaint

http://sisyphus.ru/en/srpm/xpaint

Any clue here?
Comment 7 Thomas Janssen 2009-10-03 08:36:25 EDT
Yes, if in doubt, and that's the case here, ask upstream. Would be perfect if they provide a proper LICENSE file.

You could add that then with sourceX.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 8 Paulo Roma Cavalcanti 2009-10-04 04:29:23 EDT
(In reply to comment #7)
> Yes, if in doubt, and that's the case here, ask upstream. Would be perfect if
> they provide a proper LICENSE file.
> 
> You could add that then with sourceX.
> 
>

This is the answer to my consult:

>
> I would like to include xpaint in Fedora, but it is not
> clear for me
> what is its license., even reading the file debian/copyright.
>
> Would it be possible to have an official license file in the
> project?
>

Hi:

I am sorry that I cannot really clarify the issue. Xpaint has a long
history and combined different parts with different licences, including
MIT and GPL. Everything seems to be GPL compatible and I consider all
my additions to be GPL whenever this can apply - I don't think there
is any issue, but I am not an expert in these legalities...

In any case I'll add the text of GPLv2 in the next release and I
encourage you to do so for the Fedora package if you feel this is
desirable.

Best,
Jean-Pierre Demailly
Comment 9 Thomas Janssen 2009-10-04 05:12:23 EDT
Sounds not too good. I will have fedora-legal look over it. Sorry.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 10 Andrea Musuruane 2009-10-05 08:52:29 EDT
(In reply to comment #9)
> Sounds not too good. I will have fedora-legal look over it. Sorry.

Here it is stated that MIT license is compatible with both GPLv2 and GPLv3:
https://fedoraproject.org/wiki/Licensing:Main#Good_Licenses

This means that you can combine code released under the MIT license with code released under the GNU GPL in one larger program:
http://www.gnu.org/licenses/gpl-faq.html#WhatDoesCompatMean

HTH,

Andrea
Comment 11 Andrea Musuruane 2009-10-05 09:20:32 EDT
I had a glance about the package. This is what else I noticed.

* rpmlint /var/lib/mock/fedora-11-i386/result/*.rpm
xpaint.src:246: W: macro-in-%changelog config
xpaint-devel.i586: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 2 warnings.

There are two occurences of % that haven't been escaped in the %changelog. Please fix them.

* Missing SMP flags. If it doesn't build with it, please add a comment
  (wiki: Packaging/Guidelines#parallelmake)

* Duplicate BuildRequires: zlib-devel (by libpng-devel), libXpm-devel (by libXaw-devel), xorg-x11-proto-devel (by libXaw-devel), libSM-devel (by Xaw3d-devel), libXext-devel (by Xaw3d-devel)

* Not end user documentation files installed:

/usr/share/doc/xpaint-2.8.2/INSTALL
Install instruction must not be installed
/usr/share/doc/xpaint-2.8.2/Doc/CHANGES
This changelog is so old that I really doubt it is useful
/usr/share/doc/xpaint-2.8.2/Doc/Imakefile
/usr/share/doc/xpaint-2.8.2/Doc/Makefile
/usr/share/doc/xpaint-2.8.2/Doc/Makefile.bak
3 not documentation files.
/usr/share/doc/xpaint-2.8.2/Doc/Operator.doc
Developer file.
/usr/share/doc/xpaint-2.8.2/Doc/sample.Xdefaults
Maybe this is useful but because it will be the only useful file in this directory I think it will be better to move it to the parent dir.

* Probably it is a good idea to include README.old among docs. Debian does this too.

* There is no suggest in RPM therefore I think you should require
gv, netpbm, lpr, xv, emacs

Or otherwise make a README.Fedora file where you state what other program can be installed and why.

"In the C Script Editor of XPaint, the External editor in the File menu
 will invoke <program_name>.

 The gs package will be needed for PDF/PS reading, netpbm will be needed
 for external conversion, lpr for printing.
 
<ETC....>
"

* In the desktop file:

Categories=Application;Graphics;

is not good. Application is not a registered category:
http://standards.freedesktop.org/menu-spec/latest/apa.html

Please use:
Graphics;2DGraphics;RasterGraphics;

* Please use Mandriva description because it is more accurate (and  based on author feedback too)
http://svn.mandriva.com/cgi-bin/viewvc.cgi/packages/cooker/xpaint/current/SPECS/xpaint.spec?revision=452128&view=markup

* Devel package is probably not needed and wrong. Xpaint uses some very strange C scripting. Without files you package in devel it cannot work. BTW, both Debian and Mandriva do not have a devel package.

"The C script Editor can be used to write a script in language C,
defining e.g. a programmable filter. 

This possibility will appeal to programmers who want to design their
own filter routines.

The "Compile" button compiles the C script as a shared ???.so library,
and, if successful, links dynamically that library to Xpaint.

Several examples will be found in the share/filters subdirectory. Notice 
that you can use this feature only if gcc has been installed on your 
system."

* The following lines seem unneeded because both variables should be correctly set when invoking make.
perl -p -i -e "s|CXXDEBUGFLAGS = .*|CXXDEBUGFLAGS = $RPM_OPT_FLAGS|" Makefile
perl -p -i -e "s|CDEBUGFLAGS = .*|CDEBUGFLAGS = $RPM_OPT_FLAGS|" Makefile

* You should probably patch Local.config to use "better" Fedora programs. This is what you can find there:
EDITOR = emacs -fn 9x15 -cr green -ms red -bg lightyellow -fg black
POSTSCRIPT_VIEWER = gv
EXTERN_VIEWER = xv
Comment 12 Thomas Janssen 2009-10-05 09:50:30 EDT
(In reply to comment #10)
> (In reply to comment #9)
> > Sounds not too good. I will have fedora-legal look over it. Sorry.
> 
> Here it is stated that MIT license is compatible with both GPLv2 and GPLv3:
> https://fedoraproject.org/wiki/Licensing:Main#Good_Licenses
> 
> This means that you can combine code released under the MIT license with code
> released under the GNU GPL in one larger program:
> http://www.gnu.org/licenses/gpl-faq.html#WhatDoesCompatMean

Yes, that's true. My point is "different parts with different licences, *including* MIT and GPL". Including MIT and GPL means there are others. As long as we dont know *what* licenses the others are, it's a no-go from my understanding.

Upstream could be bothered to find out. On the plus side would be that it's included in Debian. But i have no idea what "debianized" means. He could ask the Debian developer to find out.

Sorry.


-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 13 Andrea Musuruane 2009-10-05 09:59:47 EDT
(In reply to comment #12)
> Yes, that's true. My point is "different parts with different licences,
> *including* MIT and GPL". Including MIT and GPL means there are others. As long
> as we dont know *what* licenses the others are, it's a no-go from my
> understanding.

Ops... sorry for my misunderstanding.
 
> Upstream could be bothered to find out. On the plus side would be that it's
> included in Debian. But i have no idea what "debianized" means. He could ask
> the Debian developer to find out.

+1. I agree that asking Debian package maintainer is the way to go. Debian is usually very careful about license issues.

PS "Debianized" just means that someone has make a Debian .deb package.
Comment 15 Paulo Roma Cavalcanti 2009-10-10 05:40:12 EDT
http://koji.fedoraproject.org/koji/taskinfo?taskID=1739238

I will check upstream about the license.
Comment 16 Paulo Roma Cavalcanti 2009-10-14 18:11:30 EDT
Version 2.8.4 was just release under GPLv3:

http://orion.lcg.ufrj.br/RPMS/src/xpaint-2.8.4-7.fc10.src.rpm

http://orion.lcg.ufrj.br/RPMS/SPECS/xpaint.spec
Comment 17 Thomas Janssen 2009-10-21 07:43:39 EDT
I want to give that package-review to Andrea Musuruane. He has done the bigger part of the review.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 18 Thomas Spura 2009-10-22 10:44:31 EDT
Just a few comments (I'm no sponsor anyway ;)):

rpmlint output is not clean.

- Take a look to https://fedoraproject.org/wiki/Packaging/Guidelines#Beware_of_Rpath for the error with rpath
- use %configure instead just ./configure

- Doesn't the folder /usr/share/xpaint/include/ belongs to /usr/include/xpaint?
  At least this folder should go into a -devel subpackage.
  What about the c_scripts folder? It it needed at runtime?



- There is a new version upstream (again^^).
Comment 19 Andrea Musuruane 2009-10-22 10:56:03 EDT
(In reply to comment #18)
> - Doesn't the folder /usr/share/xpaint/include/ belongs to /usr/include/xpaint?
>   At least this folder should go into a -devel subpackage.
>   What about the c_scripts folder? It it needed at runtime?

Please read previous comments about this.

Andrea.
Comment 20 Andrea Musuruane 2009-10-22 10:59:34 EDT
(In reply to comment #17)
> I want to give that package-review to Andrea Musuruane. He has done the bigger
> part of the review.

Thomas, if you want to go on, you have my blessing. I have little time now to do this review.

Andrea.
Comment 21 Paulo Roma Cavalcanti 2009-10-22 11:38:28 EDT
(In reply to comment #18)
> Just a few comments (I'm no sponsor anyway ;)):

I do not need a sponsor.

http://koji.fedoraproject.org/koji/userinfo?userID=866

> 
> rpmlint output is not clean.
> 
> - Take a look to
> https://fedoraproject.org/wiki/Packaging/Guidelines#Beware_of_Rpath for the
> error with rpath
> - use %configure instead just ./configure
> 
> - Doesn't the folder /usr/share/xpaint/include/ belongs to /usr/include/xpaint?
>   At least this folder should go into a -devel subpackage.
>   What about the c_scripts folder? It it needed at runtime?
> 

The question is whether we need a devel package or not. Originally, all .c and .h were in a devel subpackage. However, xpaint allows one to write plugins in C,
and Andrea suggested the suppression of the devel package, as I understood.

I will take a look at version 2.8.5

Thanks.
Comment 22 Paulo Roma Cavalcanti 2009-10-22 12:11:50 EDT
New SRPM URL:

http://orion.lcg.ufrj.br/RPMS/src/xpaint-2.8.5-8.fc10.src.rpm
Comment 23 Christoph Wickert 2009-10-22 12:45:53 EDT
This does not build in mock because you are running chrpath, but the chrpath package is no BuildRequires, so it's not installed.

Did you try to remove the rpath with sed after configure? chrpath should only be used as last resort.
Comment 24 Christoph Wickert 2009-10-22 12:48:10 EDT
BTW: The release is wrong: You increased the version, so the Release starts with 1 again, not 8.
Comment 25 Paulo Roma Cavalcanti 2009-10-22 13:17:39 EDT
(In reply to comment #23)
> This does not build in mock because you are running chrpath, but the chrpath
> package is no BuildRequires, so it's not installed.
> 
> Did you try to remove the rpath with sed after configure? chrpath should only
> be used as last resort.  

Yes I tried. The problem is that xpaint uses "xmkmf".

I have uploaded the wrong .src.rpm, and that is why chrpath did not appear
as a BR. Can you download it again? Same link.
Comment 26 Christoph Wickert 2009-10-22 14:18:09 EDT
The link cannot be correct because the package name needs to change:
http://orion.lcg.ufrj.br/RPMS/src/xpaint-2.8.5-1.fc10.src.rpm
instead of                                     ^
http://orion.lcg.ufrj.br/RPMS/src/xpaint-2.8.5-8.fc10.src.rpm
Comment 27 Paulo Roma Cavalcanti 2009-10-22 14:56:24 EDT
Here it is:

http://orion.lcg.ufrj.br/RPMS/src/xpaint-2.8.5-1.fc10.src.rpm
Comment 28 Thomas Spura 2009-10-22 15:10:36 EDT
(In reply to comment #21)

> I do not need a sponsor.

Sorry, I read something about it in comment #4 and found no answer to it in several comments down (or STRG+f didn't find it for me ;)).


I hugely dislike the way this program handles includes and so on and therefor don't want to review this… I hope there is a way to patch this.....

You could make a devel package anyway and the normal package could require it always. This way rpmlint would be cleaner and the devel package can be noarch, because it's just source code and save a bit space across arches, but for 172K I doubt, this beeing much better^^


Judging from [1] it should always be worth splitting.


[1]https://fedoraproject.org/wiki/Features/NoarchSubpackages
Comment 29 Paulo Roma Cavalcanti 2009-10-23 06:10:54 EDT
xpaint can be run without any included "C code". 
However, a user can call the C script editor from the tab options,
and then load and compile from the template tab, a filter, a surface, etc...

Rpmlint gives us some guidelines, but there are several Fedora packages
which produce dozen of warnings. I personally do not care if xpaint has a devel package or not. It is really better a single package with everything inside, though.

Our role as packagers is secondary. We just need to ship a useful system. If we dislike some upstream decisions based on our packaging standards is not the main point, in my opinion.

Xpaint is an old program, which was part of RedHat in the past.
However, it has made some improvements along the time. According to the current developer:
 
"Versions >= 2.8.4 include a new high performance postscript
generator. The PS files are e.g. often less than 50% the size of the (already
compressed) PS files that gimp produces, and the new thing is very fast -
see enclosed 'ppmtops' prototype. I don't think there are so many
open source programs, even among the very established ones, that are
"aware" enough of these algorithms; of course 'ppmtops' relies on
a combination of PNG predictors with LZW compression which had been
patented till around 2004, so maybe it's the reason - if Adobe would
provide better support for unpatented compression schemes as an
alternative to LZW, one could do even better."
Comment 30 Paulo Roma Cavalcanti 2009-10-27 09:10:33 EDT
SRPM URL: http://orion.lcg.ufrj.br/RPMS/src/xpaint-2.8.6.1-1.fc10.src.rpm
SPEC: http://orion.lcg.ufrj.br/RPMS/SPECS/xpaint.spec 

Created a devel subpackage, which is noarch:

[lua:~/specs] rpmlint xpaint
xpaint.x86_64: E: devel-dependency xpaint-devel
1 packages and 0 specfiles checked; 1 errors, 0 warnings.

[lua:~/specs] rpmlint xpaint-devel
xpaint-devel.noarch: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Scratch build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1772200


I am just curious whether noarch subpackages will really become 
common or not.
Comment 31 Paulo Roma Cavalcanti 2009-11-01 20:19:44 EST
By the way, all .h files are now in:

/usr/include/xpaint
Comment 32 Paulo Roma Cavalcanti 2009-11-02 05:55:03 EST
Another possibility with c_scripts in the main package, since they
are not real devel files (but rpmlint spits some warnings):


SRPM URL:http://orion.lcg.ufrj.br/RPMS/src/xpaint-2.8.6.1-2.fc10.src.rpm


I think it is easy to conclude the review now, since there is no much room for
new changes.

Thanks.
Comment 33 Gwyn Ciesla 2009-11-04 10:59:11 EST
Hmm, a few issues on a full review.  

One, they didn't update the GPL licensing text in all places, but that's OK since the GPLv2 stuff says GPLv2+.  Given that, might want to change license tage to GPLv3+.

Source URL should be Source0: http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz, modified for the project name.

Why is the URL tag pointing to the SF project, and not the SF web page?

Mock build and BRs are OK.

I've also read this entire review and considered the issues raised and their solutions.

I think once the various tags above are fixed, I could approve.
Comment 34 Paulo Roma Cavalcanti 2009-11-04 11:26:30 EST
(In reply to comment #33)
> Hmm, a few issues on a full review.  
> 
> One, they didn't update the GPL licensing text in all places, but that's OK
> since the GPLv2 stuff says GPLv2+.  Given that, might want to change license
> tage to GPLv3+.

Done, and there is a new version 2.8.7 available.


> 
> Source URL should be Source0:
> http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz, modified
> for the project name.

http://downloads.sourceforge.net/sf-xpaint/xpaint-2.8.7.tar.gz

redirects to:

http://sourceforge.net/projects/sf-xpaint/files/

> 
> Why is the URL tag pointing to the SF project, and not the SF web page?

Because the SF web page does not have any downloadable link, as you can see:

http://sf-xpaint.sourceforge.net/

It is kind of unusable, IMHO.

> 
> Mock build and BRs are OK.
> 
> I've also read this entire review and considered the issues raised and their
> solutions.
> 
> I think once the various tags above are fixed, I could approve.  

SPEC: http://orion.lcg.ufrj.br/RPMS/SPECS/xpaint.spec

SRPM: http://orion.lcg.ufrj.br/RPMS/src/xpaint-2.8.7-1.fc10.src.rpm
Comment 35 Gwyn Ciesla 2009-11-04 12:55:22 EST
Sounds reasonable re: URL.

Re: Source0.  Yes, that's what it redirects to.  Today.  We use the one in the guidelines in case it eventually redirects elsewhere, to prevent breakage.

Switch, and we're good.
Comment 36 Paulo Roma Cavalcanti 2009-11-04 13:13:44 EST
Done. In fact, I posted the wrong link. This is the right one:

http://downloads.sourceforge.net/sf-xpaint/xpaint-2.8.7.tar.bz2


Same URLS, which are already updated.
Comment 37 Gwyn Ciesla 2009-11-04 13:37:42 EST
Looks good, APPROVED.
Comment 38 Paulo Roma Cavalcanti 2009-11-05 06:44:03 EST
Thanks Jon, for finishing this review, and Hans de Goede for
all of his suggestions for improving this packages.


New Package CVS Request
=======================
Package Name: xpaint
Short Description: An X Window System image editing or paint program
Owners: roma
Branches: F-10 F-11 F-12
InitialCC: roma
Comment 39 Kevin Fenzi 2009-11-06 15:43:23 EST
cvs done.
Comment 40 Paulo Roma Cavalcanti 2011-01-08 10:09:36 EST
Jason, the package is mine in Fedora.

Thanks.


Package Change Request
======================
Package Name: xpaint
New Branches: el5 el6
Owners: roma
Comment 41 Jason Tibbitts 2011-01-08 13:27:52 EST
Git done (by process-git-requests).

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