Bugzilla (bugzilla.redhat.com) will be under maintenance for infrastructure upgrades and will not be available on July 31st between 12:30 AM - 05:30 AM UTC. We appreciate your understanding and patience. You can follow status.redhat.com for details.
Bug 529374 - Review Request: ethos - Plugin framework for GLib
Summary: Review Request: ethos - Plugin framework for GLib
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Simon
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 529375 529561
TreeView+ depends on / blocked
 
Reported: 2009-10-16 12:55 UTC by Peter Robinson
Modified: 2010-01-14 23:21 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-01-14 23:21:15 UTC
Type: ---
cassmodiah: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Peter Robinson 2009-10-16 12:55:32 UTC
SPEC: http://pbrobinson.fedorapeople.org/ethos.spec
SRPM: http://pbrobinson.fedorapeople.org/ethos-0.2.2-0.1.fc12.src.rpm

Description:
Ethos is a plugin framework that is written in C using the GLib and GObject 
libraries. The goal is to have a single framework for applications that lower 
the barrier to entry for extensions. To enable as many communities as possible, 
various language bindings are provided to allow extensions in the language of 
choice.

Ethos includes a GUI library as well named libethos-ui. This library provides 
a gtk+ widget for managing plugins within your application. Typically, you can 
simply add this to a "Plugins" tab in your applications preferences dialog.

koij: http://koji.fedoraproject.org/koji/taskinfo?taskID=1749979

Comment 1 Josephine Tannhäuser 2009-11-22 09:46:04 UTC
Just an informal review

OK - MUST: $ rpmlint 
ethos.i686: W: devel-file-in-non-devel-package /usr/lib/ethos/plugin-loaders/libcloader.so
ethos.i686: W: devel-file-in-non-devel-package /usr/lib/ethos/plugin-loaders/libpythonloader.so
ethos.i686: W: devel-file-in-non-devel-package /usr/lib/ethos/plugin-loaders/libjsloader.so
these libfiles are needed, so it doesn't make sense to move them to -devel. if so, the basepkg will require the develpkg by default. this would be senseless
ethos-devel.i686: W: no-documentation
ethos-python.i686: W: no-documentation
ethos-vala.i686: W: no-documentation
you have a doc package, so this should be okay
NOT OKAY - MUST: Named according to the Package Naming Guidelines
the name of the package should be libethos, because it is a lib
Group:          System Environment/Libraries
NOT OKAY - MUST: Spec file name matches the base package %{name} 
should be libethos
OK - MUST: Package meets the Packaging Guidelines
NOT OKAY - MUST: Fedora approved license and meets the Licensing Guidelines
License is LGPLv2+, and this is a good license
NOT OKAY - MUST: License field in spec file matches the actual license
License:        LGPLv2+
OK - MUST: License files included in %doc
OK - MUST: Spec is in American English
OK - MUST: Spec is legible
NOT OKAY - MUST: Sources match the upstream source by MD5
your command "git clone git://git.dronelabs.com/ethos" give me a newer revision, you should add the command to co this version. then the md5sum od DL and pacakge should match.
OK - MUST: Successfully compiles and builds into binary rpms on i686
CAN'T TEST IT, I HAVEN'T A KOJI-ACCESS - MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch.
If you want to import this to f12, too, you should do a kojibuild for f12 to see if this will be build on all f12 supported architectures.
OK - MUST: All build dependencies are listed in BuildRequires.
N/A - MUST: Handles locales properly with %find_lang
OK - MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
N/A - MUST: If the package is designed to be relocatable, the packager muststate this fact in the request for review.
OK - MUST: Owns all directories that it creates
OK - MUST: No duplicate files in the %files listing
OK - MUST: Permissions on files are set properly, includes %defattr(...)
OK - MUST: Package has a %clean section, which contains rm -rf %{buildroot}.
OK - MUST: Consistently uses macros
OK - MUST: Package contains code, or permissable content
OK - MUST: Large documentation files should go in a -doc subpackage
OK - MUST: Files included as %doc do not affect the runtime of the application
N/A - MUST: Header files must be in a -devel package
N/A - MUST: Static libraries must be in a -static package
OK - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'.
N/A - MUST: If a package contains library files with a suffix, then library files that end in .so must go in a -devel package.
N/A - MUST: devel packages must require the base package using a fully versioned dependency
OK - MUST: The package does not contain any .la libtool archives.
N/A - MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section.
OK - MUST: Package does not own files or directories already owned by other packages.
OK - MUST: At the beginning of %install, the package runs rm -rf %{buildroot}.
OK - MUST: All filenames valid UTF-8


SHOULD Items:
OK - SHOULD: Source package includes license text(s) as a separate file.
N/A - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
OK - SHOULD: Builds in mock.
CAN'T TEST, BECAUSE I HAVEN'T AN ACCESS TO KOJI - SHOULD: Compiles and builds into binary rpms on all supported architectures.
OK - SHOULD: Functions as described.
Is required for emerillon. and emerillon works for me, so this should working, too....
OK - SHOULD: Scriptlets are used, those scriptlets must be sane.
OK - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
OK - SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg
N/A - SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.


Other items:
N/A - latest stable version
git snapshot 
NOT OKAY - SourceURL valid
# git clone git://git.dronelabs.com/ethos
you have girrevision: 9d3aae9 as source0 you should add the command how to co (explicit) this version and not the newest one.
If i tried this I would get a newer one co
OK - Compiler flags ok
OK - Debuginfo complete

Comment 2 Peter Robinson 2009-11-24 17:37:26 UTC
> NOT OKAY - MUST: Named according to the Package Naming Guidelines
> the name of the package should be libethos, because it is a lib
> Group:          System Environment/Libraries
> NOT OKAY - MUST: Spec file name matches the base package %{name} 
> should be libethos

Actually ethos is fine because that's what the upstream name of the project is.

> NOT OKAY - MUST: Fedora approved license and meets the Licensing Guidelines
> License is LGPLv2+, and this is a good license
> NOT OKAY - MUST: License field in spec file matches the actual license
> License:        LGPLv2+

The README file states that it uses the LGPLv2.1 license and doesn't state 'or later versions' hence the reason the + wasn't added. I believe this is correct.

> NOT OKAY - MUST: Sources match the upstream source by MD5
> your command "git clone git://git.dronelabs.com/ethos" give me a newer
> revision, you should add the command to co this version. then the md5sum od DL
> and pacakge should match.

There's now a new official upstream 0.2.2 release. I was using the git snapshot as it was required by emerillon. I will update the package shortly. Also its the second 'git archive' command below the clone that actually gives you the correct version of the tar ball if you use the defined variables in the spec file so you can get the exact same version.

> # git clone git://git.dronelabs.com/ethos
> you have girrevision: 9d3aae9 as source0 you should add the command how to co
> (explicit) this version and not the newest one.
> If i tried this I would get a newer one co

You can. See this is the spec
# Tarfile created using git
# git clone git://git.dronelabs.com/ethos
# git archive --format=tar --prefix=%{name}-%{version}/ %{git_version} | bzip2 > %{name}-%{version}.tar.bz2
%define git_version 9d3aae9
%define tarfile %{name}-%{version}.tar.bz2

Comment 3 Simon 2009-11-25 09:53:02 UTC
(In reply to comment #2)
> Actually ethos is fine because that's what the upstream name of the project is.
ACK. There is no rule to name a lib lib*

> The README file states that it uses the LGPLv2.1 license and doesn't state 'or
> later versions' hence the reason the + wasn't added. 
> I believe this is correct.
The README file? Please look in the COPYING. This is the file to figure out the correct license. 

> # Tarfile created using git
> # git clone git://git.dronelabs.com/ethos
> # git archive --format=tar --prefix=%{name}-%{version}/ %{git_version} | bzip2
> > %{name}-%{version}.tar.bz2
> %define git_version 9d3aae9
> %define tarfile %{name}-%{version}.tar.bz2  
This part is okay, but not what she meaned.
Josephine, if you create this tarball you get another md5sum. But this is normal.

Comment 4 Peter Robinson 2009-11-25 18:07:15 UTC
> > The README file states that it uses the LGPLv2.1 license and doesn't state 'or
> > later versions' hence the reason the + wasn't added. 
> > I believe this is correct.
> The README file? Please look in the COPYING. This is the file to figure out the
> correct license. 

The COPYING file just contain the LGPL2.1 and doesn't say anything about using a later version of the license. Sometimes you get the license text in the COPYING file and a note in the README on how to apply the license. IE LGPLv2.1 only or LGPLv2.1 or any later version at the users choice.  Hence my point.

> > # Tarfile created using git
> > # git clone git://git.dronelabs.com/ethos
> > # git archive --format=tar --prefix=%{name}-%{version}/ %{git_version} | bzip2
> > > %{name}-%{version}.tar.bz2
> > %define git_version 9d3aae9
> > %define tarfile %{name}-%{version}.tar.bz2  
> This part is okay, but not what she meaned.
> Josephine, if you create this tarball you get another md5sum. But this is
> normal.  

Nope. That's incorrect. you should be able to recreate the tar ball with the same md5sum by using the procedure. That's the whole point of the md5sum.... to ensure the source code isn't modified from upstream official code/release.

Comment 5 Peter Robinson 2009-12-02 03:27:50 UTC
Updated the spec to the latest official release.

SPEC as before.
SRPM: http://pbrobinson.fedorapeople.org/ethos-0.2.2-1.fc12.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1842620

Comment 6 Simon 2009-12-03 21:44:00 UTC
(In reply to comment #4)
> The COPYING file just contain the LGPL2.1 and doesn't say anything about using
> a later version of the license. Sometimes you get the license text in the
> COPYING file and a note in the README on how to apply the license. IE LGPLv2.1
> only or LGPLv2.1 or any later version at the users choice.  Hence my point.
I said before:
!!!! PLEASE LOOK IN THE COPYING-FILE !!!!
<snip>
Each version is given a distinguishing version number.  If the Library
specifies a version number of this License which applies to it and
"any later version", you have the option of following the terms and
conditions either of that version or of any later version published by
the Free Software Foundation.  If the Library does not specify a
license version number, you may choose any version ever published by
the Free Software Foundation.
</snip>
<snip>
    This library is free software; you can redistribute it and/or
    modify it under the terms of the GNU Lesser General Public
    License as published by the Free Software Foundation; either
    version 2.1 of the License, or (at your option) any later version.
</snip
Looks like LGPLv2+ or not?

https://fedoraproject.org/wiki/Licensing#Good_Licenses
GNU Lesser General Public License v2 (or 2.1) or later
LGPLv2+

https://fedoraproject.org/wiki/Licensing#GPLCompatibilityMatrix


> Nope. That's incorrect. you should be able to recreate the tar ball with the
> same md5sum by using the procedure. That's the whole point of the md5sum.... > to ensure the source code isn't modified from upstream official 
> code/release.  
Nope? Why nope? Did you ever take a look in your archive? your checkout creates an !UNIQUE! .git directory. This is the reason why we all get another md5sum. You have to remove the .git directory from your source0 before you archive it. THEN and !only THEN! we all will get the same md5sum...

Comment 7 Christoph Wickert 2009-12-04 11:01:47 UTC
(In reply to comment #4)
> The COPYING file just contain the LGPL2.1 and doesn't say anything about using
> a later version of the license. Sometimes you get the license text in the
> COPYING file and a note in the README on how to apply the license. IE LGPLv2.1
> only or LGPLv2.1 or any later version at the users choice.  Hence my point.

COPYING is just the license text and does not distinguish between a particular version of license or "any later versions". The paragraph Simon quoted also does not solve this question, it only says "If the Library specifies a version number ..."

So please take a look at the headers, they do specify a version number. All of them read:

 * This library is free software; you can redistribute it and/or
 * modify it under the terms of the GNU Library General Public
 * License as published by the Free Software Foundation; either
 * version 2 of the License, or (at your option) any later version.

So LGPLv2+ is the correct license tag. You should always rely on the source.

Comment 8 Peter Robinson 2009-12-17 23:03:52 UTC
Sorry, this got lost in amongst 100s of abrt reports.

Fixed the license
SPEC: http://pbrobinson.fedorapeople.org/ethos.spec
SRPM: http://pbrobinson.fedorapeople.org/ethos-0.2.2-2.fc12.src.rpm

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

Comment 9 Josephine Tannhäuser 2009-12-21 18:56:59 UTC
I would say this is ready for repo, now.
Simon, what do you think?

Comment 10 Simon 2009-12-22 10:24:48 UTC
(In reply to comment #9)
> I would say this is ready for repo, now.
> Simon, what do you think?  

I disagree with you 

%{_libdir}/python2.6/site-packages/gtk-2.0/_ethos.so
this is ugly. please use:
%{!?python_sitearch: %global python_sitearch %(%{__python} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib(1))")}
in the head of your spec and then
%{python_sitelib}/gtk-*/_%{name}.so
https://fedoraproject.org/wiki/Packaging:Python

you should use %{name} instead of the name and you should use the "*" instead of version numbers. It's easier for an mass rebuild; like a pygtk update. for example the new path will be gkt-3.0 your specfile is valid for this and it is nor required to edit it.
Please add a / to the directories in your filelists, it's easier to differ files and directories.

Comment 11 Peter Robinson 2009-12-22 15:26:47 UTC
> %{python_sitelib}/gtk-*/_%{name}.so

Actually the above needs to be %{python_sitearch} otherwise is doesn't find the files when built in koji. Rest updated.

New updated:
SPEC: http://pbrobinson.fedorapeople.org/ethos.spec
SRPM: http://pbrobinson.fedorapeople.org/ethos-0.2.2-3.fc12.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1886156

Comment 12 Josephine Tannhäuser 2009-12-23 07:37:34 UTC
as far as i understand it right, you should use wildcards for every versionnumber in your filelist. But I'm not sure, just make sense to me, as far as i understand it correctly.


%files -f %{name}.lang
%defattr(-,root,root,-)
%doc COPYING AUTHORS README NEWS
%{_libdir}/libethos*.so.*
%{_libdir}/%{name}/
%{_libdir}/girepository-*/Ethos-*.typelib
%{_datadir}/%{name}/

%files devel
%defattr(-,root,root,-)
%{_includedir}/%{name}-*/
%{_libdir}/pkgconfig/%{name}*.pc
%{_libdir}/*.so
%{_datadir}/gir-*/Ethos-*.gir

%files docs
%defattr(-,root,root,-)
%{_datadir}/gtk-doc/html/%{name}/

%files python
%defattr(-,root,root,-)
%{python_sitearch}/gtk-*/_%{name}*so
%{python_sitearch}/gtk-*/%{name}/
%{_datadir}/pygtk/*/defs/%{name}*

%files vala
%defattr(-,root,root,-)
%{_datadir}/vala/vapi/%{name}*vapi

Comment 13 Peter Robinson 2009-12-23 14:04:44 UTC
(In reply to comment #12)
> as far as i understand it right, you should use wildcards for every
> versionnumber in your filelist. But I'm not sure, just make sense to me, as far
> as i understand it correctly.

Sorry I don't understand what your asking. Is this a geneeral query, or something in the review?

Comment 14 Josephine Tannhäuser 2009-12-24 10:28:42 UTC
(In reply to comment #10)
> you should use "*" instead of version numbers. 
I tried to do that with your filelist, like:

%files -f %{name}.lang
%defattr(-,root,root,-)
%doc COPYING AUTHORS README NEWS
%{_libdir}/lib%{name}*
%{_libdir}/%{name}/
%{_libdir}/girepository-*/Ethos-*.typelib
%{_datadir}/%{name}/

%files devel
%defattr(-,root,root,-)
%{_includedir}/%{name}-*/
%{_libdir}/pkgconfig/%{name}*.pc
%{_libdir}/*.so
%{_datadir}/gir-*/Ethos-*.gir

%files docs
%defattr(-,root,root,-)
%{_datadir}/gtk-doc/html/%{name}/

%files python
%defattr(-,root,root,-)
%{python_sitearch}/gtk-*/_%{name}*so
%{python_sitearch}/gtk-*/%{name}/
%{_datadir}/pygtk/*/defs/%{name}*

%files vala
%defattr(-,root,root,-)
%{_datadir}/vala/vapi/%{name}*vapi

Comment 15 Christoph Wickert 2009-12-24 11:30:40 UTC
(In reply to comment #12)
> as far as i understand it right, you should use wildcards for every
> versionnumber in your filelist.

Not everyone, but where it makes sense.


(In reply to comment #14)

> %files -f %{name}.lang
...
> %{_libdir}/lib%{name}*

...
> %files devel
...
> %{_libdir}/*.so

This will result in files listed twice because 
%{_libdir}/lib%{name}*
and 
%{_libdir}/*.so
both include
/usr/lib64/libethos-1.0.so
/usr/lib64/libethos-ui-1.0.so

Correct is 
%{_libdir}/libethos*.so.*
for the base package (this is what Peter is using right now).


> %{_datadir}/gtk-doc/html/%{name}/

this should be 
%doc %{_datadir}/gtk-doc/html/%{name}/


The python and the vala package should be in the group
Development/Languages
instaed of
Development/Libraries


What owns/provides /usr/share/gir-1.0/? Make sure the dir is not unowned.

Comment 16 Mamoru TASAKA 2009-12-24 12:31:40 UTC
(In reply to comment #15)
> (In reply to comment #12)
> > %{_datadir}/gtk-doc/html/%{name}/
> 
> this should be 
> %doc %{_datadir}/gtk-doc/html/%{name}/

The directory %{_datadir}/gtk-doc/html/ and all files/directories/etc
under this are automatically marked as %doc (check
$ rpm --eval %__docdir_path and /usr/lib/rpm/macros )

Comment 17 Mamoru TASAKA 2009-12-24 13:52:45 UTC
(In reply to comment #15)
> What owns/provides /usr/share/gir-1.0/? Make sure the dir is not unowned.  

# repoquery --disablerepo=\* --enablerepo=rawhide --whatprovides '/usr/share/gir-1.0' | sort
atk-devel-0:1.29.4-2.fc13.i686
gobject-introspection-devel-0:0.6.7-1.fc13.i686
gtk2-devel-0:2.19.2-1.fc13.i686

Comment 18 Peter Robinson 2009-12-24 14:30:07 UTC
(In reply to comment #17)
> (In reply to comment #15)
> > What owns/provides /usr/share/gir-1.0/? Make sure the dir is not unowned.  

It should be this package that owns it.

> gobject-introspection-devel-0:0.6.7-1.fc13.i686

These two have just recently had their own native G-O-I added (seen in rawhide reports) so they shouldn't own it and at a guess the packages just need the files explicitly added rather than a dir.

> atk-devel-0:1.29.4-2.fc13.i686
> gtk2-devel-0:2.19.2-1.fc13.i686

Comment 19 Peter Robinson 2009-12-28 13:41:49 UTC
Right, re-reading through the above the only remaining thing that needed fixing was the bindings groups. This is now done:

SPEC: as above.
SRPM: http://pbrobinson.fedorapeople.org/ethos-0.2.2-3.fc12.src.rpm

Comment 20 Felix Möller 2010-01-05 00:55:06 UTC
just rebuilt the package without a problem on F12. Thanks.

Comment 21 Josephine Tannhäuser 2010-01-05 07:11:07 UTC
Simon ping?

Comment 22 Simon 2010-01-07 20:42:14 UTC
Josephine, why do you ping me?
Peter has enough informations and hints in this review and he know where he can find the guidelines for further informations...
He is provenpackager and should have the skill to finish this, easily; even I made a little mistake in #c10.

Comment 23 Peter Robinson 2010-01-07 20:55:01 UTC
I believe all the issues are fixed and I'm waiting of final sign off.

Comment 24 Christoph Wickert 2010-01-08 04:15:55 UTC
Sorry, but there are definitely blockers left:
- the unversioned libs in /usr/lib64/ethos/plugin-loaders/ belong into the devel package, see
https://fedoraproject.org/wiki/Packaging/Guidelines#Devel_Packages
- using %{_libdir}/python2.6/ in the files section is a no-go. Please see 
https://fedoraproject.org/wiki/Packaging/Python#System_Architecture
for how to do this properly.
- vala package should be noarch as it contains no arch dependent files


(In reply to comment #18)
> > (In reply to comment #15)
> > > What owns/provides /usr/share/gir-1.0/? Make sure the dir is not unowned.  
> 
> It should be this package that owns it.
> 
> > gobject-introspection-devel-0:0.6.7-1.fc13.i686

Then you need the devel package require gobject-introspection-devel for directory ownership. Currently the requirements autogenerated from the pkconfig files only include 
 pkgconfig(gobject-2.0)  
 pkgconfig(gtk+-2.0) 
This means that only glib2-devel and gtk2-devel are pulled in. And even if we had pkgconfig(introspection-1.0.pc) I'd still prefer requiring gobject-introspection-devel because all this stuff only works with newer versions of rpm.

There are more potentially unowned dirs:
- doc package puts files into %{_datadir}/gtk-doc/html, so it needs to require gtk-doc
- python package puts files into %{python_sitearch}/gtk-2.0 (sic!) and needs to require pygtk2
- python package puts files into %{_datadir}/pygtk/2.0/defs and needs to require gnome-python2
- vala package puts files into %{_datadir}/vala/vapi and needs to require vala


Other questions:
- Why is there a doc package anway? Usually gtk-doc is part of the devel package. People who don't want the docs can still install it with --excludedocs.
- Should %{_libdir}/ethos/plugin-loaders/libpythonloader.so be part of the python package?

Comment 25 Christoph Wickert 2010-01-08 04:35:24 UTC
The following are for Simon and Joesphine. They are of educational nature and not really related to the review:

(In reply to comment #3)
> The README file? Please look in the COPYING. This is the file to figure out the
> correct license. 

Never (only) rely on a COPYING file, look into header of the sources instead. How would you destinguish between LGPLv2 or LGPLv2+ here by only looking at COPYING?

(In reply to comment #10)
> %{python_sitelib}/gtk-*/_%{name}.so
> https://fedoraproject.org/wiki/Packaging:Python
> 
> you should use %{name} instead of the name and you should use the "*" instead
> of version numbers. It's easier for an mass rebuild; like a pygtk update. for
> example the new path will be gkt-3.0 your specfile is valid for this and it is
> nor required to edit it.

I fully agree on %{python_sitelib}, but the rest is questionable:
Using %{name} hardly adds any benefit. It only makes sense if Peter maintains lots of packages with similar specs, so he can copy & paste stuff there.
Using a wildcard for the gtk version also doesn't add benefit. Once gtk3 lands this package definitely needs more than a scripted rebuild, it needs a new version. This version is supposed to work with gtk2, and different gtk major versions can be installed in parallel, so using gtk-2.0 is more sane and more obvious when you look at the spec.

(In reply to comment #16)
> The directory %{_datadir}/gtk-doc/html/ and all files/directories/etc
> under this are automatically marked as %doc (check
> $ rpm --eval %__docdir_path and /usr/lib/rpm/macros )  

Of course Mamoru is right, but I still prefer marking this explicitly. I wanted to point out that it's important to check that files are correctly marked as %doc or %config.

Comment 26 Peter Robinson 2010-01-09 16:31:13 UTC
Thanks for these updates Christoph, I was quite clearly not with it when doing this package to have missed those. October was somewhat of a blur :-(

> Sorry, but there are definitely blockers left:
> - the unversioned libs in /usr/lib64/ethos/plugin-loaders/ belong into the
> devel package

Fixed.

> - using %{_libdir}/python2.6/ in the files section is a no-go. Please see 
> https://fedoraproject.org/wiki/Packaging/Python#System_Architecture
> for how to do this properly.
> - vala package should be noarch as it contains no arch dependent files

Fixed.

> > It should be this package that owns it.
> > 
> > > gobject-introspection-devel-0:0.6.7-1.fc13.i686

Fixed.

> Then you need the devel package require gobject-introspection-devel for
> directory ownership. Currently the requirements autogenerated from the pkconfig
> files only include 
>  pkgconfig(gobject-2.0)  
>  pkgconfig(gtk+-2.0) 
> This means that only glib2-devel and gtk2-devel are pulled in. And even if we
> had pkgconfig(introspection-1.0.pc) I'd still prefer requiring
> gobject-introspection-devel because all this stuff only works with newer
> versions of rpm.

For some reason I thought this was done automatically now. Don't see RPM version really as an issue as I don't plan to push it to anythin earlier than F-12. Fixed anyway.

> There are more potentially unowned dirs:
> - doc package puts files into %{_datadir}/gtk-doc/html, so it needs to require
> gtk-doc
> - python package puts files into %{python_sitearch}/gtk-2.0 (sic!) and needs to
> require pygtk2
> - python package puts files into %{_datadir}/pygtk/2.0/defs and needs to
> require gnome-python2
> - vala package puts files into %{_datadir}/vala/vapi and needs to require vala

Fixed.
 
> Other questions:
> - Why is there a doc package anway? Usually gtk-doc is part of the devel
> package. People who don't want the docs can still install it with
> --excludedocs.

Because there was a push a while ago to move all docs into noarch packages to reduce the size of the overall mirror.

> - Should %{_libdir}/ethos/plugin-loaders/libpythonloader.so be part of the
> python package?  

Done.

SRPM: http://pbrobinson.fedorapeople.org/ethos-0.2.2-4.fc12.src.rpm

Comment 27 Christoph Wickert 2010-01-09 17:36:19 UTC
(In reply to comment #26) 
> For some reason I thought this was done automatically now. Don't see RPM
> version really as an issue as I don't plan to push it to anythin earlier than
> F-12. Fixed anyway.

It *is* done automatically in rpm 4.7, but even with the automatical gnerated info we only get glib2-devel and gtk2-devel, but not gobject-introspection-devel.

> Because there was a push a while ago to move all docs into noarch packages to
> reduce the size of the overall mirror.

I think this only applies to end user docs and not to development docs. I really wish we had a guideline for that, but until then I suggest to follow the majority of our packages.

> > - Should %{_libdir}/ethos/plugin-loaders/libpythonloader.so be part of the
> > python package?  
> 
> Done.

That was a question, I didn't test.

Comment 28 Peter Robinson 2010-01-09 17:42:10 UTC
> > Because there was a push a while ago to move all docs into noarch packages to
> > reduce the size of the overall mirror.
> 
> I think this only applies to end user docs and not to development docs. I
> really wish we had a guideline for that, but until then I suggest to follow the
> majority of our packages.

From memory it applied to all because I had people bothering me about splitting them out for a whole heap of libs that I maintain which were just dev docs like the above. I agree on the guidelines though.

> > > - Should %{_libdir}/ethos/plugin-loaders/libpythonloader.so be part of the
> > > python package?  
> > 
> > Done.
> 
> That was a question, I didn't test.    

And a valid one :-D

Comment 29 Peter Robinson 2010-01-12 22:58:18 UTC
Any update on finishing this review?

Comment 30 Simon 2010-01-14 21:47:53 UTC
okay, you did all the issues, mentoined by christoph.
should be okay now. Approved

Comment 31 Peter Robinson 2010-01-14 21:54:28 UTC
Thanks for the review!

New Package CVS Request
=======================
Package Name: ethos
Short Description: Plugin framework for GLib
Owners: pbrobinson
Branches: F-12
InitialCC:

Comment 32 Jason Tibbitts 2010-01-14 22:13:03 UTC
CVS done (by process-cvs-requests.py).

Comment 33 Peter Robinson 2010-01-14 23:21:15 UTC
Imported and built in rawhide. Thanks all for the review input.


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