Bug 446651 - Review Request: rvm - RVM library
Review Request: rvm - RVM library
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On: 446650
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-15 11:07 EDT by Hans de Goede
Modified: 2009-09-09 12:27 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-05-16 18:13:16 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2008-05-15 11:07:29 EDT
Spec URL: http://people.atrpms.net/~hdegoede/rvm.spec
SRPM URL: http://people.atrpms.net/~hdegoede/rvm-1.15-1.fc10.src.rpm
Description:
The RVM persistent recoverable memory library. The RVM library is used by
the Coda distributed filesystem.
Comment 1 Mamoru TASAKA 2008-05-15 11:18:48 EDT
I would appreciate it if you would review my review request
(bug 444933)
Comment 2 Mamoru TASAKA 2008-05-15 14:43:36 EDT
For 1.15-1:

* License:
----------------------------------------------------
include/coda_mmap_anon.h	GPLv2
rvm/rvm_map.c			#include "coda_mmap_anon.h"
   792  DEBUG:  gcc -DHAVE_CONFIG_H -I. -I. -I.. -I../include -DRVM_USELWP -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 librvmlwp_la-rvm_map.lo -MD -MP -MF
.deps/librvmlwp_la-rvm_map.Tpo -c rvm_map.c  -fPIC -DPIC -o
.libs/librvmlwp_la-rvm_map.o
   828  DEBUG: gcc -shared  .libs/librvmlwp_la-rvm_init.o
.libs/librvmlwp_la-rvm_map.o .libs/librvmlwp_la-rvm_unmap.o
.libs/librvmlwp_la-rvm_tran
s.o .libs/librvmlwp_la-rvm_logstatus.o .libs/librvmlwp_la-rvm_logflush.o
.libs/librvmlwp_la-rvm_logrecovr.o .libs/librvmlwp_la-rvm_utils.o .libs/
librvmlwp_la-rvm_io.o .libs/librvmlwp_la-rvm_status.o
.libs/librvmlwp_la-rvm_debug.o .libs/librvmlwp_la-rvm_printers.o  -llwp  -m32
-march=i386 -
mtune=generic -Wl,-soname -Wl,librvmlwp.so.1 -o .libs/librvmlwp.so.1.2.2

seg/rvm_segutil.c		#include "coda_mmap_anon.h"
   846  DEBUG:  gcc -DHAVE_CONFIG_H -I. -I. -I.. -I../include -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
libseglwp_la-rvm_segutil.lo -MD -MP -MF .deps/libseglwp_la-rvm_segutil.Tpo -c
rvm_segutil.c  -fPIC -DPIC -o .libs/libseglwp_la-rvm_segutil.o
   857  DEBUG: gcc -shared  .libs/libseglwp_la-rvm_segutil.o
.libs/libseglwp_la-rvm_loadseg.o .libs/libseglwp_la-rvm_createseg.o .libs/libseglwp_
la-rvm_releaseseg.o  ../rvm/.libs/librvmlwp.so  -m32 -march=i386 -mtune=generic
-Wl,-soname -Wl,libseglwp.so.1 -o .libs/libseglwp.so.1.2.2

   900  DEBUG: gcc -shared  .libs/librdslwp_la-rds_coalesce.o
.libs/librdslwp_la-rds_free.o .libs/librdslwp_la-rds_init.o
.libs/librdslwp_la-rds_malloc.o .libs/librdslwp_la-rds_prealloc.o
.libs/librdslwp_la-rds_split.o .libs/librdslwp_la-rds_stats.o
.libs/librdslwp_la-rds_start.o .libs/librdslwp_la-rds_util.o
.libs/librdslwp_la-rds_zap.o .libs/librdslwp_la-rds_maxblock.o 
../seg/.libs/libseglwp.so ../rvm/.libs/librvmlwp.so -llwp  -m32 -march=i386
-mtune=generic -Wl,-soname -Wl,librdslwp.so.1 -o .libs/librdslwp.so.1.2.2
----------------------------------------------------
  - All GPLv2!! :(

* Dependency for devel package
----------------------------------------------------
[tasaka1@localhost ~]$ LANG=C pkg-config --libs rvmlwp
Package lwp was not found in the pkg-config search path.
Perhaps you should add the directory containing `lwp.pc'
to the PKG_CONFIG_PATH environment variable
Package 'lwp', required by 'Recoverable Virtual Memory (lwp build)', not found
----------------------------------------------------
  - This means that lwp-devel should be in the Requires
    of rvm-devel.

* rpath hack
----------------------------------------------------
# Don't use rpath!
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
# work around linking failures because of the disabling of rpath above
export LD_LIBRARY_PATH=`pwd`/rvm/.libs:`pwd`/seg/.libs
----------------------------------------------------
  - Umm.. are there any better hack than this way? Especially
    adding the need of LD_LIBRARY_PATH like above is IMO
    not good, just creating a broken libtool.

    In such case, I usually
    - add /usr/lib /lib64 to sys_lib_search_path_spec in 
      configure (as libtool is created from configure)
    
      Actually the diff of %_bindir/libtool between i386 and x86_64
      shows this.

    - Or simply "make LIBTOOL=%_bindir/libtool" or cp %_bindir/libtool
      to working directory (BR: libtool is needed).
Comment 3 Mamoru TASAKA 2008-05-15 14:54:58 EDT
(In reply to comment #2)
>     - add /usr/lib /lib64 to sys_lib_search_path_spec in 
      - add /usr/lib64 /lib64 to ....

>       configure (as libtool is created from configure)
Comment 4 Hans de Goede 2008-05-15 16:26:20 EDT
(In reply to comment #2)
> For 1.15-1:
> 

Thanks for the review!

> * License:
> ----------------------------------------------------
> include/coda_mmap_anon.h	GPLv2

<snip>

>   - All GPLv2!! :(

Yes, I already saw that, but the only code that file contains is:

#define mmap_anon(raddrptr, addrptr, len, prot) do { \
    int fd = -1, flags = MAP_ANON | MAP_PRIVATE; \
    if (addrptr) flags |= MAP_FIXED; \
    if (!MAP_ANON) fd = open("/dev/zero", O_RDWR); \  
    raddrptr = mmap((char *)addrptr, len, prot, flags, fd, 0); \
    if (fd != -1) close(fd); \
} while(0);

Which IMHO is hardly enough code to be copyrightable. Anyways rvm uses lwp, and
that definitely is GPLv2 (although I believe that is not upstream's intent, I'll
contact them about this). So no matter what rvm is effectively GPLv2 licensed.

So I'm fine with either way let me know if you agree with the barely
copyrightable, or if you want the License tag changed

> * Dependency for devel package
> ----------------------------------------------------
> [tasaka1@localhost ~]$ LANG=C pkg-config --libs rvmlwp
> Package lwp was not found in the pkg-config search path.
> Perhaps you should add the directory containing `lwp.pc'
> to the PKG_CONFIG_PATH environment variable
> Package 'lwp', required by 'Recoverable Virtual Memory (lwp build)', not found
> ----------------------------------------------------
>   - This means that lwp-devel should be in the Requires
>     of rvm-devel.
> 

Will fix when I know what todo with the license tag (and rpath)

> * rpath hack
> ----------------------------------------------------
> # Don't use rpath!
> 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
> # work around linking failures because of the disabling of rpath above
> export LD_LIBRARY_PATH=`pwd`/rvm/.libs:`pwd`/seg/.libs
> ----------------------------------------------------
>   - Umm.. are there any better hack than this way?

Well this (except for the LD_LIBRARY_PATH is whats adviced by the guidelines

>     Especially
>     adding the need of LD_LIBRARY_PATH like above is IMO
>     not good, just creating a broken libtool.
> 
>     In such case, I usually
>     - add /usr/lib /lib64 to sys_lib_search_path_spec in 
>       configure (as libtool is created from configure)
>     
>       Actually the diff of %_bindir/libtool between i386 and x86_64
>       shows this.
> 

Yes, that will work, but I don't like patching configure, as a patch will break
with each new release, but I guess I could sed this, but then again my sed isn't
as good as yours (I've been reviewing cairo-dock, some mighty fine sed code there :)

>     - Or simply "make LIBTOOL=%_bindir/libtool" or cp %_bindir/libtool
>       to working directory (BR: libtool is needed).

Thats evil, you should never use a different libtool then the one automake used
when generating the Makefiles, otherwise their might be all kinda trouble, if
you want todo this you should use "libtoolize -f" and then "autoreconf -f -i",
then again maybe autoreconf will also call the libtoolize for you so just
autoreconf will be enough.

Anyways I prefer to keep this as is, as this is how its done in all my packages,
but if you insist I'll switch to changing sys_lib_search_path_spec
Comment 5 Mamoru TASAKA 2008-05-15 20:41:30 EDT
Okay, thanks you for good explanation.

-------------------------------------------------------------------------
    This package (rvm) is APPROVED by me
-------------------------------------------------------------------------
Comment 6 Hans de Goede 2008-05-16 02:30:48 EDT
New Package CVS Request
=======================
Package Name:      rvm
Short Description: RVM library
Owners:            jwrdegoede
Branches:          F-8 F-9
InitialCC:
Cvsextras Commits: Yes

Comment 7 Kevin Fenzi 2008-05-16 11:47:30 EDT
cvs done.
Comment 8 Hans de Goede 2008-05-16 18:13:16 EDT
Imported and build for F-8 F-9 devel, will get pushed as update for F-8 and F-9
together with the rest of the coda stack once coda itself has been built too.
Comment 9 Adam Goode 2009-09-08 17:06:40 EDT
Package Change Request
======================
Package Name: rvm
New Branches: EL-5
Owners: agoode nhorman
Comment 10 Kevin Fenzi 2009-09-09 12:27:40 EDT
cvs done.

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