Bug 511895 - Review Request: clutter-imcontext - IMContext Framework Library for Clutter
Summary: Review Request: clutter-imcontext - IMContext Framework Library for Clutter
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Orcan Ogetbil
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FedoraMoblin20 506804 513452
TreeView+ depends on / blocked
 
Reported: 2009-07-15 14:40 UTC by Peter Robinson
Modified: 2009-07-27 19:53 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-07-27 19:53:16 UTC
Type: ---
Embargoed:
oget.fedora: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Comment 1 Orcan Ogetbil 2009-07-18 17:23:08 UTC
I'll take this one. Could you review bug #512500? It should be very simple.

Comment 2 Orcan Ogetbil 2009-07-19 00:11:19 UTC
Here is my review for this package. Most are minor things but there is a license issue.

- Package builds in koji rawhide:
   http://koji.fedoraproject.org/koji/taskinfo?taskID=1484535

- rpmlint is silent

* Package fails to build on F-11:
  checking for CLUTTER... configure: error: Package requirements (glib-2.0 
  clutter-0.9 >= 0.9.3) were not met:
  No package 'clutter-0.9' found

I think this BR on clutter-devel should contain the explicit version requirement.
Is this package for rawhide only?

! All relevant doc files should be packaged. It would be good to include the AUTHORS and ChangeLog files in %doc

* The files clutter-imtext.{c,h} are LGPLv3. The rest is LGPLv2. AFAIK These are incompatible. This needs to be clarified by upstream.

! BR's automake, autoconf are unnecessary since these will always be dragged in by libtool. Similarly, BR's glib2-devel, pkgconfig will be dragged in by clutter-devel. However, it is not a blocker to keep them.

* It looks like the devel package should require clutter-devel. There are other dependencies as well such as glib2-devel, pango-devel. But these will be dragged in by clutter-devel.

! The %{name} macro could be used more consistently.

? Do we need to package the Makefile* stuff in %doc? One last suggestion: You could use "%doc doc/*" instead of just "%doc doc/" to avoid an extra subdirectory.

Comment 3 Peter Robinson 2009-07-19 12:15:27 UTC
> Here is my review for this package. Most are minor things but there is a
> license issue.

Thanks

> * Package fails to build on F-11:
>   checking for CLUTTER... configure: error: Package requirements (glib-2.0 
>   clutter-0.9 >= 0.9.3) were not met:
>   No package 'clutter-0.9' found
> 
> I think this BR on clutter-devel should contain the explicit version
> requirement.
> Is this package for rawhide only?

Yes. It requires the newer clutter. There was discusson on whether to put a clutter09 package in F11 to allow for testing of various things that need it but it was decided the amount of work required wasn't worth it. I've added an explicit version requirement 
 
> ! All relevant doc files should be packaged. It would be good to include the
> AUTHORS and ChangeLog files in %doc

Added.

> * The files clutter-imtext.{c,h} are LGPLv3. The rest is LGPLv2. AFAIK These
> are incompatible. This needs to be clarified by upstream.

I've emailed upstream and if necessary will follow up with a clarification from legal.
http://lists.moblin.org/pipermail/dev/2009-July/005515.html

> ! BR's automake, autoconf are unnecessary since these will always be dragged in
> by libtool. Similarly, BR's glib2-devel, pkgconfig will be dragged in by
> clutter-devel. However, it is not a blocker to keep them.

Noted.

> * It looks like the devel package should require clutter-devel. There are other
> dependencies as well such as glib2-devel, pango-devel. But these will be
> dragged in by clutter-devel.

Added.

> ! The %{name} macro could be used more consistently.

From looking I couldn't see exactly what you meant by this. Further details would be great.

> ? Do we need to package the Makefile* stuff in %doc? One last suggestion: You
> could use "%doc doc/*" instead of just "%doc doc/" to avoid an extra
> subdirectory.  

I'll look at why the Makefiles are there and if there's specific docs that can be generated will look at getting that done during the build.

SPEC same as before. New SRPM: http://pbrobinson.fedorapeople.org/clutter-imcontext-0.1.2-2.fc11.src.rpm

Comment 4 Orcan Ogetbil 2009-07-19 19:58:35 UTC
(In reply to comment #3)
> > Here is my review for this package. Most are minor things but there is a
> > license issue.
> 
> Thanks
> 

You're welcome. I noticed one additional issue that I missed in the first pass:
The ./autogen.sh script is calling ./configure, which is redundant since you will be calling it properly via %configure.

So, we should remove the first of this duplicate call by something like
   sed -i '/configure/d' autogen.sh
in %prep.

> From looking I couldn't see exactly what you meant by this. Further details
> would be great.
> 

Sure. For instance, you have these lines in the specfile:
   ...
   Files for development with %{name}.
   ...
   %{_libdir}/libclutter-imcontext-0.1.so.0
   ...
   %{_includedir}/clutter-imcontext-0.1

My understanding of macro consistency is that the above should be
   ...
   Files for development with %{name}.
   ...
   %{_libdir}/lib%{name}-0.1.so.0
   ...
   %{_includedir}/%{name}-0.1
or
   ...
   Files for development with clutter-imcontext.
   ...
   %{_libdir}/libclutter-imcontext-0.1.so.0
   ...
   %{_includedir}/clutter-imcontext-0.1

Btw, you can also save some time when you are doing updates by variable-izing "0.1" by something like
   %global majorver 0.1
on top of the specfile. Then you can use %majorver everywhere else. Just a suggestion.

> > ? Do we need to package the Makefile* stuff in %doc? One last suggestion: 
> > You could use "%doc doc/*" instead of just "%doc doc/" to avoid an extra
> > subdirectory.  
> 
> I'll look at why the Makefiles are there and if there's specific docs that can
> be generated will look at getting that done during the build.
> 

I checked the configure script. We can pass a --enable-gtk-doc flag to %configure to actually build these documents in human readable gtk-doc/html format. What we have done by "%doc doc/" is just packaging the raw documentation, which is not that useful.

Comment 5 Peter Robinson 2009-07-20 10:19:06 UTC
SRPM: http://pbrobinson.fedorapeople.org/clutter-imcontext-0.1.2-3.fc11.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1486257

Updated. The license issue has been clarified and I've put a note in the spec file until the next release which will fix the issue completely. Response below and quoted in the spec file.
http://lists.moblin.org/pipermail/dev/2009-July/005522.html

I've updated all of the above. Thanks

Comment 6 Peter Robinson 2009-07-23 16:18:06 UTC
Is there anything outstanding?

Comment 7 Orcan Ogetbil 2009-07-25 22:02:58 UTC
Peter,
Sorry I was away for a couple days due to unplanned circumstances. I will return to the review tonight or tomorrow.

Comment 8 Peter Robinson 2009-07-25 22:07:16 UTC
No probs. Thanks!

Comment 9 Orcan Ogetbil 2009-07-26 02:25:25 UTC
This won't work:
   ./autogen.sh --disable-static --enable-gtk-doc
   #%configure --disable-static --enable-gtk-doc
Check the build.log. You will see that the configure is executed twice. It's also not safe. Because %configure is a multi-line macro. If you do a #%configure, you are commenting out only the first line, which defines CFLAGS. If you want to comment out a macro use double %, e.g. #%%configure. But IMHO the best solution is to eliminate the ./configure call from the autogen.sh (Note that this is not the only solution :)).

I keep seeing some clutter broken deps in rawhide for the last few rawhide reports. Is this package affected by this?

Comment 10 Peter Robinson 2009-07-26 08:13:56 UTC
(In reply to comment #9)
> This won't work:
>    ./autogen.sh --disable-static --enable-gtk-doc
>    #%configure --disable-static --enable-gtk-doc
> Check the build.log. You will see that the configure is executed twice. It's
> also not safe. Because %configure is a multi-line macro. If you do a
> #%configure, you are commenting out only the first line, which defines CFLAGS.
> If you want to comment out a macro use double %, e.g. #%%configure. But IMHO
> the best solution is to eliminate the ./configure call from the autogen.sh
> (Note that this is not the only solution :)).

I've uncommented the configure. There are lots of ways of fixing it. I personally don't see the problem with it running it twice other than a a few extra cpu cycles :-)

> I keep seeing some clutter broken deps in rawhide for the last few rawhide
> reports. Is this package affected by this?  

No, this package isn't affected as its written against 0.9/1.0. The reason for the breakages in rawhide is due to the various clutter and pyclutter packages haven't had 0.9 releases to update them to the new apis.

SRPM: http://pbrobinson.fedorapeople.org/clutter-imcontext-0.1.2-4.fc11.src.rpm

Comment 11 Peter Robinson 2009-07-26 16:23:42 UTC
Added in post and tested "sed -i '/configure/d' autogen.sh"

koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1523493
SRPM: http://pbrobinson.fedorapeople.org/clutter-imcontext-0.1.2-4.fc11.src.rpm

Comment 12 Orcan Ogetbil 2009-07-26 19:25:11 UTC
Thanks! Imagine all packages take 3 seconds longer to build. These would add up to total build times that can't be ignored. I believe that it's good to keep things at minimum (e.g. remove unnecessary BR's, commands etc).

I think everything is good now

----------------------------------------------------
This package (clutter-imcontext) is APPROVED by oget
----------------------------------------------------

Comment 13 Peter Robinson 2009-07-26 19:31:16 UTC
New Package CVS Request
=======================
Package Name: clutter-imcontext
Short Description: IMContext Framework Library for Clutter
Owners: pbrobinson
Branches: F-11
InitialCC:

Comment 14 Kevin Fenzi 2009-07-26 19:45:54 UTC
cvs done.

Comment 15 Peter Robinson 2009-07-27 09:29:00 UTC
Built and on its way to rawhide

Comment 16 Peter Robinson 2009-07-27 19:53:16 UTC
In rawhide. Thanks for the review :-)


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