Bug 226061 - Merge Review: libwvstreams
Merge Review: libwvstreams
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kamil Dudka
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 14:29 EST by Nobody's working on this, feel free to take it
Modified: 2010-01-14 14:56 EST (History)
3 users (show)

See Also:
Fixed In Version: libwvstreams-4.6.1-2.fc13
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-01-13 12:47:00 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kdudka: fedora‑review+


Attachments (Terms of Use)
failure of parallel build (9.07 KB, text/plain)
2010-01-13 03:17 EST, Kamil Dudka
no flags Details
patching making it possible to build in parallel (1.58 KB, patch)
2010-01-13 06:03 EST, Kamil Dudka
ovasik: review+
Details | Diff
patch making it possible to build in parallel (V2) (1.68 KB, patch)
2010-01-13 09:56 EST, Kamil Dudka
ovasik: review+
Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 14:29:08 EST
Fedora Merge Review: libwvstreams

http://cvs.fedora.redhat.com/viewcvs/devel/libwvstreams/
Initial Owner: harald@redhat.com
Comment 1 Kamil Dudka 2010-01-12 11:07:08 EST
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 12:29:06 EST
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 12:40:03 EST
(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 03:17:17 EST
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 06:03:38 EST
Created attachment 383456 [details]
patching making it possible to build in parallel
Comment 6 Ondrej Vasik 2010-01-13 07:38:18 EST
Comment on attachment 383456 [details]
patching making it possible to build in parallel

looks sane...
Comment 7 Kamil Dudka 2010-01-13 09:56:25 EST
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 11:46:43 EST
Thanks Kamil. Finally built as libwvstreams-4.6.1-2.fc13.
Comment 9 Kamil Dudka 2010-01-13 11:56:37 EST
fedora‑review+, package is sane ... this bug may be closed
Comment 10 Ondrej Vasik 2010-01-13 12:47:00 EST
Ok, closing... thanks for review.
Comment 11 Kamil Dudka 2010-01-14 14:56:00 EST
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.