Bug 182463

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.

Change to FE_NEEDSPONSOR, becouse submitter wrote, that he needs sponsorship.

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.

> 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.

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.

> > - 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.

I dont' understand. There are definitely cairomm tarball releases.

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

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)

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

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

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/

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.

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.

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.

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/

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)

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. 

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

0.6.0-1 approved.

Package Change Request
======================
Package Name: cairomm
New Branches: EL-6

cvs done.