Bug 590244 - Review Request: pinta - Simple Paint Application
Review Request: pinta - Simple Paint Application
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Andrea Musuruane
Fedora Extras Quality Assurance
:
: 569038 (view as bug list)
Depends On: 612233
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-08 06:23 EDT by Paul Lange
Modified: 2010-12-09 08:15 EST (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-12-09 08:15:44 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
musuruan: fedora‑review+
petersen: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Paul Lange 2010-05-08 06:23:14 EDT
Spec URL: http://palango.fedorapeople.org/pinta/pinta.spec
SRPM URL: http://palango.fedorapeople.org/pinta/pinta-0.3-1.20100412git.fc12.src.rpm
Description: An easy to use drawing and image editing program
Comment 1 Chen Lei 2010-05-08 07:40:47 EDT
*** Bug 569038 has been marked as a duplicate of this bug. ***
Comment 2 Andrea Musuruane 2010-05-08 09:24:46 EDT
Let's start with some remarks:

- The code is licensed under the MIT license while the icons are licensed as CC-BY. Therefore:

# the code is licensed under the MIT license while the icons are licensed as CC-BY
License:        MIT and CC-BY

(adding a comment in the SPEC file is really suggest to avoid further questions).

- These explicit Requires are not needed and they must be dropped:
Requires:		mono-core, gtk-sharp2

- It is not needed to copy SOURCE1 in the top source directory.

- You must use desktop-file-install to install desktop files.

- Comment and Generic name should be different in desktop files. See Debian package for ideas.

- You are using an icon found in an external package without requiring it. Anyway I suggest you to use the icon Debian for consistency. Moreover it is derived from pinta logo.

- You are not installing any doc.

- Rpmlint output is not clean. You must fix:
pinta.x86_64: W: incoherent-version-in-changelog 0.3-2.20100412git ['0.3-1.20100412git.fc12', '0.3-1.20100412git']
pinta.x86_64: W: no-documentation

- If you run /usr/bin/pinta you'll get an error stating "Could not open file: ". This is because you are not correctly escaping "$@" when creating the bash script.


Debian Pinta pacakge:
http://patch-tracker.debian.org/package/pinta/
Comment 3 Andrea Musuruane 2010-05-24 08:22:47 EDT
Ping.
Comment 4 Paul Lange 2010-06-03 12:24:36 EDT
Sorry for the long delay, didn't have much time.

Recently pinta upstream got autotools support. I think it would be clever to wait for the next release? Comments?

Anyway, I'm not a bash pro, how do I correctly escape "$@"?
Comment 5 Andrea Musuruane 2010-06-06 08:45:29 EDT
(In reply to comment #4)
> Sorry for the long delay, didn't have much time.
> 
> Recently pinta upstream got autotools support. I think it would be clever to
> wait for the next release? Comments?

If you think it is better to wait a new upstream release (the one that will use autotools) then you must mark this bug as NotReady:
https://fedoraproject.org/wiki/Package_Review_Process#The_Whiteboard

Anyway I would at least make a release to fix the aforementioned problems (so that they are not forgotten).

> Anyway, I'm not a bash pro, how do I correctly escape "$@"?    

"\$@"
Comment 6 Paul Lange 2010-07-07 16:41:19 EDT
0.4 is out. Pakcaging should be much easier with the new version.
I will update the review as soon as bug #612233 is resolved which stops us from compiling pinta 0.4.
Comment 7 Paul Lange 2010-07-29 07:21:09 EDT
I updated the package to 0.4 and also fixed all problems mentioned above.

See:
http://palango.fedorapeople.org/pinta/pinta.spec
http://palango.fedorapeople.org/pinta/pinta-0.4-1.fc13.src.rpm

test build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2356648
Comment 8 Andrea Musuruane 2010-07-29 10:06:58 EDT
* I suggest you to use desktop-file-validator instead of desktop-file-install if no modification in the desktop file are needed:
https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage

* Anyway, I see from the koji build that there is a warning in the desktop file that must be fixed: 'warning: key "MimeType" is a list and does not have a semicolon as trailing character, fixing'. Please report this problem upstream.

* If an application installs icons into one of the subdirectories in %{_datadir}/icons/ (such as hicolor), icon caches must be updated so that the installed icons show up in menus right after package installation:
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

* Any relevant documentation included in the source distribution should be included in the package (e.g. readme.txt, todo.txt):
https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

* 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 must be included in %doc.
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

* You must keep a changelog even if this package is still being reviewed.

* You must BuildRequires: desktop-file-utils
https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usage

* You must fix directory ownership. The package must Requires: hicolor-icon-theme.

* Debian and upstream have some bugfixing patches available:
http://git.debian.org/?p=pkg-cli-apps/packages/pinta.git;a=tree;f=debian/patches;h=599486d6fcdcf4efec772299dc51b0179a77411a;hb=HEAD
https://bugs.launchpad.net/pinta

I just had a quick look at the spec file. I still have to play with it deeply.
Comment 9 Paul Lange 2010-07-29 12:29:13 EDT
Thanks for the fast look at this Andrea!

I fixed all problems, hope thats fine now:
http://palango.fedorapeople.org/pinta/pinta.spec
http://palango.fedorapeople.org/pinta/pinta-0.4-2.fc13.src.rpm

build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2356719
Comment 10 Andrea Musuruane 2010-07-30 10:58:17 EDT
* It seems the koji build has not been done with the uploaded spec file because I see you used desktop-file-install in the koji build and desktop-file-validate in the spec file.

* The license file "license-pdn.txt" is missing among docs.

* All patches in Fedora spec files SHOULD have a comment above them about their upstream status. Any time you create a patch, it is best practice to file it in an upstream bug tracker, and include a link to that in the comment above the patch.

https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment


This was still another quick glance. I hope to have a closer look next weekend.
Comment 11 Andrea Musuruane 2010-07-31 09:27:23 EDT
(In reply to comment #9)
> I fixed all problems, hope thats fine now:
> http://palango.fedorapeople.org/pinta/pinta.spec
> http://palango.fedorapeople.org/pinta/pinta-0.4-2.fc13.src.rpm

* This SRPM has not the above SPEC file in it.

* If I use the above SPEC file instead, the build will fail because your patch for fixing the mime types in the desktop file is wrong.

The problem in the desktop file is that there is a semicolon missing at the end of the entry "...;image/openraster" -> "...;image/openraster;". You removed all the semicolons instead.

* Moreover your patches should have the package name in it. Usually something like pinta-0.4-desktop-file-mime-list.patch or pinta-desktop-file-mime-list.patch.

* After fixing the above bug and installing the rpm I hit another problem. The /usr/bin/pinta file has the following content:

#!/bin/sh 
exec /usr/bin/mono /usr/lib/pinta/Pinta.exe "$@"

As you can see there is not way that this will work on 64 bits architectures because they use lib64 instead of lib. Please fix this issue in %prep.

* %define installdir %{_libdir} is not needed and must be removed.
Comment 13 Paul Lange 2010-08-07 07:13:34 EDT
this was a wrong build, this one is the correct:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2386929
Comment 14 Ismael Olea 2010-08-18 10:40:47 EDT
Paul:

if it helps, you can borrow some ideas from CJ spec, which is Mono based too: http://cvs.fedoraproject.org/viewvc/rpms/chronojump/devel/chronojump.spec?revision=1.4&view=markup
Comment 15 Robert Scheck 2010-08-30 19:13:08 EDT
Any news here? In the meantime, pinta 0.6 got released by the way. And is there
any help needed? I played with pinta 0.4 a bit and it seems to satisfy my needs.
Comment 16 Andrea Musuruane 2010-08-31 04:35:01 EDT
(In reply to comment #15)
> Any news here? In the meantime, pinta 0.6 got released by the way. And is there
> any help needed? I played with pinta 0.4 a bit and it seems to satisfy my
> needs.

I can't see the release of pinta 0.6 anywhere.

BTW, I wanted to review this, but I'm really busy right now. If someone else wants to step in, please do!
Comment 17 Paul Lange 2010-10-14 08:59:17 EDT
Doesn't seem like 0.6 is already out.

Is there anyone who can finish the review. I'd really appreciate that!
Comment 18 Andrea Musuruane 2010-11-20 09:15:02 EST
Pinta 0.5 has been release on November 2nd, 2010. Please update your package. I'll try to review it after that.
Comment 19 Paul Lange 2010-11-30 12:22:25 EST
I updated the package but unfortunately I get an error and can't find information about it:

+ desktop-file-validate /home/paul/rpmbuild/BUILDROOT/pinta-0.5-1.fc14.i386//usr/share/applications/pinta.desktop
/home/paul/rpmbuild/BUILDROOT/pinta-0.5-1.fc14.i386//usr/share/applications/pinta.desktop: error: file contains at least one line ending with a carriage return, while lines should only be separated by a line feed character. First such line is: "[Desktop Entry]"

Does anyone has an idea on how to solve this?
Comment 20 gatlibs 2010-11-30 14:14:26 EST
Did you use an editor that uses non-unix carriage returns? Windows or OS X style, perhaps?
Comment 21 Paul Lange 2010-11-30 18:05:32 EST
No, this happens with the original tarball from the website.
Comment 22 Andrea Musuruane 2010-12-01 04:45:13 EST
(In reply to comment #19)
> I updated the package but unfortunately I get an error and can't find
> information about it:
> 
> + desktop-file-validate
> /home/paul/rpmbuild/BUILDROOT/pinta-0.5-1.fc14.i386//usr/share/applications/pinta.desktop
> /home/paul/rpmbuild/BUILDROOT/pinta-0.5-1.fc14.i386//usr/share/applications/pinta.desktop:
> error: file contains at least one line ending with a carriage return, while
> lines should only be separated by a line feed character. First such line is:
> "[Desktop Entry]"
> 
> Does anyone has an idea on how to solve this?

Try this in %prep:
%{__sed} -i 's/\r//' path_to/pinta.deskop
Comment 23 Paul Lange 2010-12-01 05:51:09 EST
Thanks for the help!

Updates package:
http://palango.fedorapeople.org/pinta/pinta.spec
http://palango.fedorapeople.org/pinta/pinta-0.5-1.fc14.src.rpm


However there are quite a lot rpmlint errors about line endings. I reported a bug upstream but do not know if it makes sense to correct all this stuff now.

[paul@paullaptop i686]$ rpmlint pin*
pinta.i686: I: enchant-dictionary-not-found en_US
pinta.i686: E: no-binary
pinta.i686: W: only-non-binary-in-usr-lib
pinta.i686: E: script-without-shebang /usr/share/applications/pinta.desktop
pinta.i686: E: script-without-shebang /usr/share/icons/hicolor/scalable/apps/pinta.svg
pinta.i686: E: wrong-script-end-of-line-encoding /usr/share/icons/hicolor/scalable/apps/pinta.svg
pinta.i686: W: spurious-executable-perm /usr/share/doc/pinta-0.5/readme.txt
pinta.i686: E: wrong-script-end-of-line-encoding /usr/share/doc/pinta-0.5/readme.txt
pinta.i686: W: spurious-executable-perm /usr/share/doc/pinta-0.5/license-mit.txt
pinta.i686: E: wrong-script-end-of-line-encoding /usr/share/doc/pinta-0.5/license-mit.txt
pinta.i686: E: script-without-shebang /usr/share/pixmaps/pinta.xpm
pinta.i686: E: wrong-script-end-of-line-encoding /usr/share/pixmaps/pinta.xpm
pinta.i686: W: spurious-executable-perm /usr/share/man/man1/pinta.1.gz
pinta.i686: E: wrong-script-end-of-line-encoding /usr/bin/pinta
pinta.i686: W: spurious-executable-perm /usr/share/doc/pinta-0.5/license-pdn.txt
pinta.i686: E: wrong-script-end-of-line-encoding /usr/share/doc/pinta-0.5/license-pdn.txt
pinta.i686: W: spurious-executable-perm /usr/share/doc/pinta-0.5/todo.txt
pinta.i686: E: wrong-script-end-of-line-encoding /usr/share/doc/pinta-0.5/todo.txt
Comment 24 Andrea Musuruane 2010-12-01 06:02:08 EST
(In reply to comment #23)
> However there are quite a lot rpmlint errors about line endings. I reported a
> bug upstream but do not know if it makes sense to correct all this stuff now.

All the reported rpmlint errors and warnings must be corrected to pass the package review. 

It is up to you to wait for upstream to fix them or fix them yourself in the spec file. I suggest you the latter but if you want to wait for upstream please put add NotReady to the Whiteboard field and remove it only after these problems will be solved.
Comment 26 Andrea Musuruane 2010-12-05 09:42:03 EST
Paul, your latest package do not build under x86_64. I get the following error:

+ /usr/lib/rpm/redhat/brp-strip-static-archive /usr/bin/strip
+ /usr/lib/rpm/brp-python-bytecompile /usr/bin/python 1
+ /usr/lib/rpm/redhat/brp-python-hardlink
+ /usr/lib/rpm/redhat/brp-java-repack-jars
Processing files: pinta-0.5-2.fc14.x86_64
errore: File non trovato: /home/andrea/rpmbuild/BUILDROOT/pinta-0.5-2.fc14.x86_64/usr/lib64/pinta
Esecuzione(%doc) in corso: /bin/sh -e /var/tmp/rpm-tmp.IsVCDo
+ umask 022
+ cd /home/andrea/rpmbuild/BUILD
+ cd pinta-0.5
+ DOCDIR=/home/andrea/rpmbuild/BUILDROOT/pinta-0.5-2.fc14.x86_64/usr/share/doc/pinta-0.5
+ export DOCDIR
+ rm -rf /home/andrea/rpmbuild/BUILDROOT/pinta-0.5-2.fc14.x86_64/usr/share/doc/pinta-0.5
+ /bin/mkdir -p /home/andrea/rpmbuild/BUILDROOT/pinta-0.5-2.fc14.x86_64/usr/share/doc/pinta-0.5
+ cp -pr todo.txt readme.txt license-mit.txt license-pdn.txt /home/andrea/rpmbuild/BUILDROOT/pinta-0.5-2.fc14.x86_64/usr/share/doc/pinta-0.5
+ exit 0


Errori di compilazione RPM:
    File non trovato: /home/andrea/rpmbuild/BUILDROOT/pinta-0.5-2.fc14.x86_64/usr/lib64/pinta
Comment 28 Andrea Musuruane 2010-12-08 11:59:24 EST
Package Review
==============

Key:
- = N/A
x = Check
! = Problem
? = Not evaluated

=== REQUIRED ITEMS ===
[x]  Package is named according to the Package Naming Guidelines. [1]
[x]  Spec file name must match the base package %{name}, in the format %{name}.spec.
[x]  Spec file is legible and written in American English.
[x]  Spec file lacks Packager, Vendor, PreReq tags.
[x]  Spec uses macros instead of hard-coded directory names.
[!]  Package consistently uses macros.
[x]  Macros in Summary, %description expandable at SRPM build time.
[x]  PreReq is not used.
[x]  Requires correct, justified where necessary.
[x]  All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [2]
[x]  Buildroot is correct (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)).
[x]  Package run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) and the beginning of %install.
[-]  Package use %makeinstall only when ``make install DESTDIR=...'' doesn't work.
[x]  Package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[x]  The spec file handles locales properly.
[x]  Changelog in prescribed format.
[x]  Rpmlint output is silent.
$ rpmlint /home/andrea/rpmbuild/SRPMS/pinta-0.5-3.fc14.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint /home/andrea/rpmbuild/RPMS/x86_64/pinta-0.5-3.fc14.x86_64.rpm 
pinta.x86_64: E: no-binary
pinta.x86_64: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 1 errors, 1 warnings.

These are fine, as defined by the mono packaging guidelines.

[x]  License field in the package spec file matches the actual license.
[x]  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 %doc.
[-]  License file installed when any subpackage combination is installed.
[x]  Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [3,4]
[x]  Sources contain only permissible code or content.
[x]  Sources used to build the package matches the upstream source, as provided in the spec URL.
     MD5SUM this package     : e7bbee973f9dcf86b4c9a5a232f513f9
     MD5SUM upstream package : e7bbee973f9dcf86b4c9a5a232f513f9
[-]  Compiler flags are appropriate.
[-]  %build honors applicable compiler flags or justifies otherwise.
[-]  ldconfig called in %post and %postun if required.
[x]  Package must own all directories that it creates.
[x]  Package does not own files or directories owned by other packages.
[x]  Package requires other packages for directories it uses.
[x]  Package does not contain duplicates in %files.
[!]  Permissions on files are set properly.
[x]  Each %files section contains %defattr.
[x]  No %config files under /usr.
[-]  %config files are marked noreplace or the reason is justified.
[x]  Package contains a properly installed %{name}.desktop using desktop-file-install file if it is a GUI application. [5]
[x]  Package contains a valid .desktop file.
[x]  Package contains code, or permissible content.
[-]  Package contains a SysV-style init script if in need of one.
[x]  File names are valid UTF-8.
[-]  Large documentation files are in a -doc subpackage, if required.
[x]  Package uses nothing in %doc for runtime.
[x]  Package contains no bundled libraries.
[-]  Header files in -devel subpackage, if present.
[-]  Static libraries in -static subpackage, if present.
[x]  Package contains no static executables.
[-]  Package requires pkgconfig, if .pc files are present.
[-]  Development .so files in -devel subpackage, if present.
[-]  Fully versioned dependency in subpackages, if present.
[x]  Package does not contain any libtool archives (.la).
[x]  Useful -debuginfo package or justification otherwise.
[x]  Rpath absent or only used for internal libs.
[x]  Package does not generate any conflict.
[x]  Package does not contains kernel modules.
[x]  Package is not relocatable.
[x]  Package successfully compiles and builds into binary rpms on at least one supported architecture.
F14/x86_64
[x]  Package is not known to require ExcludeArch.
"ExcludeArch: sparc64" is fine. Mono is available only for sparc32.
[x]  Package installs properly.
[x]  Package obeys FHS, except libexecdir and /usr/target.
[!]  Package meets the Packaging Guidelines. [6]

=== SUGGESTED ITEMS ===
[x]  Package functions as described.
[x]  Latest version is packaged.
[x]  Package does not include license text files separate from upstream.
[-]  If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
[-]  Description and summary sections in the package spec file contains translations for supported Non-English languages, if available.
[x]  SourceX is a working URL.
[x]  SourceX / PatchY prefixed with %{name}.
[x]  Final provides and requires are sane (rpm -q --provides and rpm -q --requires).
[-]  %check is present and all tests pass.
[-]  Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[?]  Reviewer should test that the package builds in mock.
[-]  Package should compile and build into binary rpms on all supported architectures.
[x]  Dist tag is present.
[!]  Spec use %global instead of %define.
[x]  Scriptlets must be sane, if used.
[-]  The placement of pkgconfig(.pc) files are correct.
[x]  No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[!]  Packages should try to preserve timestamps of original installed files.
[-]  File based requires are sane.
[!]  Man pages included for all executables.
[x]  Uses parallel make.
[-]  Patches link to upstream bugs/comments/lists or are otherwise justified.

=== Issues ===
1. Please do not reference Paint.NET in the description:
http://fedoraproject.org/wiki/Packaging/Guidelines#Trademarks_in_Summary_or_Description

2. Macros are not used consistently. You used %{__sed} and make (not %{__make}). Just use the short form. Command macros are more readable in this way.

3. chmod -x pinta.1 in %prep and not pinta.1.gz in %install

4. Use %global instead of %define.
http://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define

5. When adding file copying commands in the spec file, consider using a command that preserves the files' timestamps, eg. cp -p or install -p. 
http://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

Items 1 to 4 are mandatory, otherwise the package is in quite good shape. Good work!



[1] https://fedoraproject.org/wiki/Packaging:NamingGuidelines
[2] https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2 
[3] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines
[4] https://fedoraproject.org/wiki/Licensing:Main
[5] https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files
[6] https://fedoraproject.org/wiki/Packaging:Guidelines
Comment 29 Paul Lange 2010-12-08 14:47:03 EST
Thanks a lot for the review!

1 to 4 are resoloved. However I cannot find any cp in the spec file, so I have no idea on how to solve 5.

http://palango.fedorapeople.org/pinta/pinta.spec
http://palango.fedorapeople.org/pinta/pinta-0.5-4.fc14.src.rpm
Comment 30 Andrea Musuruane 2010-12-08 16:54:49 EST
(In reply to comment #29)
> 1 to 4 are resoloved. However I cannot find any cp in the spec file, so I have
> no idea on how to solve 5.

That's because autotools use install and not cp :-) Try running %configure this way (I haven't test it but it should work):

%configure INSTALL="install -p"

If you want you can solve 5 before importing pinta in git. The package for me is fine.

APPROVED!
Comment 31 Paul Lange 2010-12-08 17:42:38 EST
New Package SCM Request
=======================
Package Name: pinta
Short Description: An easy to use drawing and image editing program
Owners: palango
Branches: f14
InitialCC:
Comment 32 Robert Scheck 2010-12-08 17:51:36 EST
I would say, it should be

  make install DESTDIR=%{buildroot} INSTALL='install -p'

rather

  %configure INSTALL="install -p"

in the *.spec file.
Comment 33 Jens Petersen 2010-12-08 19:44:51 EST
Git done (by process-git-requests).
Comment 34 Paul Lange 2010-12-09 08:15:44 EST
Pinta is now available in devel.

Thanks to everyone who helped!

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