Bug 182463 - Review Request: cairomm (C++ bindings for cairo)
Review Request: cairomm (C++ bindings for cairo)
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-02-22 13:49 EST by Rick L Vinyard Jr
Modified: 2010-07-14 12:42 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-05-01 22:15:00 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Rick L Vinyard Jr 2006-02-22 13:49:06 EST
This is my first package and I need a sponsor.

Spec Name or Url: http://miskatonic.cs.nmsu.edu/pub/cairomm.spec

SRPM Name or Url: http://miskatonic.cs.nmsu.edu/pub/fedora/4/srpms/cairomm-0.5.0-3.20060222cvs.src.rpm

Description: C++ bindings for cairo; the current spec and srpm are a pre-release from CVS and should comply with the pre-release versioning specs.
Comment 1 Jochen Schmitt 2006-02-22 15:06:12 EST

Bad:
- Locak build will not possible on FC-4
- Please give the full URL in the Source tag.
- The BuildRoot must be cleaned at the beginning of %install
- Please remove Copyright text from spec file.
- Please remove Epach tag.
- Requires for /sbin/ldconfig are not requires.
- I not see any make step in the %build stanza
- Please remove static libraries
- Mock build (FC5/i686) failed

checking for pkg-config... /usr/bin/pkg-config
checking pkg-config is at least version 0.9.0... yes
checking for CAIROMM... configure: error: Package requirements (cairo >= 1.0) we
re not met:

Package x11 was not found in the pkg-config search path.
Perhaps you should add the directory containing `x11.pc'
to the PKG_CONFIG_PATH environment variable
Package 'x11', required by 'Xrender', not found

Consider adjusting the PKG_CONFIG_PATH environment variable if you
installed software in a non-standard prefix.

Alternatively, you may set the environment variables CAIROMM_CFLAGS
and CAIROMM_LIBS to avoid the need to call pkg-config.
See the pkg-config man page for more details.

error: Bad exit status from /var/tmp/rpm-tmp.67558 (%build)

Becouse cairo is not part of FC-4 anyone else may overtake the review part.
Comment 2 Jochen Schmitt 2006-02-22 15:10:39 EST
Change to FE_NEEDSPONSOR, becouse submitter wrote, that he needs sponsorship.
Comment 3 Rick L Vinyard Jr 2006-02-22 16:20:25 EST
The .spec file is updated in the same place. The new srpm can be found at:
http://miskatonic.cs.nmsu.edu/pub/fedora/4/srpms/cairomm-0.5.0-4.20060222cvs.src.rpm

- Locak build will not possible on FC-4
Possible, but requires backports of Gtk+ et. al. (available at the same site as
the srpm if anyone really wants them for FC4 before FC5 comes out) But, the
intent isn't to submit for FC4, just FC5.

- Please give the full URL in the Source tag.
There are no snapshot releases yet. Only cvs via cairographics.org directly.

- The BuildRoot must be cleaned at the beginning of %install
The first line is:
  rm -rf ${RPM_BUILD_ROOT}
Is there more to do? In case it was an oversight, I put an extra line in to make
it fit the convention that other rpm's seem to use.

- Please remove Copyright text from spec file.
Removed

- Please remove Epach tag.
Removed

- Requires for /sbin/ldconfig are not requires.
Removed

- I not see any make step in the %build stanza
Added

- Please remove static libraries
Removed

- Mock build (FC5/i686) failed
I'm not sure how to handle this one. This looks like mock isn't finding any of
the pkgconfig .pc files.
Comment 4 Christian Iseli 2006-02-22 17:19:17 EST
> Change to FE_NEEDSPONSOR, becouse submitter wrote, that he needs sponsorship.

The idea is that FE-NEEDSPONSOR should be added, not used to replace FE-NEW.
Comment 5 Rick L Vinyard Jr 2006-02-23 01:10:53 EST
Sorry, one more fix; I missed one place when %{epoch} was used.

The .spec file above is still good, but the srpm is now:
http://miskatonic.cs.nmsu.edu/pub/fedora/4/srpms/cairomm-0.5.0-5.20060222cvs.src.rpm

Also, I did try building on a FC5-T3 i686 machine and no problems, so I think
the aforementioned problem was in mock.
Comment 6 Ralf Corsepius 2006-02-23 01:25:13 EST
> > - Please give the full URL in the Source tag.
> There are no snapshot releases yet. Only cvs via cairographics.org directly.
IMO, then we should not ship it.
Comment 7 Murray Cumming 2006-02-24 05:38:44 EST
I dont' understand. There are definitely cairomm tarball releases.
Comment 8 Rick L Vinyard Jr 2006-02-25 00:35:52 EST
My mistake on the cairomm tarballs. I didn't realize they were there. The URL is
now corrected, and I think everything is ready to go.

Spec Name or Url: http://miskatonic.cs.nmsu.edu/pub/cairomm.spec

SRPM Name or Url:
http://miskatonic.cs.nmsu.edu/pub/fedora/4/srpms/cairomm-0.5.0-6.src.rpm
Comment 9 Ralf Corsepius 2006-02-25 00:58:53 EST
NEEDSWORK

Not a review, just a some remarks on the spec file:

1) It contains this:
URL:		  http://www.cairographics.org/releases//cairomm-0.5.0.tar.gz
Source:           cairomm-0.5.0.tar.gz

"Source:" should contain an URL pointing to the tarball, 
"URL" should point to the projects homepage.

I.e. you probably want something similar to this:
URL:		  http://www.cairographics.org
Source:           http://www.cairographics.org/releases/cairomm-0.5.0.tar.gz


2) The spec contains this:
%files devel
...
%{_includedir}/cairomm-1.0/*

The last line should be
%{_includedir}/cairomm-1.0
(without the asterisk - The directory and all files underneath should be
included into the *-devel package)
Comment 10 Rick L Vinyard Jr 2006-02-25 02:12:31 EST
I think we're there...

Spec Name or Url: http://miskatonic.cs.nmsu.edu/pub/cairomm.spec

SRPM Name or Url:
http://miskatonic.cs.nmsu.edu/pub/fedora/4/srpms/cairomm-0.5.0-7.src.rpm
Comment 11 Rick L Vinyard Jr 2006-03-05 16:32:10 EST
A few minor tweaks, including cleanup of the Source tag and the %setup section.

Spec Name or Url: http://miskatonic.cs.nmsu.edu/pub/cairomm.spec

SRPM Name or Url:
http://miskatonic.cs.nmsu.edu/pub/fedora/4/srpms/cairomm-0.5.0-9.src.rpm
Comment 12 Callum Lerwick 2006-03-06 01:13:43 EST
I can't sponsor, but here's my first review ever:

MUST items:

- rpmlint: ?
E: cairomm shlib-with-non-pic-code /usr/lib/libcairomm-1.0.so.0.0.5

I don't know how big a problem this is or how to fix it. (FC5t3 i386)

- Package name: Ok
- Spec name: Ok
- Meets packaging guidelines: Ok
- License: Ok
- Spec in American English: Ok
- Spec legible: Ok
- Sources match upstream: Ok
- Builds: Yes (FC5t3 i386)
- BuildRequires: Ok
- Locales: Ok
- ldconfig: Ok
- Relocation: Ok
- Directory ownership: Ok
- %files: Ok
- %clean: Ok
- Macros: Ok
- Code vs. Content: Ok
- Documentation: Ok
- devel package: Ok
- .desktop file: n/a

SHOULD:

- Includes license text: Ok
- Spec translations: Ok?
- Mock build: n/a, I don't have mock set up yet
- Builds on all archs: n/a, I only have i386
- Package functional: Ok, included png_file example compiles and runs
- Scriptlets: Ok
- Subpackages: Ok

NEEDSWORK:

There are duplicate Group: lines in the devel package.

I notice the devel package installs the API reference into
/usr/share/doc/libcairomm-1.0, upstream seems confused as to what the
name/verison really is. This is probably something to bug upstream about. For
now, I'd see if you can use configure flags to convince it to put reference/
into /usr/share/doc/cairomm-%{version}/ instead, failing that, move it over in
%install

Style soapbox (These aren't blockers):

IMHO, macros are for versions and paths. Stuff that can and will change. I don't
like the over use of %{name} everywhere. Especially in %description and Summary.
IMHO, %{name} should be used in BuildRoot and nowhere else. Renaming a package
shouldn't happen very often, so there's little need for using an ugly macro.

Also, I would specify %files a bit more exactly. I find its best to be aware of
when upstream adds/renames/removes files, excessive globbing can end up with
mysterious new files turning up in the wrong package. Better to error out due to
unpackaged files. I would use:

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

%files devel
%defattr(-,root,root,-)
%{_libdir}/libcairomm-1.0.so
%{_libdir}/pkgconfig/cairomm-1.0.pc
%{_includedir}/cairomm-1.0/
%doc %{_datadir}/doc/libcairomm-1.0/
Comment 13 Callum Lerwick 2006-03-06 01:24:31 EST
I forgot to mention, I was talking to rvinyard on IRC and he said he gets this
from rpmlint:

W: cairomm unstripped-binary-or-object /usr/lib/libcairomm-1.0.so.0.0.5

I'm building on FC5t3 and he's on an up to date devel. Someone who knows gcc
voodoo please tell us whats going on, and if it can be ignored.
Comment 14 Rick L Vinyard Jr 2006-03-06 02:04:54 EST
Spec Name or Url: http://miskatonic.cs.nmsu.edu/pub/cairomm.spec

SRPM Name or Url:
http://miskatonic.cs.nmsu.edu/pub/fedora/4/srpms/cairomm-0.5.0-10.src.rpm

FIXED: The duplicate devel line and the docs. For now, rather than messing with
them, I just disabled doc building until the naming is fixed upstream.

As for the macros, I've just used those to genericize the spec file a bit, so
the same template can be used for several projects. It's actually not a big deal
to change it because the spec files are actually getting created by autoconf, so
it's a matter of substituting %{name} for @PACKAGE_NAME@ and who (autoconf or
rpmbuild) does the substitution.

Thanks again Seg.
Comment 15 Rick L Vinyard Jr 2006-03-06 02:12:20 EST
I forgot to mention that I'm almost certain that the only reason I got the
warning was because I had debug set to nil and instead of stripping the debug
symbols and building the debug package, it left the symbols in.

What concerns me is the PIC error that Callum was getting.
Comment 16 Michael Schwendt 2006-03-24 18:00:25 EST
Package is in quite good shape. Almost there. These minor issues
are left:

* missing "BuildRequires: pkgconfig"

* cairomm-devel should really "Requires: pkgconfig", because (1) it
contains a pkg-config file and (2) the library headers are stored in
a path that is unlikely to be found if pkg-config is not used

* prefer "make DESTDIR=%{buildroot} install" over %makeinstall, since
while the former is de facto standard, the latter is just a hack

* AUTHORS %doc file is about libxmlplusplus and points to its home page

* README %doc file has libxml++ at the top, very confusing, both files
should really be corrected

* Also note that the cairomm-devel package contains more than "headers
and static library". ;)  The very important *.so symlink is included, too.

> E: cairomm shlib-with-non-pic-code /usr/lib/libcairomm-1.0.so.0.0.5

Cannot reproduce this on i686, but if it occurs on x86_64, it must
be patched to compile everything with -fPIC. Also skim over the build
log and check whether you see the configure checks and whether -fPIC
is used during compilation:

| checking for gcc option to produce PIC... -fPIC
| checking if gcc PIC flag -fPIC works... yes

| checking for g++ option to produce PIC... -fPIC
| checking if g++ PIC flag -fPIC works... yes


> W: cairomm unstripped-binary-or-object /usr/lib/libcairomm-1.0.so.0.0.5

Cannot reproduce. When you get this, most likely you either don't
have the redhat-rpm-config package installed or you've disabled
creation of debuginfo packages via ~/.rpmmacros: %debug_package %{nil}


As a hint at the bottom:

| %{_includedir}/cairomm-1.0/*
vs.
| %{_includedir}/cairomm-1.0

Even better to the eyes, since you see it's a directory (!):

%{_includedir}/cairomm-1.0/
Comment 17 Callum Lerwick 2006-03-26 13:25:56 EST
I get:

checking for gcc option to produce PIC... -fPIC
checking if gcc PIC flag -fPIC works... no

Why would that be... (My laptop is now running FC5 final)
Comment 18 Callum Lerwick 2006-03-26 13:39:51 EST
Figured it out, my opt flags were throwing things off:

configure:6408: gcc -c -Os -g -pipe -march=pentium3 -mcpu=pentium3 -ffast-math
-Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -fasynchronous-unwind-tables  -fPIC -DPIC
conftest.c >&5
`-mcpu=' is deprecated. Use `-mtune=' or '-march=' instead.
configure:6412: $? = 0
configure:6423: result: no

Hmmm, seems gcc's obnoxious warning is throwing off the test. Changing to
-mtune= fixes it. 
Comment 19 Rick L Vinyard Jr 2006-04-09 14:06:01 EDT
Spec Name or Url: http://miskatonic.cs.nmsu.edu/pub/cairomm.spec

SRPM Name or Url: http://miskatonic.cs.nmsu.edu/pub/fedora/5/srpms/cairomm-0.6.
0-1.src.rpm

Summary of changes:
- New upstream release
- Added docs back in by moving them in install and including in devel files

Other changes from feedback:
* missing "BuildRequires: pkgconfig"
 - ADDED

* cairomm-devel should really "Requires: pkgconfig", because (1) it
contains a pkg-config file and (2) the library headers are stored in
a path that is unlikely to be found if pkg-config is not used
 - ADDED to cairomm BuildRequires and cairomm-devel Requires

* prefer "make DESTDIR=%{buildroot} install" over %makeinstall, since
while the former is de facto standard, the latter is just a hack
 - CHANGED

* AUTHORS %doc file is about libxmlplusplus and points to its home page
 - UPSTREAM RELEASE fixes this

* README %doc file has libxml++ at the top, very confusing, both files
should really be corrected
 - UPSTREAM RELEASE fixes this

* Also note that the cairomm-devel package contains more than "headers
and static library". ;)  The very important *.so symlink is included, too.
 - CHANGED

* As a hint at the bottom:
 - CHANGED to the latter suggestion... directory style
Comment 20 Michael Schwendt 2006-04-18 20:53:04 EDT
0.6.0-1 approved.
Comment 21 Rick L Vinyard Jr 2010-07-14 11:14:20 EDT
Package Change Request
======================
Package Name: cairomm
New Branches: EL-6
Comment 22 Kevin Fenzi 2010-07-14 12:42:00 EDT
cvs done.

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