Bug 608319 - Review Request: memaker - An avatar creator
Summary: Review Request: memaker - An avatar creator
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mario Blättermann
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-06-26 19:58 UTC by Ankur Sinha (FranciscoD)
Modified: 2011-09-07 00:24 UTC (History)
7 users (show)

Fixed In Version: memaker-20100110-1.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-08-04 18:43:00 UTC
Type: ---
mario.blaettermann: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Ankur Sinha (FranciscoD) 2010-06-26 19:58:52 UTC
Spec URL: http://ankursinha.fedorapeople.org/memaker/memaker.spec
SRPM URL: http://ankursinha.fedorapeople.org/memaker/memaker-1.5-1.fc13.src.rpm
Description: 

MeMaker gives users a wide variety of images that, when placed 
together, create an avatar. This avatar is intended to represent 
the way that this person is in some way. The goal of the project 
is to have enough images that anyone can create an image that 
they feel would closely represent them without having to use a photo
in the image itself.

Comment 1 Ankur Sinha (FranciscoD) 2010-06-26 20:00:46 UTC
hey,

There are some issues already:

It's a python module, but doesn't use any setuptools etc. I've manually installed *all* files. Therefore, there are no egg files at the moment (I couldn't figure out how to generate them manually). 

regards,
Ankur

Comment 2 Till Maas 2010-07-01 10:41:34 UTC
(In reply to comment #1)

> There are some issues already:
> 
> It's a python module, but doesn't use any setuptools etc. I've manually
> installed *all* files. Therefore, there are no egg files at the moment (I
> couldn't figure out how to generate them manually). 

Imho the best solution is to ask upstream to use a setup.py file or write one yourself and submit it upstream.

Btw. it looks to me that %{python_sitelib}/MeMaker/ is not owned, so a %dir %{python_sitelib}/MeMaker/ needs to be added to %files.

And the patch should be submitted upstream if appropriate and a comment added that explains it's upstream status or why it is not appropriate for upstream.

Comment 3 Till Maas 2010-07-01 10:42:16 UTC
%{_datadir}/%{name} seems also not to be owned.

Comment 4 Jason Tibbitts 2010-11-23 20:05:43 UTC
It's been several months without response to the above commentary.  I'll close this soon if there's no further progress.

Comment 5 Ankur Sinha (FranciscoD) 2011-01-04 12:14:53 UTC
Hi,

I'm sorry this has been without comment for such a long period. I'll get down to this within the coming week.

regards,
Ankur

Comment 6 Ankur Sinha (FranciscoD) 2011-01-11 02:33:46 UTC
Hello,

SPEC: http://ankursinha.fedorapeople.org/memaker/memaker.spec

SRPM: http://ankursinha.fedorapeople.org/memaker/memaker-20100110-1.fc15.src.rpm

RPM: http://ankursinha.fedorapeople.org/memaker/memaker-20100110-1.fc14.noarch.rpm

rest of mock build results at: http://ankursinha.fedorapeople.org/memaker/

I've made this one from a bzr checkout. This has a setup.py and seems to work well. The setup.py isn't complete, but it installs most of the files correctly. 

Thank you.

Comment 7 Mario Blättermann 2011-04-30 20:35:35 UTC
$ rpmlint -v *
memaker.noarch: I: checking
memaker.noarch: I: checking-url https://launchpad.net/memaker (timeout 10 seconds)
memaker.noarch: W: hidden-file-or-dir /usr/share/memaker/themes/glyphFace/Nose/..svg
memaker.noarch: W: no-manual-page-for-binary memaker
memaker.src: I: checking
memaker.src: I: checking-url https://launchpad.net/memaker (timeout 10 seconds)
memaker.src: W: invalid-url Source0: memaker-20100110-bzr.tar.gz
memaker.spec: W: invalid-url Source0: memaker-20100110-bzr.tar.gz
2 packages and 1 specfiles checked; 0 errors, 4 warnings.

The mentioned hidden file isn't hidden really. It is the filename ~.svg, which is erroneously recognized as hidden.

Please replace all occurences of the application name in the spec with %{name}.

Wouldn't it be better to use the "install" command instead of cp, especially to keep timestamps?

Comment 8 Martin Gieseking 2011-05-01 07:10:17 UTC
(In reply to comment #7)
> Please replace all occurences of the application name in the spec with %{name}.

That's optional and only useful in particular cases where the package name is directly related to another entry in the spec file, e.g. the name of the tarball.
Using %{name} in %install or %files is not required because there's no benefit of doing so. If you want to use it there anyway, it should be consistent, though (no mix between %{name} and the plain name string). 


> Wouldn't it be better to use the "install" command instead of cp, especially to
> keep timestamps?

cp -a preserves the timestamps too.

Comment 9 Ankur Sinha (FranciscoD) 2011-05-04 17:30:26 UTC
Hi Mario, Martin,

Thanks for the comments. I've updated the spec. 

* Wed May 04 2011 Ankur Sinha <ankursinha AT fedoraproject DOT org> - 20100110-1
- #608319
- replaced "memaker" with name macro 
- changed cp -v to cp -av to preserve timestamps
- update to upstream dev 

http://ankursinha.fedorapeople.org/memaker/memaker.spec

http://ankursinha.fedorapeople.org/memaker/memaker-20100110-1.fc15.src.rpm

http://ankursinha.fedorapeople.org/memaker/memaker-20100110-1.fc15.noarch.rpm

Regards,
Ankur

Comment 10 Mario Blättermann 2011-05-07 17:03:22 UTC
(In reply to comment #3)
> %{_datadir}/%{name} seems also not to be owned.

Just put a / after this entry to let your package own this directory.

http://fedoraproject.org/wiki/Packaging:UnownedDirectories#Wildcarding_Files_inside_a_Created_Directory

Comment 11 Ankur Sinha (FranciscoD) 2011-05-09 11:38:35 UTC
(In reply to comment #10)
> (In reply to comment #3)
> > %{_datadir}/%{name} seems also not to be owned.
> 
> Just put a / after this entry to let your package own this directory.
> 
> http://fedoraproject.org/wiki/Packaging:UnownedDirectories#Wildcarding_Files_inside_a_Created_Directory

Hi Mario,

Updated spec:

http://ankursinha.fedorapeople.org/memaker/memaker.spec

SRPM:

http://ankursinha.fedorapeople.org/memaker/memaker-20100110-1.fc16.src.rpm

Rest of the results from my mock build can be found here:

http://ankursinha.fedorapeople.org/memaker/

Thanks,
Ankur

Comment 12 Mario Blättermann 2011-07-28 19:05:31 UTC
Koji scratch build:

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

$ rpmlint -i -v *
memaker.noarch: I: checking
memaker.noarch: I: checking-url https://launchpad.net/memaker (timeout 10 seconds)
memaker.noarch: W: hidden-file-or-dir /usr/share/memaker/themes/glyphFace/Nose/..svg
The file or directory is hidden. You should see if this is normal, and delete
it from the package if not.

memaker.noarch: W: no-manual-page-for-binary memaker
Each executable in standard binary directories should have a man page.

memaker.src: I: checking
memaker.src: I: checking-url https://launchpad.net/memaker (timeout 10 seconds)
memaker.src: W: invalid-url Source0: memaker-20100110-bzr.tar.gz
The value should be a valid, public HTTP, HTTPS, or FTP URL.

memaker.spec: W: invalid-url Source0: memaker-20100110-bzr.tar.gz
The value should be a valid, public HTTP, HTTPS, or FTP URL.

/home/mariobl/Arbeitsfläche/memaker/memaker-20100110-1.fc16.src/memaker.spec: W: invalid-url Source0: memaker-20100110-bzr.tar.gz
The value should be a valid, public HTTP, HTTPS, or FTP URL.

2 packages and 2 specfiles checked; 0 errors, 5 warnings.


For the »hidden« file, see comment #7.
Of course, the source package cannot be named as an URL. That's why, no real issues.

---------------------------------
key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
    GPLv3+
[+] MUST: The License field in the package spec file must match the actual
license.
[+] MUST: The file containing the text of the license(s) for the package must
be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum *
    c64b256c500f9eae6f78a03332087553  memaker-20100110-bzr.tar.gz
    a432d5d0f771ccd2edf48f860249b25a  memaker-20100110-bzr.tar.gz.packaged

    The checksums doesn't match. I assume it is due to differing algorithms
    in gzip, which is a common problem with other packages which are based
    on a VCS checkout, too.

[+] MUST: The package MUST successfully compile and build into binary rpms on
at least one primary architecture.
- See Koji build above.
[.] MUST: If the package does not successfully compile, build or work on an
architecture, ...
[+] MUST: All build dependencies must be listed in BuildRequires.
[.] MUST: The spec file MUST handle locales properly.
[+] MUST: If a package installs files below %{_datadir}/icons, the icon cache
must be updated.
[.] MUST: Packages storing shared library files (not just symlinks) must call
ldconfig in %post and %postun.
[.] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in %files.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Packages must not provide RPM dependency information when that
information is not global in nature, or are otherwise handled.
[.] MUST: When filtering automatically generated RPM dependency information,
the filtering system implemented by Fedora must be used.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[.] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[.] MUST: If a package contains library files with a suffix (e.g.
libfoo.so.1.1), ...
[.] MUST: devel packages must require the base package using a fully versioned
dependency.
[.] MUST: Packages must NOT contain any .la libtool archives.
[+] MUST: Packages containing GUI applications must include a %{name}.desktop
file
[+] MUST: .desktop files must be properly installed with desktop-file-install
in the %install section.
[+] MUST: Packages must not own files or directories already owned by other
packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

[.] SHOULD: If the source package does not include license text(s) as a
    separate file from upstream, the packager SHOULD query upstream...

[+] SHOULD: Timestamps of files should be preserved.
[+] SHOULD: The reviewer should test that the package builds in mock.
    See Koji build above (which uses mock anyway)
[+] SHOULD: The reviewer should test that the package functions as described.
    Works properly, given the current development state.

[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base
package using a fully versioned dependency.
[.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin,
/usr/bin, or /usr/sbin ...
[.] SHOULD: Your package should contain man pages for binaries/scripts.
    Currently no man page available.


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

PACKAGE APPROVED

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


For Fedora (not for EPEL) you may remove the %defattr and %clean macros and the BuildRoot definition.

Comment 13 Ankur Sinha (FranciscoD) 2011-07-29 10:45:19 UTC
Thank you for the review Mario!

I'll remove the %defattr etc when I push it to scm. 

Ankur

Comment 14 Ankur Sinha (FranciscoD) 2011-07-29 10:53:30 UTC
New Package SCM Request
=======================
Package Name: memaker
Short Description: An avatar creator
Owners: ankursinha
Branches: f14 f15 f16
InitialCC:

Comment 15 Gwyn Ciesla 2011-07-29 11:57:51 UTC
Git done (by process-git-requests).

Comment 16 Fedora Update System 2011-07-30 10:19:28 UTC
memaker-20100110-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/memaker-20100110-1.fc15

Comment 17 Fedora Update System 2011-07-30 10:19:35 UTC
memaker-20100110-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/memaker-20100110-1.fc16

Comment 18 Fedora Update System 2011-07-30 10:19:44 UTC
memaker-20100110-1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/memaker-20100110-1.fc14

Comment 19 Fedora Update System 2011-08-01 20:17:43 UTC
memaker-20100110-1.fc16 has been pushed to the Fedora 16 testing repository.

Comment 20 Fedora Update System 2011-08-28 05:34:49 UTC
memaker-20100110-1.fc16 has been pushed to the Fedora 16 stable repository.

Comment 21 Fedora Update System 2011-09-07 00:21:27 UTC
memaker-20100110-1.fc15 has been pushed to the Fedora 15 stable repository.

Comment 22 Fedora Update System 2011-09-07 00:24:22 UTC
memaker-20100110-1.fc14 has been pushed to the Fedora 14 stable repository.


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