Bug 519538 - Review Request: clutter-sharp - C#/.NET bindings to Clutter
Review Request: clutter-sharp - C#/.NET bindings to Clutter
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: 2009-08-26 20:26 EDT by Christian Krause
Modified: 2009-10-25 19:24 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-10-25 19:24:32 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 Christian Krause 2009-08-26 20:26:21 EDT
Spec URL: http://chkr.fedorapeople.org/review/clutter-sharp.spec
SRPM URL: http://chkr.fedorapeople.org/review/clutter-sharp-0-0.1.20090827.fc11.src.rpm
Description: C#/.NET bindings to Clutter

Package builds on F12 for all architectures:
https://koji.fedoraproject.org/koji/taskinfo?taskID=1636949
Comment 1 Michel Alexandre Salim 2009-08-26 22:58:14 EDT
Taking this review. Could you put a comment above the source line, describing how exactly to reproduce the tarball you're using? With SVN and Hg, I'd recommend just using the revision number instead of revision date, but with Git's long revision number...

say

# DIRNAME=... ./clutter-sharp-make-git-snapshot.sh the-commit-id
Source0: ...
Comment 2 Christian Krause 2009-08-27 20:09:27 EDT
I have added the commit id (usually a part of it is sufficient) to the spec file, enhanced the snapshot script and documented its usage.
- commit id is needed to allow reproducible creation of tarball
- date is needed to create correct tag for snapshot releases

Spec URL: http://chkr.fedorapeople.org/review/clutter-sharp.spec
SRPM URL:
http://chkr.fedorapeople.org/review/clutter-sharp-0-0.2.20090828.fc11.src.rpm
Comment 3 Paul Lange 2009-08-28 09:23:09 EDT
Thanks for creating the package. I don't have the time to make a full review but I noticed some things.

* mixed use of %{buildroot} and $RPM_BUILD_ROOT ($RPM_BUILD_ROOT only in %install)
* latest changelog version is 0-0.3 but release field says 0.2
* shouldn't the %defattr filed be %defattr(-,root,root,-)?
Comment 4 Christian Krause 2009-08-28 10:32:28 EDT
(In reply to comment #3)
> Thanks for creating the package. I don't have the time to make a full review
> but I noticed some things.
> 
> * mixed use of %{buildroot} and $RPM_BUILD_ROOT ($RPM_BUILD_ROOT only in
> %install)
> * latest changelog version is 0-0.3 but release field says 0.2
> * shouldn't the %defattr filed be %defattr(-,root,root,-)?  

Thanks for the comment - all issues fixed:

Spec URL: http://chkr.fedorapeople.org/review/clutter-sharp.spec
SRPM URL:
http://chkr.fedorapeople.org/review/clutter-sharp-0-0.3.20090828.fc11.src.rpm
Comment 5 Michel Alexandre Salim 2009-08-31 17:17:01 EDT
Almost ready to pass review; there is a trivial summary change to make, and a more serious case of some dependencies not being listed

Koji build (F-12):
http://koji.fedoraproject.org/koji/taskinfo?taskID=1646731

MUST

• rpmlint:

source -- clean

clutter-sharp.x86_64: E: no-binary
clutter-sharp.x86_64: W: only-non-binary-in-usr-lib

These are harmless, rpmlint is not Mono-aware

clutter-sharp-devel.x86_64: W: summary-not-capitalized pkgconfig file for clutter-sharp
clutter-sharp-devel.x86_64: W: no-documentation

The first should probably be fixed. The default spec file has

  Development files for %{name}

which could be used. Esp. since we're packaging more than just the .pc file!

OK package name
OK spec file name
OK package guideline-compliant
OK license complies with guidelines
OK license field accurate
OK license file not deleted
OK spec in US English
OK spec legible
OK source matches upstream
OK builds under >= 1 archs, others excluded (Koji)
OK build dependencies complete (Koji)

FAIL on the other hand, runtime dependencies are not complete

     rpm -q --requires clutter-sharp

     rpm -e clutter 2>out.txt
     cat out.txt | grep sharp

  shows that the dependency on clutter is not picked up; this should be
  added manually.

  Also, once clutter-gtk can be enabled (prod the maintainer for an update?)
  you'd want to Requires: clutter-gtk, and meanwhile, now you want to
  %exclude %{_libdir}/pkgconfig/clutter-gtk-sharp.pc 

NA locales handled using %find_lang, no %{_datadir}/locale
NA library -> ldconfig
NA relocatable: give reason
OK own all directories
OK no dupes in %files
OK permission
OK %clean RPM_BUILD_ROOT
OK macros used consistently
OK Package contains code
OK headers in -devel
OK if contains *.pc, req pkgconfig
OK devel requires versioned base package
OK clean buildroot before install
OK filenames UTF-8

SHOULD
OK package build in mock on all architectures
?  package functioned as described
     not tested
OK scriplets are sane
OK require package not files
Comment 6 Michel Alexandre Salim 2009-09-11 15:24:52 EDT
Ping? There are some small changes that need to be made, otherwise the package is OK.
Comment 7 Christian Krause 2009-09-16 16:37:15 EDT
Thank you very much for the review.

(In reply to comment #6)
> Ping? There are some small changes that need to be made, otherwise the package
> is OK.  

Sorry for the late response. I came just back from 2 weeks of vacation. ;-)

The new packages can be found here:

Spec URL: http://chkr.fedorapeople.org/review/clutter-sharp.spec
SRPM URL:
http://chkr.fedorapeople.org/review/clutter-sharp-0-0.4.20090828.fc11.src.rpm  

(In reply to comment #5)
> clutter-sharp-devel.x86_64: W: summary-not-capitalized pkgconfig file for
> clutter-sharp
> clutter-sharp-devel.x86_64: W: no-documentation
> 
> The first should probably be fixed. The default spec file has
> 
>   Development files for %{name}
> 
> which could be used. Esp. since we're packaging more than just the .pc file!

Fixed.

> OK build dependencies complete (Koji)
> 
> FAIL on the other hand, runtime dependencies are not complete
> 
>      rpm -q --requires clutter-sharp
> 
>      rpm -e clutter 2>out.txt
>      cat out.txt | grep sharp
> 
>   shows that the dependency on clutter is not picked up; this should be
>   added manually.

Fixed.
 
>   Also, once clutter-gtk can be enabled (prod the maintainer for an update?)

Unfortunately clutter-gtk is not yet released yet: http://www.clutter-project.org/sources/clutter-gtk/0.10/ . I'll try to keep this in mind and create a bug entry for clutter-gtk once 0.10.3 is released.

>   you'd want to Requires: clutter-gtk, and meanwhile, now you want to
>   %exclude %{_libdir}/pkgconfig/clutter-gtk-sharp.pc 

Fixed.
Comment 8 Michel Alexandre Salim 2009-09-19 21:28:52 EDT
(In reply to comment #7)
> Thank you very much for the review.
> 
> (In reply to comment #6)
> Sorry for the late response. I came just back from 2 weeks of vacation. ;-)
Ah, nice!

> The new packages can be found here:
> 
> Spec URL: http://chkr.fedorapeople.org/review/clutter-sharp.spec
> SRPM URL:
> http://chkr.fedorapeople.org/review/clutter-sharp-0-0.4.20090828.fc11.src.rpm  
Will review tomorrow; I had to dual-boot to Windows to do some .NET testing, and now am stuck waiting for a rather big download from a tediously slow server ([rant]seriously, upstreams should consider using BitTorrent with a private tracker -- [/rant])

> >   Also, once clutter-gtk can be enabled (prod the maintainer for an update?)
> 
> Unfortunately clutter-gtk is not yet released yet:
> http://www.clutter-project.org/sources/clutter-gtk/0.10/ . I'll try to keep
> this in mind and create a bug entry for clutter-gtk once 0.10.3 is released.
> 
How about packaging a pre-release snapshot? As long as we know for certain -- or reasonably so -- that the final version will come out before we freeze for final, that's probably good enough.
Comment 9 Christian Krause 2009-09-20 18:47:54 EDT
(In reply to comment #8)
> > Unfortunately clutter-gtk is not yet released yet:
> > http://www.clutter-project.org/sources/clutter-gtk/0.10/ . I'll try to keep
> > this in mind and create a bug entry for clutter-gtk once 0.10.3 is released.
> > 
> How about packaging a pre-release snapshot? As long as we know for certain --
> or reasonably so -- that the final version will come out before we freeze for
> final, that's probably good enough.  

I've checked the clutter-gtk's git-repository and it looks like that there was not much activity in the last week:
http://git.clutter-project.org/cgit.cgi?url=clutter-gtk/log/

Since the final freeze for F12 is on 9/29 I believe it would be too risky. :-(
Comment 10 Michel Alexandre Salim 2009-09-21 13:50:01 EDT
Changes look fine -- APPROVED.
Comment 11 Christian Krause 2009-09-21 16:50:31 EDT
New Package CVS Request
=======================
Package Name: clutter-sharp
Short Description: C#/.NET bindings to Clutter
Owners: chkr
Branches: F-11 F-10
InitialCC:
Comment 12 Jason Tibbitts 2009-09-21 22:10:01 EDT
CVS done.

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