Bug 691317 - Review Request: libmash - Mash is a small library for using real 3D models within a Clutter scene
Review Request: libmash - Mash is a small library for using real 3D models wi...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michel Alexandre Salim
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-28 04:29 EDT by Richard Hughes
Modified: 2011-04-05 11:59 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-04-05 11:59:43 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
michel: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Richard Hughes 2011-03-28 04:29:14 EDT
Spec URL: http://people.freedesktop.org/~hughsient/temp/libmash.spec
SRPM URL: http://people.freedesktop.org/~hughsient/temp/libmash-0.1.0-1.fc15.src.rpm
Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2953084
Description:

Mash is a small library for using real 3D models within a Clutter
scene. Models can be exported from Blender or other 3D modeling
software as PLY files and then used as actors. It also supports a
lighting model with animatable lights.

Note: gnome-color-manager upstream depends on mash to show the ICC profile 3D gamut hull. It's an optional dependency, but recommended.

Note2: the package is called libmash for two reasons.
 1. There is only a library and docs, no binary files
 2. There already exists a 'mash' package in Fedora.
Comment 1 Richard Hughes 2011-03-28 04:53:50 EDT
Output of rpmlint:

[hughsie@hughsie-t510-rawhide rpmbuild]$ rpmlint */libmash*
libmash.x86_64: W: spelling-error %description -l en_US animatable -> stableman, imitable
libmash.src: W: spelling-error %description -l en_US animatable -> stableman, imitable
4 packages and 1 specfiles checked; 0 errors, 2 warnings.
Comment 2 Elad Alfassa 2011-03-30 04:54:16 EDT
I'll do an unofficial review.
Comment 3 Elad Alfassa 2011-03-30 04:58:01 EDT
I'll not do an unofficial review, because I can't install all the build dependencies, mirrors give me 404 error... Sorry.
Comment 4 Michel Alexandre Salim 2011-03-30 11:12:26 EDT
Will take the review
Comment 5 Michel Alexandre Salim 2011-03-31 08:48:46 EDT
There are several issues still, see review below:

* TODO Review [60%]
  - [X] Names [2/2]
    - [X] Package name
    - [X] Spec name
  - [X] Package version [2/2]
	http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Package_Versioning
    - [X] Version number
	  http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Version_Tag
    - [X] Release tag
	  http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Release_Tag
	  http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Pre-Release_packages
  - [X] Meets [[http://fedoraproject.org/wiki/Packaging/Guidelines][guidelines]]
  - [X] Source files match upstream
	$ sha1sum mash-0.1.0.tar.bz2 ../SOURCES/mash-0.1.0.tar.bz2 
        162242e7008c76b1a481db10bb32c0d5454a94ff  mash-0.1.0.tar.bz2
        162242e7008c76b1a481db10bb32c0d5454a94ff  ../SOURCES/mash-0.1.0.tar.bz2
  - [-] [[http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries][No bundled libraries]]
	bundles RPly: http://w3.impa.br/~diego/software/rply/
  - [-] License [3/4]
    - [X] License is Fedora-approved
    - [X] No licensing conflict
    - [-] License field accurate
	  see bundled issue. RPly is under MIT, must be mentioned if
	  bundling is approved
    - [X] License included iff packaged by upstream
  - [X] rpmlint [2/2]
    - [X] on src.rpm
	  libmash.src: W: spelling-error %description -l en_US animatable -> stableman, imitable
          1 packages and 0 specfiles checked; 0 errors, 1 warnings.
          ==> safe to ignore
    - [X] on x86_64.rpm
	  libmash.x86_64: W: spelling-error %description -l en_US animatable -> stableman, imitable
          3 packages and 0 specfiles checked; 0 errors, 1 warnings.

  - [-] Language & locale [2/3]
    - [X] Spec in US English
    - [-] Spec legible
	  the word 'Mash' probably should not be in the summary
	  (rpmlint does not catch it because it's not libmash)
    - [X] Use %find_lang to handle locale files
	  N/A

  - [-] Build [1/3]
    - [X] Koji results
	  http://koji.fedoraproject.org/koji/taskinfo?taskID=2963278
    - [-] BRs complete
	  There's an optional dependency on libmx, should it not be
	  added as a BR?
    - [-] Directory ownership
      - girepository-1.0 is owned by gdk-pixbuf2 so it's probably OK
      - but %{_datadir}/gir-1.0 is not owned by any package pulled in
	by mash
  - [-] Spec inspection [8/10]
    - [X] ldconfig for libraries
    - [X] No duplicate files
    - [X] File permissions
    - [X] Filenames must be UTF-8
    - [X] no BuildRoot definition ([[https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag][except if targeting EPEL5]])
    - [X] No %clean section
          (except for RHEL:
           https://fedoraproject.org/wiki/Packaging/Guidelines#.25clean)
    - [-] %buildroot cleaned on %install
	  This still needs to be done
    - [X] Macro usage consistent
    - [-] Documentation [2/3]
      - [X] If large docs, separate -doc
	    N/A
      - [X] %doc files are non-essential
      - [-] Relevant docs packaged
	    Shouldn't README be included?
    - [X] Development [5/5]
      - [X] Headers in -devel
      - [X] If versioned .so's, unversioned in -devel
      - [X] Static only if necessary, put in -static
	    N/A
      - [X] -devel, -static requires main
      - [X] No .la
Comment 6 Richard Hughes 2011-03-31 10:46:57 EDT
(In reply to comment #5)
>   - [-] [[http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries][No
> bundled libraries]]
>  bundles RPly: http://w3.impa.br/~diego/software/rply/

I've sent a patch upstream to the mash project to use the system rply -- and included that patch in version -2. This means that although rply is in the mash tarball, the system version is used. I've therefore added rply-devel as a BR.

>     - [-] Spec legible
>    the word 'Mash' probably should not be in the summary
>    (rpmlint does not catch it because it's not libmash)

Fixed in -2.

>     - [-] BRs complete
>    There's an optional dependency on libmx, should it not be
>    added as a BR?

It's only used by the not-installed demo lighting program, and I've added a note in -2 about the "missing" dep.

>     - [-] Directory ownership
>       - girepository-1.0 is owned by gdk-pixbuf2 so it's probably OK
>       - but %{_datadir}/gir-1.0 is not owned by any package pulled in
>  by mash

I think it's best to own both in this case, which I've done in -2.

>     - [-] %buildroot cleaned on %install
>    This still needs to be done

This package is suitable for F15 and rawhide, so no need for F10 and below compatibility.

>       - [-] Relevant docs packaged
>      Shouldn't README be included?

Good catch, thanks. Fixed in -2.

New SRPM: http://people.freedesktop.org/~hughsient/temp/libmash-0.1.0-2.fc15.src.rpm
New Spec: http://people.freedesktop.org/~hughsient/temp/libmash.spec
New Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2963688

The rpmlint output is unchanged. Thanks for the super-quick review!

Richard.
Comment 7 Michel Alexandre Salim 2011-04-01 10:16:14 EDT
Ah, indeed. the templates created by mock are really out of date!

All the review issues are addressed -- package is APPROVED
Comment 8 Richard Hughes 2011-04-05 11:41:20 EDT
New Package SCM Request
=======================
Package Name: libmash
Short Description: A library for using real 3D models within a Clutter scene
Owners: rhughes
Branches: f15
InitialCC: rhughes
Comment 9 Jason Tibbitts 2011-04-05 11:46:20 EDT
Git done (by process-git-requests).
Comment 10 Richard Hughes 2011-04-05 11:59:43 EDT
Building rawhide and F15 now. Thanks guys.

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