Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 226061 - Merge Review: libwvstreams
Summary: Merge Review: libwvstreams
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kamil Dudka
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 19:29 UTC by Nobody's working on this, feel free to take it
Modified: 2010-01-14 19:56 UTC (History)
3 users (show)

Fixed In Version: libwvstreams-4.6.1-2.fc13
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-01-13 17:47:00 UTC
kdudka: fedora-review+


Attachments (Terms of Use)
failure of parallel build (deleted)
2010-01-13 08:17 UTC, Kamil Dudka
no flags Details
patching making it possible to build in parallel (deleted)
2010-01-13 11:03 UTC, Kamil Dudka
ovasik: review+
Details | Diff
patch making it possible to build in parallel (V2) (deleted)
2010-01-13 14:56 UTC, Kamil Dudka
ovasik: review+
Details | Diff

Description Nobody's working on this, feel free to take it 2007-01-31 19:29:08 UTC
Fedora Merge Review: libwvstreams

http://cvs.fedora.redhat.com/viewcvs/devel/libwvstreams/
Initial Owner: harald@redhat.com

Comment 1 Kamil Dudka 2010-01-12 16:07:08 UTC
considered OK
=============
- silent rpmlint
- sane summary/description
- valid project URL and source URL
- upstream tarball is the same as the one from look-aside cache
- md5 hash matches the sources
- dist tag OK
- license tag OK
- BuildRoot tag OK
- configure options OK
- %clean OK
- %files OK
- %defattr OK
- %doc OK


may be better
=============
- duplicated BuildRequires for pkgconfig (already required by openssl-devel)
- patch wvstreams-4.5-noxplctarget.patch has no link to the upstream thread
- make LIBS_DBUS=/%{_lib}/libdbus-1.so deserves a comment
- make should either use %{?_smp_mflags} or there should be a comment why the parallel build does not work

- -fpermissive should not appear in COPTS:
cc1: warning: command line option "-fpermissive" is valid for C++/ObjC++ but not for C

Comment 2 Ondrej Vasik 2010-01-12 17:29:06 UTC
Thanks for review...

I would prefer to keep BuildRequires for pkgconfig - although it is almost sure it will be required in future by openssl-devel, someone may modify the spec file, throw away with-openssl configure option and openssl-devel requirement - and missing dependency will appear. It is harmless as it (slightly) affects only build speed/dependency resolving...

Patch is missing link to upstream thread as it probably was not reported, it seems I forgot to do that. I'll do that tomorrow.

make LIBS_DBUS=/%{_lib}/libdbus-1.so commented - upstream searches for .a library and and without hardcoding LIBS_DBUS it caused build failures.

parallel build will be used, "-fpermissive" from COPTS thrown away.

Commited to CVS, building as libwvstreams-4.6.1-2.fc13 .

Comment 3 Kamil Dudka 2010-01-12 17:40:03 UTC
(In reply to comment #2)
> I would prefer to keep BuildRequires for pkgconfig

fine by me

> make LIBS_DBUS=/%{_lib}/libdbus-1.so commented - upstream searches for .a
> library and and without hardcoding LIBS_DBUS it caused build failures.

great, the added comment makes it more obvious

> Commited to CVS, building as libwvstreams-4.6.1-2.fc13 .    

I'll check it tomorrow if nothing urgent happens.

Comment 4 Kamil Dudka 2010-01-13 08:17:17 UTC
Created attachment 383429 [details]
failure of parallel build

I realized that adding %{?_smp_mflags} wasn't the best idea.  Perhaps I should have tested it myself, before writing the review :-)  Attached is the build log from my 8-core box:

$ rpmbuild --rebuild libwvstreams-4.6.1-2.fc13.src.rpm 2>&1 | tee build.log

I think the change should be reverted and commented in the specfile and maybe reported upstream if the fix is non-trivial.  I haven't investigated further.

Comment 5 Kamil Dudka 2010-01-13 11:03:38 UTC
Created attachment 383456 [details]
patching making it possible to build in parallel

Comment 6 Ondrej Vasik 2010-01-13 12:38:18 UTC
Comment on attachment 383456 [details]
patching making it possible to build in parallel

looks sane...

Comment 7 Kamil Dudka 2010-01-13 14:56:25 UTC
Created attachment 383497 [details]
patch making it possible to build in parallel (V2)

add libwvdbus.so to the list of dependencies when building with dbus support

Comment 8 Ondrej Vasik 2010-01-13 16:46:43 UTC
Thanks Kamil. Finally built as libwvstreams-4.6.1-2.fc13.

Comment 9 Kamil Dudka 2010-01-13 16:56:37 UTC
fedora‑review+, package is sane ... this bug may be closed

Comment 10 Ondrej Vasik 2010-01-13 17:47:00 UTC
Ok, closing... thanks for review.

Comment 11 Kamil Dudka 2010-01-14 19:56:00 UTC
Comment on attachment 383497 [details]
patch making it possible to build in parallel (V2)

patch proposed upstream:

http://groups.google.com/group/wvstreams-devel/browse_thread/thread/a76875e0c904f04a


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