Bug 511895 - Review Request: clutter-imcontext - IMContext Framework Library for Clutter
Review Request: clutter-imcontext - IMContext Framework Library for Clutter
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FedoraMoblin20 506804 513452
  Show dependency treegraph
 
Reported: 2009-07-15 10:40 EDT by Peter Robinson
Modified: 2009-07-27 15:53 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-07-27 15:53:16 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Comment 1 Orcan Ogetbil 2009-07-18 13:23:08 EDT
I'll take this one. Could you review bug #512500? It should be very simple.
Comment 2 Orcan Ogetbil 2009-07-18 20:11:19 EDT
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 08:15:27 EDT
> 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 15:58:35 EDT
(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 06:19:06 EDT
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 12:18:06 EDT
Is there anything outstanding?
Comment 7 Orcan Ogetbil 2009-07-25 18:02:58 EDT
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 18:07:16 EDT
No probs. Thanks!
Comment 9 Orcan Ogetbil 2009-07-25 22:25:25 EDT
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 04:13:56 EDT
(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 12:23:42 EDT
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 15:25:11 EDT
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 15:31:16 EDT
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 15:45:54 EDT
cvs done.
Comment 15 Peter Robinson 2009-07-27 05:29:00 EDT
Built and on its way to rawhide
Comment 16 Peter Robinson 2009-07-27 15:53:16 EDT
In rawhide. Thanks for the review :-)

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