Bug 530617 - Review Request: libixp - Stand-alone client/server 9P library
Review Request: libixp - Stand-alone client/server 9P library
Status: CLOSED WONTFIX
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jochen Schmitt
Fedora Extras Quality Assurance
:
: 454025 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-23 15:02 EDT by Simon
Modified: 2010-06-08 06:07 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-06-08 06:07:09 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Simon 2009-10-23 15:02:30 EDT
Spec URL:
http://cassmodiah.fedorapeople.org/libixp/libixp.spec

SRPM URL:
http://cassmodiah.fedorapeople.org/libixp/libixp-0.5-1.fc11.src.rpm

Description:
libixp is a stand-alone client/server 9P library.
libixp's server api is heavily based on that of Plan 9's lib9p.
Comment 1 Peter Lemenkov 2009-10-29 10:14:49 EDT
*** Bug 454025 has been marked as a duplicate of this bug. ***
Comment 2 Michael Schwendt 2009-11-10 17:10:25 EST
> %{_libdir}/*.a

You build a static library only. You put it into the -devel package with a virtual -static package name.

Then why do you run /sbin/ldconfig in post/postun scriptlets?


> %package -n     ixpc
> Summary:        Plan9 file protocol client
> Group:          Applications/System
> Requires:       %{name}-static = %{version}-%{release}

Why does a client binary require the static library package?


> make %{?_smp_mflags} CC="%{__cc} -c %{optflags}"

This doesn't make the package adhere to the optflags guidelines. The project's internal CFLAGS override some of the optflags. It would be more clean if you could patch mk/gcc.mk and append $RPM_OPT_FLAGS (or %optflags) to $CFLAGS.
Comment 3 Jochen Schmitt 2009-11-19 11:27:16 EST
Please work out the comments of mschwendt and upload the fixed release of your package.
Comment 4 Simon 2009-11-21 03:44:22 EST
(In reply to comment #2)
> Then why do you run /sbin/ldconfig in post/postun scriptlets?
the ldconfig scriptlets are in the lib* template. i forgot to remove them!

> Why does a client binary require the static library package?
Doesn't make sense, because they are built static in the binary.
sorry 


> This doesn't make the package adhere to the optflags guidelines. The project's
> internal CFLAGS override some of the optflags. It would be more clean if you
> could patch mk/gcc.mk and append $RPM_OPT_FLAGS (or %optflags) to $CFLAGS.  

mh, I don't understand that

-current-
CC="gcc -c -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables" CFLAGS="-I$(echo .:../include:/usr/local/include:/usr/include|sed 's/:/ -I/g') -D_XOPEN_SOURCE=600 -std=c99 -pedantic -pipe -fno-strict-aliasing -Wall -Wimplicit -Wmissing-prototypes -Wno-comment -Wno-missing-braces -Wno-parentheses -Wno-sign-compare -Wno-switch -Wpointer-arith -Wreturn-type -Wstrict-prototypes -Wtrigraphs -g -O1 -fno-builtin -fno-inline -fno-omit-frame-pointer -fno-optimize-sibling-calls -fno-unroll-loops -DIXPlint -O0  -DVERSION=\"0.5\" -D_XOPEN_SOURCE=600" ../util/compile ixpc.o ixpc.c

-new?-
CC="gcc -c -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables" CFLAGS="-I$(echo .:../include:/usr/local/include:/usr/include|sed 's/:/ -I/g') -D_XOPEN_SOURCE=600 -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables -std=c99 -pedantic -pipe -fno-strict-aliasing -Wall -Wimplicit -Wmissing-prototypes -Wno-comment -Wno-missing-braces -Wno-parentheses -Wno-sign-compare -Wno-switch -Wpointer-arith -Wreturn-type -Wstrict-prototypes -Wtrigraphs -g -O1 -fno-builtin -fno-inline -fno-omit-frame-pointer -fno-optimize-sibling-calls -fno-unroll-loops -DIXPlint -O0  -DVERSION=\"0.5\" -D_XOPEN_SOURCE=600" ../util/compile ixpc.o ixpc.c

I know these outputs are different, but i printed it and tallied it. in the new is not a flag more or less then in the current and reverse. Or not?
The order of the flags is important, or?
In the new? output i sorted it fedora_optflags and upstream_flags. You said that upstream_flags override some fedora_optflags, so it should be upstream_flags and fedora_optflags, the secound mentoined which conflicts with the first mentoined will override the first mentoined, or? (This sounds logical to me)

-upstream and fedora instead of fedora and upstream-
CC="gcc -c -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables" CFLAGS="-I$(echo .:../include:/usr/local/include:/usr/include|sed 's/:/ -I/g') -D_XOPEN_SOURCE=600 -std=c99 -pedantic -pipe -fno-strict-aliasing -Wall -Wimplicit -Wmissing-prototypes -Wno-comment -Wno-missing-braces -Wno-parentheses -Wno-sign-compare -Wno-switch -Wpointer-arith -Wreturn-type -Wstrict-prototypes -Wtrigraphs -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables -g -O1 -fno-builtin -fno-inline -fno-omit-frame-pointer -fno-optimize-sibling-calls -fno-unroll-loops -DIXPlint -O0  -DVERSION=\"0.5\" -D_XOPEN_SOURCE=600" ../util/compile ixpc.o ixpc.c

This output is equal to the current-output...  I'm confused, sorry!
Comment 5 Jochen Schmitt 2009-11-25 16:15:45 EST
I suggest you should modified the mk/gcc.mk file, so that  $RPM_OPT_FLAGS will be added to CFLAGS.
Comment 7 Jochen Schmitt 2009-11-29 15:28:57 EST
Good:
+ Basename of the SPEC file matches with package name.
+ Package fullfill naming guildelines
+ Consistently usage of rpm macros
+ URL tag shows on proper project homepage
+ Package contains most recent version of the software
+ Could download upstream tarball via spectool -g
+ Package tar ball matches with upstream
(md5sum: 2a394310c209605ba54ecf5eab518bff)
+ License tag states MIT and LPL as valid oSS licenses
+ Package conatins verbain copy of the license text
+ Package contains subpackages
+ Subpackage has proper Requies to main package
+ Package has proper definition of BuildRoot
+ Package use smp_mflags
+ Buildroot will be cleaned at the beginning fo %clean and %install
+ Local build works fine
+ Rpmlint is silent on source rpm


Bad:
- License tag should be MIT and LPL and Public Domain, because I have found a source files which is declared as public domain oh the head of the source file
- Package doesn't create a so file of the library.
- Devel package shouldn't contains a static library
- libixp package missing
Comment 9 Michael Schwendt 2009-12-14 08:00:31 EST
This doesn't fix the issues since now you've added further problems. Probably it's necessary to expand a bit:


> %package        devel
> Provides:       %{name}-static = %{version}-%{release}

This combination, a -devel package providing a virtual -static package, is dangerous, because it is special-case 2 on:
http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries
It requires special attention during review and later updates.

$ rpmls -p libixp-devel-0.5-3.fc12.i686.rpm | grep -v shar
-rw-r--r--  /usr/include/ixp.h
-rw-r--r--  /usr/include/ixp_srvutil.h

Package doesn't contain any static library. Raises the question whether a) the header files contain enough code fragments that would be inserted when compiling them, or b) whether a library for these API headers is missing?

The answer is yes to b). After disabling the static build, the shared library for this API is missing, because it is misplaced in the "ixpc" subpackage:

$ rpmls -p ixpc-0.5-3.fc12.i686.rpm | head -3
-rwxr-xr-x  /usr/bin/ixpc
-rwxr-xr-x  /usr/lib/libixp.so
-rwxr-xr-x  /usr/lib/libixp_pthread.so

This needs work. libixp-devel alone is useless. No library to link with. Notice how /usr/bin/ixpc and examples are linked with -lixp.

rpmlint also prints new warnings when running it on the built rpms:
ixpc.i686: W: no-soname /usr/lib/libixp_pthread.so
ixpc.i686: W: no-soname /usr/lib/libixp.so

No versioned SONAME, no SONAME at all, automatic RPM dependencies will be less helpful => pray that libixp API and ABI will stay stable.

$ rpm -qpR ixpc-0.5-3.fc12.i686.rpm | grep ixp
libixp.so  


* A shared library linked with executables, such as /usr/bin/ixpc, will benefit from adding the /sbin/ldconfig scriptlets (which were inappropriate with only the static lib).


* The CFLAGS still look strange. See build.log. /usr/local/include at beginning of search paths. %optflags at the beginning, too, with a multitude of -O1, -O0 and other optimisations overriding them more than once.
Comment 10 Simon 2010-06-01 04:33:29 EDT
I still haven't understand why I must not build this package as a static lib.
Comment 11 Simon 2010-06-08 06:07:09 EDT
closed, because wmii 3.9 will ship an own copy of the ixp lib.
https://fedoraproject.org/wiki/Packaging:Guidelines#Duplication_of_system_libraries
It makes more sense to use the shipped version instead.
Thank you for your comments!

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