This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 460260 - Review Request: pangomm - C++ wrapper for pango
Review Request: pangomm - C++ wrapper for pango
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Robinson
Fedora Extras Quality Assurance
:
Depends On:
Blocks: F10Blocker/F10FinalBlocker
  Show dependency treegraph
 
Reported: 2008-08-27 03:15 EDT by Denis Leroy
Modified: 2008-08-29 06:30 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-29 06:30:15 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
pbrobinson: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Denis Leroy 2008-08-27 03:15:33 EDT
Spec URL: http://www.poolshark.org/src/pangomm.spec
SRPM URL: http://www.poolshark.org/src/pangomm-2.13.7-1.fc10.src.rpm

Description: Pangomm is a C++ wrapper library for pango: a system for layout and rendering of internationalized text. Pangomm is part of the gtkmm project, the C++ interface for GTK+ and GNOME.


Note to reviewer:

Pangomm used to be part of the gtkmm package itself but now lives in its own tarball (and becomes a build dependency for gtkmm). There are still issues in the way the documentation is split between gtkmm and pangomm. The spec file sets up the documentation similarly to what is done with gtkmm, but the links are known to be broken. This should be resolved by upstream before pangomm hits 2.14 stable.
Comment 1 Denis Leroy 2008-08-27 03:21:54 EDT
This is a F-10 blocker, since gtkmm needs to follow the gtk2 2.14 track.
Comment 2 Peter Robinson 2008-08-27 10:13:42 EDT
Just a few minor issues. Ownership of dirs created with the %dir macro and the
Requires for the devel package. 

+ rpmlint output

rpmlint pangomm-2.13.7-1.fc10.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

+ package name satisfies the packaging naming guidelines
+ specfile name matches the package base name
+ package should satisfy packaging guidelines
+ license meets guidelines and is acceptable to Fedora
+ license matches the actual package license

+ %doc includes license file
+ spec file written in American English
+ spec file is legible
+ upstream sources match sources in the srpm
  148578fbfa7286de2141f3291d64d1e3 pangomm-2.13.7.tar.bz2
+ package successfully builds on at least one architecture
  tested on x86_64 using mock
+ BuildRequires list all build dependencies
+ %find_lang instead of %{_datadir}/locale/*
+ binary RPM with shared library files must call ldconfig in %post and %postun
+ does not use Prefix: /usr
- package owns all directories it creates
  doesn't own the following dirs that it creates:
  %{_datadir}/gtk-doc/html/pangomm-1.4
  %{_includedir}/pangomm-1.4
  %{_includedir}/pangomm-1.4/pangomm
n/a no duplicate files in %files
+ %defattr line
+ %clean contains rm -rf $RPM_BUILD_ROOT
+ consistent use of macros
+ package must contain code or permissible content
n/a large documentation files should go in -doc subpackage
+ files marked %doc should not affect package
+ header files should be in -devel
n/a static libraries should be in -static
- packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
+ libfoo.so must go in -devel
+ devel must require the fully versioned base
+ packages should not contain libtool .la files
n/a packages containing GUI apps must include %{name}.desktop file
+ packages must not own files or directories owned by other packages
+ %install must start with rm -rf %{buildroot} etc.
+ filenames must be valid UTF-8

Optional:

n/a if there is no license file, packager should query upstream
n/a translations of description and summary for non-English languages, if
available
+ reviewer should build the package in mock
n/a the package should build into binary RPMs on all supported architectures
+ review should test the package functions as described (basic testing using
kvm)
+ scriptlets should be sane
+ pkgconfig files should go in -devel
+ shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or
/usr/sbin

I think also the devel package needs to have a "Requires: gtk-doc" as well as
the pkgconfig requires as it uses gtk-doc
Comment 3 Mamoru TASAKA 2008-08-27 10:48:41 EDT
Well,

(In reply to comment #2)
> - package owns all directories it creates
>   doesn't own the following dirs that it creates:
>   %{_datadir}/gtk-doc/html/pangomm-1.4
>   %{_includedir}/pangomm-1.4
>   %{_includedir}/pangomm-1.4/pangomm

  - -devel subpackage actually owns these directories.
    When written as
---------------------------------------------------------------------------
%files
%{_includedir}/pangomm-%{apiver}
---------------------------------------------------------------------------
    This contains the directory %{_includedir}/pangomm-%{apiver} and all directories/files/etc
    under this directory.

Some notes:
  - "BuildRequires: pango-devel" is listed twice.
  - The %description of -devel subpackage is not right.
---------------------------------------------------------------------------
This package contains the static libraries and header files needed for
                          ^^^^^^
---------------------------------------------------------------------------
  - Would you consider to use
---------------------------------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
---------------------------------------------------------------------------
    to keep timestamps on as many files as possible? This method usually works for
    recent autotool based Makefiles.
  - Would you explain why you want to remove files under %_libdir/pangomm-%apiver ?
  - %{_datadir}/gtk-doc/html/ is already marked as docDirs (see Changelog in "rpm" package:
    from 2007-06-29)
Comment 4 Denis Leroy 2008-08-27 13:48:09 EDT
> - package owns all directories it creates

Hmm, looks good to me over here. I think Mamoru-san addressed this in his comment.

# rpm -qf /usr/share/gtk-doc/html/pangomm-1.4
pangomm-devel-2.13.7-1.fc10.i386
# rpm -qf /usr/include/pangomm-1.4
pangomm-devel-2.13.7-1.fc10.i386

> - packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'

Fixed.

> I think also the devel package needs to have a "Requires: gtk-doc"
> as well as the pkgconfig requires as it uses gtk-doc

Fixed.

> "BuildRequires: pango-devel" is listed twice.

Oops. Fixed.

> The %description of -devel subpackage is not right.

Yes, odd, I can't remember where that's coming from. Fixed.

> Would you consider to use make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"

Sure. Fixed.

> %{_datadir}/gtk-doc/html/ is already marked as docDirs

Fixed.


> Would you explain why you want to remove files under
> %_libdir/pangomm-%apiver ?

In all gtkmm project tarballs, %{libdir}/%{name} always contains files
that are related to the code generation scripts use by the C++ wrapper
(the actual C++ interface code is generated dynamically by m4 scripts
when the tarball is created, i.e before build time). So these files
are not particularly useful even to a gtkmm-based project developer. I
removed them mainly to reduce polution of the libdir directory. I've
asked upstream before about this, the answer iirc was "in case someone
is interested"...



Spec URL: http://www.poolshark.org/src/pangomm.spec
SRPM URL: http://www.poolshark.org/src/pangomm-2.13.7-2.fc10.src.rpm
Comment 5 Peter Robinson 2008-08-27 14:08:31 EDT
Spec file seems to be broken and hence I'm seeing buld failure in mock.

pm-ad%files
%defattr(-, root, root, -)
%doc AUTHORS ChangeLog COPYING NEWS README
%{_libdir}/*.so.*

From mock build.log

Processing files: pangomm-debuginfo-2.13.7-2.fc10
Checking for unpackaged file(s): /usr/lib/rpm/check-files /builddir/build/BUILDROOT/pangomm-2.13.7-2.fc10.x86_64
RPM build errors:
error: Installed (but unpackaged) file(s) found:
   /usr/lib64/libpangomm-1.4.so.1
   /usr/lib64/libpangomm-1.4.so.1.0.30
    Installed (but unpackaged) file(s) found:
   /usr/lib64/libpangomm-1.4.so.1
   /usr/lib64/libpangomm-1.4.so.1.0.30
Child returncode was: 1
EXCEPTION: Command failed. See logs for output.
 # ['bash', '--login', '-c', 'rpmbuild -bb --target x86_64 --nodeps builddir/build/SPECS/pangomm.spec']
Traceback (most recent call last):
  File "/usr/lib/python2.5/site-packages/mock/trace_decorator.py", line 70, in trace
    result = func(*args, **kw)
  File "/usr/lib/python2.5/site-packages/mock/util.py", line 316, in do
    raise mock.exception.Error, ("Command failed. See logs for output.\n # %s" % (command,), child.returncode)
Error: Command failed. See logs for output.
 # ['bash', '--login', '-c', 'rpmbuild -bb --target x86_64 --nodeps builddir/build/SPECS/pangomm.spec']
LEAVE do --> EXCEPTION RAISED
Comment 6 Denis Leroy 2008-08-27 14:13:43 EDT
Oops, bad case of emacs-itis. Try again, I've fixed the uploads.
Comment 7 Mamoru TASAKA 2008-08-27 14:23:45 EDT
Now I leave this review request to how Peter-san judges.
Comment 8 Peter Robinson 2008-08-27 14:30:13 EDT
(In reply to comment #7)
> Now I leave this review request to how Peter-san judges.

Mamoru are you happy with the explanation for your "Would you explain why you want to remove files under %_libdir/pangomm-%apiver ?" question?

Peter
Comment 9 Mamoru TASAKA 2008-08-27 14:42:05 EDT
(In reply to comment #8)
> (In reply to comment #7)
> Mamoru are you happy with the explanation for your "Would you explain why you
> want to remove files under %_libdir/pangomm-%apiver ?" question?
> 
> Peter

Yes.
Comment 10 Peter Robinson 2008-08-27 14:46:35 EDT
Looks good to me now. Approved.

+ rpmlint output

rpmlint pangomm-2.13.7-2.fc10.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

+ package name satisfies the packaging naming guidelines
+ specfile name matches the package base name
+ package should satisfy packaging guidelines
+ license meets guidelines and is acceptable to Fedora
+ license matches the actual package license

+ %doc includes license file
+ spec file written in American English
+ spec file is legible
+ upstream sources match sources in the srpm
  148578fbfa7286de2141f3291d64d1e3 pangomm-2.13.7.tar.bz2
+ package successfully builds on at least one architecture
  tested on x86_64 using mock
+ BuildRequires list all build dependencies
+ %find_lang instead of %{_datadir}/locale/*
+ binary RPM with shared library files must call ldconfig in %post and %postun
+ does not use Prefix: /usr
+ package owns all directories it creates
n/a no duplicate files in %files
+ %defattr line
+ %clean contains rm -rf $RPM_BUILD_ROOT
+ consistent use of macros
+ package must contain code or permissible content
n/a large documentation files should go in -doc subpackage
+ files marked %doc should not affect package
+ header files should be in -devel
n/a static libraries should be in -static
+ packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
+ libfoo.so must go in -devel
+ devel must require the fully versioned base
+ packages should not contain libtool .la files
n/a packages containing GUI apps must include %{name}.desktop file
+ packages must not own files or directories owned by other packages
+ %install must start with rm -rf %{buildroot} etc.
+ filenames must be valid UTF-8

Optional:

n/a if there is no license file, packager should query upstream
n/a translations of description and summary for non-English languages, if
available
+ reviewer should build the package in mock
n/a the package should build into binary RPMs on all supported architectures
+ review should test the package functions as described (basic testing using kvm)
+ scriptlets should be sane
+ pkgconfig files should go in -devel
+ shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or
/usr/sbin

+ I think also the devel package needs to have a "Requires: gtk-doc" as well as the pkgconfig requires as it uses gtk-doc
Comment 11 Denis Leroy 2008-08-27 18:21:17 EDT
Peter, Mamoru-san, many thanks for the review.


New Package CVS Request
=======================
Package Name: pangomm
Short Description: C++ wrapper for pango
Owners: denis
Branches:
InitialCC:
Comment 12 Kevin Fenzi 2008-08-29 00:49:04 EDT
cvs done.

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