Bug 484933 - Review Request: libwps - Library for reading and converting Microsoft Works word processor documents
Review Request: libwps - Library for reading and converting Microsoft Works w...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-10 13:48 EST by Simon
Modified: 2009-04-13 15:39 EDT (History)
4 users (show)

See Also:
Fixed In Version: 0.1.2-5.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-18 12:31:59 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
bugs.michael: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Simon 2009-02-10 13:48:10 EST
Spec URL:
http://cassmodiah.fedorapeople.org/libwps-0.1.2/libwps.spec

SRPM URL:
http://cassmodiah.fedorapeople.org/libwps-0.1.2/libwps-0.1.2-1.fc10.src.rpm

Description:
Library that handles Microsoft Works documents.
Comment 1 Michael Schwendt 2009-02-12 07:45:29 EST
Blockers:

> mv %{buildroot}/%{_includedir}/%{name}-0.1/%{name}/* %{buildroot}/%{_includedir}/%{name}/
> rm -rf %{buildroot}/%{_includedir}/%{name}-0.1/

Not only should something like this be explained with a comment in the spec file. Why is the default install location changed? Did you notice that this breaks the pkgconfig files?


> make install -p DESTDIR=%{buildroot}

This is not what you want. This is make --print-data-base ...!
Most likely you wanted: make DESTDIR=%{buildroot} INSTALL="install -p"



Have you looked into adding a %check section for "make check"?


$ rpmls -p /home/qa/tmp/rpm/RPMS/libwps-doc-0.1.2-1.fc10.i386.rpm 
drwxr-xr-x  /usr/share/doc/libwps-0.1.2
-rw-r--r--  /usr/share/doc/libwps-0.1.2/CHANGES
-rw-r--r--  /usr/share/doc/libwps-0.1.2/COPYING
-rw-r--r--  /usr/share/doc/libwps-0.1.2/CREDITS
-rw-r--r--  /usr/share/doc/libwps-0.1.2/HACKING
-rw-r--r--  /usr/share/doc/libwps-0.1.2/README

These are only files, and they are also found in the main package.
I see you want to install doxygen docs, but here it didn't work out.
The %files section for the -doc subpackage is wrong, too, because

  %{_docdir}/%{name}-%{version}/

is the default location for all files in %doc lines.
Comment 2 Orcan Ogetbil 2009-02-13 11:46:25 EST
Apart from what Michael said:

* The group for the -doc subpackage should probably be Documentation instead of Applications/Publishing.

* The source tarball does not have the proper timestamp. You can use, for example
   wget -N http://URLtoSOURCEfile
to get it with the right timestamp.
Comment 3 Michael Schwendt 2009-02-14 14:02:32 EST
Preserving source tarball timestamps is not mandatory. And it is of only limited use (i.e. one can recognise the age of tarballs where timestamps are accurate, without listing its contents). Upon loading source archives into the lookaside cache, timestamps are lost anyway.
Comment 4 Simon 2009-02-14 16:03:03 EST
Okay, I will drop the doc package and corerct my install argument and use the original pkgconfig location,

but

1) why %check? 


2) Timestamps
[cassmodiah@schafwiese Desktop]$ LC_ALL=C wget -N http://downloads.sourceforge.net/libwps/libwps-0.1.2.tar.gz
--2009-02-13 18:21:12--  http://downloads.sourceforge.net/libwps/libwps-0.1.2.tar.gz
Resolving downloads.sourceforge.net... 216.34.181.60
Connecting to downloads.sourceforge.net|216.34.181.60|:80... connected.
HTTP request sent, awaiting response... 302 Found
Location: http://mesh.dl.sourceforge.net/sourceforge/libwps/libwps-0.1.2.tar.gz [following]
--2009-02-13 18:21:13--  http://mesh.dl.sourceforge.net/sourceforge/libwps/libwps-0.1.2.tar.gz
Resolving mesh.dl.sourceforge.net... 213.203.218.122
Connecting to mesh.dl.sourceforge.net|213.203.218.122|:80... connected.
HTTP request sent, awaiting response... 302 Found
Location: http://213.203.218.125/l/li/libwps/libwps-0.1.2.tar.gz [following]
--2009-02-13 18:21:13--  http://213.203.218.125/l/li/libwps/libwps-0.1.2.tar.gz
Connecting to 213.203.218.125:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 413360 (404K) [application/octet-stream]
Saving to: `libwps-0.1.2.tar.gz'

100%[======================================>] 413,360      320K/s   in 1.3s    

Last-modified header missing -- time-stamps turned off.
2009-02-13 18:21:14 (320 KB/s) - `libwps-0.1.2.tar.gz' saved [413360/413360]
Comment 5 Michael Schwendt 2009-02-14 16:51:24 EST
> I will drop the doc package

Why? You don't build --without-docs. It is due to a packaging mistake in your spec file that the html docs are lost. They get installed (see build.log), but you overwrite the installed files with your %doc entries. Examine a -bi --short-circuit install with an empty %files list if you don't believe me.

> why %check? 

To run the unittests.
Comment 6 Simon 2009-02-15 15:18:34 EST
Spec URL:
http://cassmodiah.fedorapeople.org/libwps-0.1.2/libwps.spec

SRPM URL:
http://cassmodiah.fedorapeople.org/libwps-0.1.2/libwps-0.1.2-2.fc10.src.rpm


i hope this will do the trick :-)

without 

%check
make check

it doesn't work for me

http://fpaste.org/paste/3724
line 803-841

#
Fehler:
#
cppunit/CompilerOutputter.h: Datei oder Verzeichnis nicht gefunden
#
test.cpp:38: Fehler: »CPPUNIT_NS« wurde nicht deklariert
#
test.cpp:38: Fehler: expected `{' before »TestFixture«
#
test.cpp:38: Fehler: Funktionsdefinition deklariert keine Parameter
#
test.cpp:53: Fehler: invalid use of incomplete type »class Test«
#
test.cpp:38: Fehler: forward declaration of »class Test«
#
test.cpp:61: Fehler: invalid use of incomplete type »class Test«
#
test.cpp:38: Fehler: forward declaration of »class Test«
#
test.cpp:66: Fehler: invalid use of incomplete type »class Test«
#
test.cpp:38: Fehler: forward declaration of »class Test«
#
test.cpp:207: Fehler: expected constructor, destructor, or type conversion before »;« token
#
test.cpp: In function »int main(int, char**)«:
#
test.cpp:212: Fehler: »CPPUNIT_NS« wurde nicht deklariert
#
test.cpp:212: Fehler: expected `;' before »controller«
#
test.cpp:215: Fehler: »CPPUNIT_NS« wurde nicht deklariert
#
test.cpp:215: Fehler: expected `;' before »result«
#
test.cpp:216: Fehler: »controller« wurde in diesem Gültigkeitsbereich nicht definiert
#
test.cpp:216: Fehler: »result« wurde in diesem Gültigkeitsbereich nicht definiert
#
test.cpp:219: Fehler: »CPPUNIT_NS« wurde nicht deklariert
#
test.cpp:219: Fehler: expected `;' before »progress«
#
test.cpp:220: Fehler: »progress« wurde in diesem Gültigkeitsbereich nicht definiert
#
test.cpp:223: Fehler: »CPPUNIT_NS« wurde nicht deklariert
#
test.cpp:223: Fehler: expected `;' before »runner«
#
test.cpp:224: Fehler: »runner« wurde in diesem Gültigkeitsbereich nicht definiert
#
test.cpp:224: Fehler: »CPPUNIT_NS« wurde nicht deklariert
#
test.cpp:228: Fehler: »CPPUNIT_NS« wurde nicht deklariert
#
test.cpp:228: Fehler: expected `;' before »outputter«
#
test.cpp:229: Fehler: »outputter« wurde in diesem Gültigkeitsbereich nicht definiert
#
make[1]:
#
*** [test.o] Fehler 1
#
make[1]: Leaving directory `/builddir/build/BUILD/libwps-0.1.2/src/test'
#
make:
#
*** [check] Fehler 2
#
Fehler:
#
Fehler beim Bauen des RPM:
#
Fehler-Status beim Beenden von /var/tmp/rpm-tmp.YL5rL9 (%check)
#
Fehler-Status beim Beenden von /var/tmp/rpm-tmp.YL5rL9 (%check)
#
Child returncode was: 1

can we leave %check out? it looks like very "optional"
Comment 7 Michael Schwendt 2009-02-15 15:43:39 EST
You're missing "BuildRequires: cppunit-devel"

"check" is a top-level target in the Makefile. Among packagers, it's common practise to run such a testsuite, if available, and only ignore it if it's known to be out-of-date/broken.
Comment 8 Simon 2009-02-15 16:22:01 EST
http://rafb.net/p/WgG5M476.html

my local build log

line 1416-1431 (in english this time, sorry for this german post above)

cd src/test && make check
make[1]: Entering directory `/builddir/build/BUILD/libwps-0.1.2/src/test'
if g++ -DHAVE_CONFIG_H -I. -I. -I../..    -I../../src/lib/ -I/usr/include/libwpd-0.8   -DNDEBUG -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -MT test.o -MD -MP -MF ".deps/test.Tpo" -c -o test.o test.cpp; \
	then mv -f ".deps/test.Tpo" ".deps/test.Po"; else rm -f ".deps/test.Tpo"; exit 1; fi
/bin/sh ../../libtool --tag=CXX --mode=link g++ -I../../src/lib/ -I/usr/include/libwpd-0.8   -DNDEBUG -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables   -o test -L../lib/ `cppunit-config --libs` test.o ../lib/libwps-0.1.la ../lib/libwps-stream-0.1.la -lwpd-0.8    
mkdir .libs
g++ -I../../src/lib/ -I/usr/include/libwpd-0.8 -DNDEBUG -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -o .libs/test test.o  -L/builddir/build/BUILD/libwps-0.1.2/src/lib -lcppunit -ldl ../lib/.libs/libwps-0.1.so ../lib/.libs/libwps-stream-0.1.so -lwpd-0.8
creating test
./test
/builddir/build/BUILD/libwps-0.1.2/src/test/.libs/lt-test: error while loading shared libraries: libwps-0.1.so.1: cannot open shared object file: No such file or directory
make[1]: 
*** [check] Error 127
make[1]: Leaving directory `/builddir/build/BUILD/libwps-0.1.2/src/test'
make: 
*** [check] Error 2
RPM build errors:



looks like make check will need the libwps package
Comment 9 Simon 2009-02-15 17:16:22 EST
mh, he can't build it, because Orcan 'oget' Ogetbil detects a rpath and send me these to lines to fix that.

sed -i 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g' libtool
sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool

so I have to patch the makefile to handle %check?
should i do that?
Comment 10 Michael Schwendt 2009-02-16 03:26:49 EST
Typically, such test-suites are made to be run after "make install", which means in RPM buildroot installations one needs to consider that:

   %check
   LD_LIBRARY_PATH=$(pwd)/src/lib/.libs make check

I'm certain other packages do something similar.

[...]

cd src/test && make check
make[1]: Entering directory `/home/qa/tmp/rpm/BUILD/libwps-0.1.2/src/test'
./test
Test::testStream : OK
OK (1)
make[1]: Leaving directory `/home/qa/tmp/rpm/BUILD/libwps-0.1.2/src/test'
Comment 11 Simon 2009-02-16 13:43:55 EST
Spec URL:
http://cassmodiah.fedorapeople.org/libwps-0.1.2/libwps.spec

SRPM URL:
http://cassmodiah.fedorapeople.org/libwps-0.1.2/libwps-0.1.2-3.fc10.src.rpm

Okay, I added this - Thank you.

I`ll try to learn more about this!
Comment 12 Michael Schwendt 2009-02-17 02:47:15 EST
There's still some oddity.

Please uncomment all your %doc entries, then run "rpmbuild -bi libwps.spec", and take a close look at the RPM build errors. What do you notice?  (cf. comment 5)
Comment 13 Simon 2009-02-17 05:52:26 EST
i know there is a conflict

i would rather pack all docs in -doc as &{_docdir}/%{name}-%{version} 
but i thought splitting will be a good compromiss, but i noticed i will overwrite the doc-directory of the installation.

what is the right way?
Comment 14 Michael Schwendt 2009-02-17 06:26:41 EST
Nothing in the guidelines, so:

In %install I would fix the permissions of the installed documentation files (chmod -x) in the buildroot, remove the INSTALL file, and use this:

  %files doc
  %defattr(-,root,root,-)
  %_docdir/%{name}-%{version}/html/

and in the main pkg:

  %_docdir/%{name}-%{version}/
  %exclude %_docdir/%{name}-%{version}/html/

Of course, you can distribute the files to subpkgs even further, but why? All documentation is in the same doc rootdir, which is fine as it is.

The -doc package is just 168 KB and 1.7 MB extracted. It would not even hurt much if you included the html tree in the -devel pkg instead. A matter of taste.

[...]

On the contrary, your current spec file creates four different docdirs:

  /usr/share/doc/libwps-0.1.2/
  /usr/share/doc/libwps-devel-0.1.2/
  /usr/share/doc/libwps-doc.0.1.2/
  /usr/share/doc/libwps-tools-0.1.2/

with -tools and -devel only containing a single file, and with the API docs not in -devel, but in the -doc dir.
Comment 15 Simon 2009-02-26 15:07:07 EST
I'm very unhappy with it...

%files
%defattr(-,root,root,-)
%doc CHANGES COPYING CREDITS README
%{_libdir}/*.so.*


%files devel
%defattr(-,root,root,-)
%doc HACKING docs/doxygen/html
%{_includedir}/*
%{_libdir}/*.so
%{_libdir}/pkgconfig/%{name}*


%files tools
%defattr(-,root,root,-)
%{_bindir}/wps2*


this is imho shorter and easier.
Comment 16 Michael Schwendt 2009-03-02 04:44:44 EST
Meanwhile I'm inclined to say:

  Oh well, go ahead, shoot yourself into the foot. ;)

It's just documentation afterall. :-) Perhaps you'll be lucky and in future upgrades this particular package will never install any additional doc files which you would kill silently with %doc.
Comment 17 Simon 2009-03-10 15:03:23 EDT
mh,

hope we can finished it, withouf foot-killing... but hey I have two of them...

Spec URL:
http://cassmodiah.fedorapeople.org/libwps-0.1.2/libwps.spec

SRPM URL:
http://cassmodiah.fedorapeople.org/libwps-0.1.2/libwps-0.1.2-5.fc10.src.rpm

I did it your way (not Frank Sinatra's way), but without chmod -x :-p

Hope this is the last docissue for this package :-)
Comment 18 Michael Schwendt 2009-03-16 07:07:14 EDT
APPROVED: libwps-0.1.2-5.fc10.src.rpm

[...]

It's still less than ideal that you create three different %doc dirs, two of them for just a single file,

/usr/share/doc/libwps-0.1.2/
/usr/share/doc/libwps-tools-0.1.2/
/usr/share/doc/libwps-devel-0.1.2/

with files 'HACKING' and 'README' duplicated in the main pkg, and with the API documentation not in the -devel %docdir, but as you're free to modify the package in pkg cvs, it wouldn't surprise me if you changed your mind for a future update. ;)
Comment 19 Simon 2009-03-16 15:50:14 EDT
thank you for your review!


New Package CVS Request
=======================
Package Name: libwps
Short Description: Library for reading and converting Microsoft Works word processor documents
Owners: cassmodiah
Branches: F-10
Comment 20 Kevin Fenzi 2009-03-17 01:17:39 EDT
cvs done.
Comment 21 Fedora Update System 2009-03-17 10:09:37 EDT
libwps-0.1.2-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/libwps-0.1.2-5.fc10
Comment 22 Fedora Update System 2009-04-13 15:39:53 EDT
libwps-0.1.2-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

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