Bug 920518 - Review Request: metis - Serial Graph Partitioning and Fill-reducing Matrix Ordering
Summary: Review Request: metis - Serial Graph Partitioning and Fill-reducing Matrix O...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Paulo Andrade
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: ascend
TreeView+ depends on / blocked
 
Reported: 2013-03-12 10:03 UTC by Antonio T. (sagitter)
Modified: 2013-04-14 19:20 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-03-24 22:40:29 UTC
Type: ---
Embargoed:
paulo.cesar.pereira.de.andrade: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
build.log from fedora-reviw results directory (8.37 KB, application/x-gzip)
2013-03-20 17:11 UTC, Antonio T. (sagitter)
no flags Details

Description Antonio T. (sagitter) 2013-03-12 10:03:32 UTC
Spec URL: http://sagitter.fedorapeople.org/metis/metis.spec
SRPM URL: http://sagitter.fedorapeople.org/metis/metis-5.0.3-0.fc18.src.rpm

Description: METIS is a set of serial programs for partitioning graphs, 
partitioning finite element meshes, and producing fill reducing 
orderings for sparse matrices. 
The algorithms implemented in METIS are based on the multilevel 
recursive-bisection, multilevel k-way, and multi-constraint 
partitioning schemes developed in our lab.

Fedora Account System Username: sagitter

Koji builds: http://koji.fedoraproject.org/koji/taskinfo?taskID=5111564

Metis now is released with a license compatible with Fedora. In source, there are other licenses:

$ licensecheck -r * | grep -v UNKNOWN
GKlib/getopt.c: LGPL (v2.1 or later) (with incorrect FSF address)
GKlib/ms_stdint.h: BSD (2 clause)
GKlib/gkregex.c: LGPL (v2.1 or later) (with incorrect FSF address)
GKlib/gk_mksort.h: LGPL (v2.1 or later) (with incorrect FSF address)
GKlib/random.c: BSD
GKlib/ms_inttypes.h: BSD (2 clause)
GKlib/gkregex.h: LGPL (v2.1 or later) (with incorrect FSF address)
libmetis/gklib_rename.h: GENERATED FILE

I don't know if they must included all in .spec file together with the ASL 2.0 .

Comment 1 Paulo Andrade 2013-03-14 13:42:47 UTC
Some comments after an initial look in the package:

--------------------------------------------------
1. You should avoid as much as possible a manual install because that
   easily does not do what upstream intend or end up installing
   files that should not be installed. After running make
   install setting DESTDIR I see:
$ find .
.
./usr
./usr/include
./usr/include/metis.h
./usr/lib
./usr/lib/libmetis.so
./usr/bin
./usr/bin/gpmetis
./usr/bin/cmpfillin
./usr/bin/m2gmetis
./usr/bin/mpmetis
./usr/bin/ndmetis
./usr/bin/graphchk

That means at first that -DLIB_SUFFIX=64 is not being passed to
cmake, so, either should patch the Makefile that calls cmake, or,
since the cmake wrapper in the toplevel Makefile is not that
complex, call cmake explicitly, example:
mkdir build
pushd build
  %cmake $CMAKE_OPTIONS ..
  %make %{?_smp_mflags}
popd

Also, after running make install it is not required to mess
with rpath:
$ chrpath -l usr/bin/cmpfillin 
usr/bin/cmpfillin: no rpath or runpath tag found.


--------------------------------------------------
2. Several features appear to not be set/used. I believe another
   strong reason to call cmake explicitly:
---%<---
$ cmake -LA
-- The C compiler identification is GNU 4.8.0
-- The CXX compiler identification is GNU 4.8.0
-- Check for working C compiler: /usr/lib64/ccache/cc
-- Check for working C compiler: /usr/lib64/ccache/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: /usr/lib64/ccache/c++
-- Check for working CXX compiler: /usr/lib64/ccache/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Looking for execinfo.h
-- Looking for execinfo.h - found
-- Looking for getline
-- Looking for getline - found
CMake Error at CMakeLists.txt:9 (ADD_EXECUTABLE):
  Cannot find source file:

    GKlib/conf/check_thread_storage.c

  Tried extensions .c .C .c++ .cc .cpp .cxx .m .M .mm .h .hh .h++ .hm .hpp
  .hxx .in .txx


CMake Error: Internal CMake error, TryCompile generation of cmake failed
-- checking for thread-local storage - not found
-- Configuring incomplete, errors occurred!
-- Cache values
ASSERT:BOOL=OFF
ASSERT2:BOOL=OFF
CMAKE_AR:FILEPATH=/usr/bin/ar
CMAKE_BUILD_TYPE:STRING=
CMAKE_COLOR_MAKEFILE:BOOL=ON
CMAKE_CXX_COMPILER:FILEPATH=/usr/lib64/ccache/c++
CMAKE_CXX_FLAGS:STRING=
CMAKE_CXX_FLAGS_DEBUG:STRING=-g
CMAKE_CXX_FLAGS_MINSIZEREL:STRING=-Os -DNDEBUG
CMAKE_CXX_FLAGS_RELEASE:STRING=-O3 -DNDEBUG
CMAKE_CXX_FLAGS_RELWITHDEBINFO:STRING=-O2 -g -DNDEBUG
CMAKE_C_COMPILER:FILEPATH=/usr/lib64/ccache/cc
CMAKE_C_FLAGS:STRING=
CMAKE_C_FLAGS_DEBUG:STRING=-g
CMAKE_C_FLAGS_MINSIZEREL:STRING=-Os -DNDEBUG
CMAKE_C_FLAGS_RELEASE:STRING=-O3 -DNDEBUG
CMAKE_C_FLAGS_RELWITHDEBINFO:STRING=-O2 -g -DNDEBUG
CMAKE_EXE_LINKER_FLAGS:STRING= 
CMAKE_EXE_LINKER_FLAGS_DEBUG:STRING=
CMAKE_EXE_LINKER_FLAGS_MINSIZEREL:STRING=
CMAKE_EXE_LINKER_FLAGS_RELEASE:STRING=
CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO:STRING=
CMAKE_EXPORT_COMPILE_COMMANDS:BOOL=OFF
CMAKE_INSTALL_PREFIX:PATH=/usr/local
CMAKE_LINKER:FILEPATH=/usr/bin/ld
CMAKE_MAKE_PROGRAM:FILEPATH=/usr/bin/gmake
CMAKE_MODULE_LINKER_FLAGS:STRING= 
CMAKE_MODULE_LINKER_FLAGS_DEBUG:STRING=
CMAKE_MODULE_LINKER_FLAGS_MINSIZEREL:STRING=
CMAKE_MODULE_LINKER_FLAGS_RELEASE:STRING=
CMAKE_MODULE_LINKER_FLAGS_RELWITHDEBINFO:STRING=
CMAKE_NM:FILEPATH=/usr/bin/nm
CMAKE_OBJCOPY:FILEPATH=/usr/bin/objcopy
CMAKE_OBJDUMP:FILEPATH=/usr/bin/objdump
CMAKE_RANLIB:FILEPATH=/usr/bin/ranlib
CMAKE_SHARED_LINKER_FLAGS:STRING= 
CMAKE_SHARED_LINKER_FLAGS_DEBUG:STRING=
CMAKE_SHARED_LINKER_FLAGS_MINSIZEREL:STRING=
CMAKE_SHARED_LINKER_FLAGS_RELEASE:STRING=
CMAKE_SHARED_LINKER_FLAGS_RELWITHDEBINFO:STRING=
CMAKE_SKIP_INSTALL_RPATH:BOOL=NO
CMAKE_SKIP_RPATH:BOOL=NO
CMAKE_STRIP:FILEPATH=/usr/bin/strip
CMAKE_USE_RELATIVE_PATHS:BOOL=OFF
CMAKE_VERBOSE_MAKEFILE:BOOL=FALSE
DEBUG:BOOL=OFF
GDB:BOOL=OFF
GKLIB_PATH:PATH=GKlib
GKRAND:BOOL=OFF
GKREGEX:BOOL=OFF
GPROF:BOOL=OFF
OPENMP:BOOL=OFF
PCRE:BOOL=OFF
SHARED:BOOL=FALSE
---%<---

  Yet another issue is:
---%<---
$ cd GKlib
$ make install DESTDIR=/tmp/gklib
Scanning dependencies of target GKlib
[  3%] Building C object CMakeFiles/GKlib.dir/random.c.o
[  6%] Building C object CMakeFiles/GKlib.dir/fs.c.o
[  9%] Building C object CMakeFiles/GKlib.dir/b64.c.o
[ 12%] Building C object CMakeFiles/GKlib.dir/seq.c.o
[ 16%] Building C object CMakeFiles/GKlib.dir/mcore.c.o
[ 19%] Building C object CMakeFiles/GKlib.dir/graph.c.o
[ 22%] Building C object CMakeFiles/GKlib.dir/gkregex.c.o
[ 25%] Building C object CMakeFiles/GKlib.dir/util.c.o
[ 29%] Building C object CMakeFiles/GKlib.dir/io.c.o
[ 32%] Building C object CMakeFiles/GKlib.dir/fkvkselect.c.o
[ 35%] Building C object CMakeFiles/GKlib.dir/timers.c.o
[ 38%] Building C object CMakeFiles/GKlib.dir/pqueue.c.o
[ 41%] Building C object CMakeFiles/GKlib.dir/blas.c.o
[ 45%] Building C object CMakeFiles/GKlib.dir/rw.c.o
[ 48%] Building C object CMakeFiles/GKlib.dir/string.c.o
[ 51%] Building C object CMakeFiles/GKlib.dir/itemsets.c.o
[ 54%] Building C object CMakeFiles/GKlib.dir/csr.c.o
[ 58%] Building C object CMakeFiles/GKlib.dir/tokenizer.c.o
[ 61%] Building C object CMakeFiles/GKlib.dir/getopt.c.o
[ 64%] Building C object CMakeFiles/GKlib.dir/memory.c.o
[ 67%] Building C object CMakeFiles/GKlib.dir/htable.c.o
[ 70%] Building C object CMakeFiles/GKlib.dir/omp.c.o
[ 74%] Building C object CMakeFiles/GKlib.dir/sort.c.o
[ 77%] Building C object CMakeFiles/GKlib.dir/evaluate.c.o
[ 80%] Building C object CMakeFiles/GKlib.dir/pdb.c.o
[ 83%] Building C object CMakeFiles/GKlib.dir/error.c.o
Linking C static library libGKlib.a
[ 83%] Built target GKlib
Scanning dependencies of target fis
[ 87%] Building C object test/CMakeFiles/fis.dir/fis.c.o
Linking C executable fis
[ 87%] Built target fis
Scanning dependencies of target gkgraph
[ 90%] Building C object test/CMakeFiles/gkgraph.dir/gkgraph.c.o
Linking C executable gkgraph
[ 90%] Built target gkgraph
Scanning dependencies of target gksort
[ 93%] Building C object test/CMakeFiles/gksort.dir/gksort.c.o
Linking C executable gksort
[ 93%] Built target gksort
Scanning dependencies of target rw
[ 96%] Building C object test/CMakeFiles/rw.dir/rw.c.o
Linking C executable rw
[ 96%] Built target rw
Scanning dependencies of target strings
[100%] Building C object test/CMakeFiles/strings.dir/strings.c.o
Linking C executable strings
[100%] Built target strings
Install the project...
-- Install configuration: ""
-- Installing: /tmp/gklib/usr/local/lib/libGKlib.a
-- Installing: /tmp/gklib/usr/local/include/gk_mkrandom.h
-- Installing: /tmp/gklib/usr/local/include/ms_stat.h
-- Installing: /tmp/gklib/usr/local/include/ms_stdint.h
-- Installing: /tmp/gklib/usr/local/include/gk_mkutils.h
-- Installing: /tmp/gklib/usr/local/include/gk_struct.h
-- Installing: /tmp/gklib/usr/local/include/gk_getopt.h
-- Installing: /tmp/gklib/usr/local/include/ms_inttypes.h
-- Installing: /tmp/gklib/usr/local/include/GKlib.h
-- Installing: /tmp/gklib/usr/local/include/gk_types.h
-- Installing: /tmp/gklib/usr/local/include/gk_mkpqueue.h
-- Installing: /tmp/gklib/usr/local/include/gk_mkmemory.h
-- Installing: /tmp/gklib/usr/local/include/gk_proto.h
-- Installing: /tmp/gklib/usr/local/include/gk_externs.h
-- Installing: /tmp/gklib/usr/local/include/gkregex.h
-- Installing: /tmp/gklib/usr/local/include/gk_arch.h
-- Installing: /tmp/gklib/usr/local/include/gk_macros.h
-- Installing: /tmp/gklib/usr/local/include/gk_defs.h
-- Installing: /tmp/gklib/usr/local/include/gk_mkpqueue2.h
-- Installing: /tmp/gklib/usr/local/include/gk_mkblas.h
-- Installing: /tmp/gklib/usr/local/include/gk_mksort.h
---%<---

As you can see, it installs only header files, possibly in a
wrong directory, and a static library. What I believe is not
correct in the metis-static package you are generating.

Also, should check what is, if any, the reason to build a
static libGKlib.a, otherwise, I suggest experimenting with
a shared one, e.g. untested pseudo patch to
GKlib/CMakeLists.txt:
---%<---
-add_library(GKlib STATIC ${GKlib_sources})
-add_library(GKlib SHARED ${GKlib_sources})
---%<---
*But* it looks like the GKlib sources are already added
to libmetis.so; a quick check on a random symbol...
---%<---
$ objdump -T build/Linux-x86_64/libmetis/libmetis.so| grep gk_find_frequent_itemsets
000000000001dfa0 g    DF .text  0000000000000208  Base        gk_find_frequent_itemsets
$ objdump -t GKlib/build/Linux-x86_64/libGKlib.a| grep gk_find_frequent_itemsets
00000000000003e0 g     F .text  0000000000000208 gk_find_frequent_itemsets
---%<---
so, they are indeed duplicated, and if anything, I think
should only install GKlib headers, but are those really
supposed to be exported, instead of only stuff in metis.h?

--------------------------------------------------
3. You most like need to patch include/metis.h, on Install.txt I see:
--%<--
3. Edit the file include/metis.h and specify the width (32 or 64 bits) of the
   elementary data type used in METIS. This is controled by the IDXTYPEWIDTH
   constant.

   For now, on a 32 bit architecture you can only specify a width of 32, 
   whereas for a 64 bit architecture you can specify a width of either 
   32 or 64 bits.
--%<--
So, for a 64 bit build, you probably want to patch include/metis.h,
somewhat like:
-#define IDXTYPEWIDTH 32
+#define IDXTYPEWIDTH 64
and the other option is:
--%<--
/*--------------------------------------------------------------------------
 Specifies the data type that will hold floating-point style information.

 Possible values:
   32 : single precission floating point (float)
   64 : double precission floating point (double)
--------------------------------------------------------------------------*/
#define REALTYPEWIDTH 32
--%<--
That looks better using 64 bit double, but needs testing of course...


--------------------------------------------------
4. You should use a soname for libmetis.so, I suggest something like
   the (untested) pseudo patch to libmetis/CMakeLists.txt:
---%<---
 add_library(metis ${METIS_LIBRARY_TYPE} ${GKlib_sources} ${metis_sources})
+set_target_properties(metis SOVERSION 0) 
---%<---


--------------------------------------------------
5. You should not need to install the graphs subdir to
   %{_datadir}/%{name}/graphs, but instead, use them, as
   they are actually example input files, in a %check
   section. Also, should add GKlib/test binaries to a
   %check run.

--------------------------------------------------
6. I suggest installing manual/manual.pdf as %doc.

Comment 2 Antonio T. (sagitter) 2013-03-15 13:57:50 UTC
(In reply to comment #1)
> Some comments after an initial look in the package:
> 
> --------------------------------------------------
> 1. You should avoid as much as possible a manual install because that
>    easily does not do what upstream intend or end up installing
>    files that should not be installed. After running make
>    install setting DESTDIR I see:
> $ find .
> .
> ./usr
> ./usr/include
> ./usr/include/metis.h
> ./usr/lib
> ./usr/lib/libmetis.so
> ./usr/bin
> ./usr/bin/gpmetis
> ./usr/bin/cmpfillin
> ./usr/bin/m2gmetis
> ./usr/bin/mpmetis
> ./usr/bin/ndmetis
> ./usr/bin/graphchk
> 
> That means at first that -DLIB_SUFFIX=64 is not being passed to
> cmake, so, either should patch the Makefile that calls cmake, or,
> since the cmake wrapper in the toplevel Makefile is not that
> complex, call cmake explicitly, example:
> mkdir build
> pushd build
>   %cmake $CMAKE_OPTIONS ..
>   %make %{?_smp_mflags}
> popd
> 
> Also, after running make install it is not required to mess
> with rpath:
> $ chrpath -l usr/bin/cmpfillin 
> usr/bin/cmpfillin: no rpath or runpath tag found.
> 

Some cmake option seem not be accepted by using both %cmake macro and cmake command. 

With %cmake:

CMake Warning:
  Manually-specified variables were not used by the project:

    CMAKE_C_FLAGS_RELEASE
    INCLUDE_INSTALL_DIR
    LIB_INSTALL_DIR
    LIB_SUFFIX
    SHARE_INSTALL_PREFIX
    SYSCONF_INSTALL_DIR

With cmake command:

CMake Warning:
  Manually-specified variables were not used by the project:

    CMAKE_C_FLAGS_RELEASE
    LIB_SUFFIX

so libmetis.so/libGKlib.a are always located in /usr/lib.
I don't know what I must patch to fix this issue.

Following there is a temporary version of src rpm.

Spec URL: http://sagitter.fedorapeople.org/metis/metis.spec
SRPM URL: http://sagitter.fedorapeople.org/metis/metis-5.0.3-2.fc18.src.rpm


> 
> --------------------------------------------------
> 2. Several features appear to not be set/used. I believe another
>    strong reason to call cmake explicitly:
> ---%<---
> $ cmake -LA
> -- The C compiler identification is GNU 4.8.0
> -- The CXX compiler identification is GNU 4.8.0
> -- Check for working C compiler: /usr/lib64/ccache/cc
> -- Check for working C compiler: /usr/lib64/ccache/cc -- works
> -- Detecting C compiler ABI info
> -- Detecting C compiler ABI info - done
> -- Check for working CXX compiler: /usr/lib64/ccache/c++
> -- Check for working CXX compiler: /usr/lib64/ccache/c++ -- works
> -- Detecting CXX compiler ABI info
> -- Detecting CXX compiler ABI info - done
> -- Looking for execinfo.h
> -- Looking for execinfo.h - found
> -- Looking for getline
> -- Looking for getline - found
> CMake Error at CMakeLists.txt:9 (ADD_EXECUTABLE):
>   Cannot find source file:
> 
>     GKlib/conf/check_thread_storage.c
> 
>   Tried extensions .c .C .c++ .cc .cpp .cxx .m .M .mm .h .hh .h++ .hm .hpp
>   .hxx .in .txx
> 
> 
> CMake Error: Internal CMake error, TryCompile generation of cmake failed
> -- checking for thread-local storage - not found
> -- Configuring incomplete, errors occurred!
> -- Cache values
> ASSERT:BOOL=OFF
> ASSERT2:BOOL=OFF
> CMAKE_AR:FILEPATH=/usr/bin/ar
> CMAKE_BUILD_TYPE:STRING=
> CMAKE_COLOR_MAKEFILE:BOOL=ON
> CMAKE_CXX_COMPILER:FILEPATH=/usr/lib64/ccache/c++
> CMAKE_CXX_FLAGS:STRING=
> CMAKE_CXX_FLAGS_DEBUG:STRING=-g
> CMAKE_CXX_FLAGS_MINSIZEREL:STRING=-Os -DNDEBUG
> CMAKE_CXX_FLAGS_RELEASE:STRING=-O3 -DNDEBUG
> CMAKE_CXX_FLAGS_RELWITHDEBINFO:STRING=-O2 -g -DNDEBUG
> CMAKE_C_COMPILER:FILEPATH=/usr/lib64/ccache/cc
> CMAKE_C_FLAGS:STRING=
> CMAKE_C_FLAGS_DEBUG:STRING=-g
> CMAKE_C_FLAGS_MINSIZEREL:STRING=-Os -DNDEBUG
> CMAKE_C_FLAGS_RELEASE:STRING=-O3 -DNDEBUG
> CMAKE_C_FLAGS_RELWITHDEBINFO:STRING=-O2 -g -DNDEBUG
> CMAKE_EXE_LINKER_FLAGS:STRING= 
> CMAKE_EXE_LINKER_FLAGS_DEBUG:STRING=
> CMAKE_EXE_LINKER_FLAGS_MINSIZEREL:STRING=
> CMAKE_EXE_LINKER_FLAGS_RELEASE:STRING=
> CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO:STRING=
> CMAKE_EXPORT_COMPILE_COMMANDS:BOOL=OFF
> CMAKE_INSTALL_PREFIX:PATH=/usr/local
> CMAKE_LINKER:FILEPATH=/usr/bin/ld
> CMAKE_MAKE_PROGRAM:FILEPATH=/usr/bin/gmake
> CMAKE_MODULE_LINKER_FLAGS:STRING= 
> CMAKE_MODULE_LINKER_FLAGS_DEBUG:STRING=
> CMAKE_MODULE_LINKER_FLAGS_MINSIZEREL:STRING=
> CMAKE_MODULE_LINKER_FLAGS_RELEASE:STRING=
> CMAKE_MODULE_LINKER_FLAGS_RELWITHDEBINFO:STRING=
> CMAKE_NM:FILEPATH=/usr/bin/nm
> CMAKE_OBJCOPY:FILEPATH=/usr/bin/objcopy
> CMAKE_OBJDUMP:FILEPATH=/usr/bin/objdump
> CMAKE_RANLIB:FILEPATH=/usr/bin/ranlib
> CMAKE_SHARED_LINKER_FLAGS:STRING= 
> CMAKE_SHARED_LINKER_FLAGS_DEBUG:STRING=
> CMAKE_SHARED_LINKER_FLAGS_MINSIZEREL:STRING=
> CMAKE_SHARED_LINKER_FLAGS_RELEASE:STRING=
> CMAKE_SHARED_LINKER_FLAGS_RELWITHDEBINFO:STRING=
> CMAKE_SKIP_INSTALL_RPATH:BOOL=NO
> CMAKE_SKIP_RPATH:BOOL=NO
> CMAKE_STRIP:FILEPATH=/usr/bin/strip
> CMAKE_USE_RELATIVE_PATHS:BOOL=OFF
> CMAKE_VERBOSE_MAKEFILE:BOOL=FALSE
> DEBUG:BOOL=OFF
> GDB:BOOL=OFF
> GKLIB_PATH:PATH=GKlib
> GKRAND:BOOL=OFF
> GKREGEX:BOOL=OFF
> GPROF:BOOL=OFF
> OPENMP:BOOL=OFF
> PCRE:BOOL=OFF
> SHARED:BOOL=FALSE
> ---%<---
> 
>   Yet another issue is:
> ---%<---
> $ cd GKlib
> $ make install DESTDIR=/tmp/gklib
> Scanning dependencies of target GKlib
> [  3%] Building C object CMakeFiles/GKlib.dir/random.c.o
> [  6%] Building C object CMakeFiles/GKlib.dir/fs.c.o
> [  9%] Building C object CMakeFiles/GKlib.dir/b64.c.o
> [ 12%] Building C object CMakeFiles/GKlib.dir/seq.c.o
> [ 16%] Building C object CMakeFiles/GKlib.dir/mcore.c.o
> [ 19%] Building C object CMakeFiles/GKlib.dir/graph.c.o
> [ 22%] Building C object CMakeFiles/GKlib.dir/gkregex.c.o
> [ 25%] Building C object CMakeFiles/GKlib.dir/util.c.o
> [ 29%] Building C object CMakeFiles/GKlib.dir/io.c.o
> [ 32%] Building C object CMakeFiles/GKlib.dir/fkvkselect.c.o
> [ 35%] Building C object CMakeFiles/GKlib.dir/timers.c.o
> [ 38%] Building C object CMakeFiles/GKlib.dir/pqueue.c.o
> [ 41%] Building C object CMakeFiles/GKlib.dir/blas.c.o
> [ 45%] Building C object CMakeFiles/GKlib.dir/rw.c.o
> [ 48%] Building C object CMakeFiles/GKlib.dir/string.c.o
> [ 51%] Building C object CMakeFiles/GKlib.dir/itemsets.c.o
> [ 54%] Building C object CMakeFiles/GKlib.dir/csr.c.o
> [ 58%] Building C object CMakeFiles/GKlib.dir/tokenizer.c.o
> [ 61%] Building C object CMakeFiles/GKlib.dir/getopt.c.o
> [ 64%] Building C object CMakeFiles/GKlib.dir/memory.c.o
> [ 67%] Building C object CMakeFiles/GKlib.dir/htable.c.o
> [ 70%] Building C object CMakeFiles/GKlib.dir/omp.c.o
> [ 74%] Building C object CMakeFiles/GKlib.dir/sort.c.o
> [ 77%] Building C object CMakeFiles/GKlib.dir/evaluate.c.o
> [ 80%] Building C object CMakeFiles/GKlib.dir/pdb.c.o
> [ 83%] Building C object CMakeFiles/GKlib.dir/error.c.o
> Linking C static library libGKlib.a
> [ 83%] Built target GKlib
> Scanning dependencies of target fis
> [ 87%] Building C object test/CMakeFiles/fis.dir/fis.c.o
> Linking C executable fis
> [ 87%] Built target fis
> Scanning dependencies of target gkgraph
> [ 90%] Building C object test/CMakeFiles/gkgraph.dir/gkgraph.c.o
> Linking C executable gkgraph
> [ 90%] Built target gkgraph
> Scanning dependencies of target gksort
> [ 93%] Building C object test/CMakeFiles/gksort.dir/gksort.c.o
> Linking C executable gksort
> [ 93%] Built target gksort
> Scanning dependencies of target rw
> [ 96%] Building C object test/CMakeFiles/rw.dir/rw.c.o
> Linking C executable rw
> [ 96%] Built target rw
> Scanning dependencies of target strings
> [100%] Building C object test/CMakeFiles/strings.dir/strings.c.o
> Linking C executable strings
> [100%] Built target strings
> Install the project...
> -- Install configuration: ""
> -- Installing: /tmp/gklib/usr/local/lib/libGKlib.a
> -- Installing: /tmp/gklib/usr/local/include/gk_mkrandom.h
> -- Installing: /tmp/gklib/usr/local/include/ms_stat.h
> -- Installing: /tmp/gklib/usr/local/include/ms_stdint.h
> -- Installing: /tmp/gklib/usr/local/include/gk_mkutils.h
> -- Installing: /tmp/gklib/usr/local/include/gk_struct.h
> -- Installing: /tmp/gklib/usr/local/include/gk_getopt.h
> -- Installing: /tmp/gklib/usr/local/include/ms_inttypes.h
> -- Installing: /tmp/gklib/usr/local/include/GKlib.h
> -- Installing: /tmp/gklib/usr/local/include/gk_types.h
> -- Installing: /tmp/gklib/usr/local/include/gk_mkpqueue.h
> -- Installing: /tmp/gklib/usr/local/include/gk_mkmemory.h
> -- Installing: /tmp/gklib/usr/local/include/gk_proto.h
> -- Installing: /tmp/gklib/usr/local/include/gk_externs.h
> -- Installing: /tmp/gklib/usr/local/include/gkregex.h
> -- Installing: /tmp/gklib/usr/local/include/gk_arch.h
> -- Installing: /tmp/gklib/usr/local/include/gk_macros.h
> -- Installing: /tmp/gklib/usr/local/include/gk_defs.h
> -- Installing: /tmp/gklib/usr/local/include/gk_mkpqueue2.h
> -- Installing: /tmp/gklib/usr/local/include/gk_mkblas.h
> -- Installing: /tmp/gklib/usr/local/include/gk_mksort.h
> ---%<---
> 
> As you can see, it installs only header files, possibly in a
> wrong directory, and a static library. What I believe is not
> correct in the metis-static package you are generating.
> 
> Also, should check what is, if any, the reason to build a
> static libGKlib.a, otherwise, I suggest experimenting with
> a shared one, e.g. untested pseudo patch to
> GKlib/CMakeLists.txt:
> ---%<---
> -add_library(GKlib STATIC ${GKlib_sources})
> -add_library(GKlib SHARED ${GKlib_sources})
> ---%<---
> *But* it looks like the GKlib sources are already added
> to libmetis.so; a quick check on a random symbol...
> ---%<---
> $ objdump -T build/Linux-x86_64/libmetis/libmetis.so| grep
> gk_find_frequent_itemsets
> 000000000001dfa0 g    DF .text  0000000000000208  Base       
> gk_find_frequent_itemsets
> $ objdump -t GKlib/build/Linux-x86_64/libGKlib.a| grep
> gk_find_frequent_itemsets
> 00000000000003e0 g     F .text  0000000000000208 gk_find_frequent_itemsets
> ---%<---
> so, they are indeed duplicated, and if anything, I think
> should only install GKlib headers, but are those really
> supposed to be exported, instead of only stuff in metis.h?

I've added two patches but they seem not work.

Comment 3 Antonio T. (sagitter) 2013-03-15 17:12:48 UTC
Spec URL: http://sagitter.fedorapeople.org/metis/metis.spec
SRPM URL: http://sagitter.fedorapeople.org/metis/metis-5.0.3-3.fc18.src.rpm

Now the patches should be better.
Static sub-package is removed.

I'm working to include %check section properly.

Comment 4 Paulo Andrade 2013-03-16 03:36:31 UTC
Another pass on looking at the package, for a pre
review request :-)

First, I have a feeling GKlib is not supposed to be installed,
well, the .c files are compiled and added to libmetis.so, so
anything linking to libmetis and libGKlib will fail due to
duplicated symbols. And upstream may not want the header files
installed to not have a compromise with an API/ABI.

1. Use %{name}-*.patch, example pseudo patch:
-Patch0:  metis-libmetis.patch
+Patch0:  %{name}-libmetis.patch
Latest rawhide fedora-review does not issue an error,
but previous versions would, so should be a good
practice to do it.

2. I think using version as soname may not be
   a good idea, example pseudo patch:
-set_target_properties(metis PROPERTIES SOVERSION 5.0.3)
+set_target_properties(metis PROPERTIES SOVERSION 0)

3. I think you edited metis-width-datatype.patch :-)
  This looks odd:
---%<---
@@ -40,7 +40,7 @@
    32 : single precission floating point (float)
    64 : double precission floating point (double)
 --------------------------------------------------------------------------*/
-#define REALTYPEWIDTH 32
+#define REALTYPEWIDTH 32


---%<---
I had suggested defining it to 64 to use a double
precision value, but that depends on if the precision
is really required, that is, usually 32 bit float is
only meant for speed with very low compromise with
precision.


4. This is working by accident because the variables
   are being ignored and using CFLAGS and CXXFLAGS, but
   you should quote %{optflags} in the spec if using it:
        -DCMAKE_CXX_FLAGS=%{optflags} -DCMAKE_C_FLAGS_RELEASE=%{optflags} \


5. There is a missing Requires in the -devel package,
   should have:
Requires:	%{name}%{?_isa} = %{version}-%{release}


6. Should not use x86_64 to detect 64 bit, instead use,
   something like the pseudo patch:
-%ifarch x86_64
+if [ %{__isa_bits} = "64" ]; then
+%patch2 -p1
+fi
-%endif


7. My suggestion to invoke cmake directly was due
   to no other way to set some options, but you should
   follow the pattern in the toplevel Makefile, that has:
---%<---
CONFIG_FLAGS = -DCMAKE_VERBOSE_MAKEFILE=1
ifeq ($(gklib_path), not-set)
    gklib_path = GKlib
endif
CONFIG_FLAGS += -DGKLIB_PATH=$(abspath $(gklib_path))
ifneq ($(gdb), not-set)
    CONFIG_FLAGS += -DGDB=$(gdb)
endif
ifneq ($(assert), not-set)
    CONFIG_FLAGS += -DASSERT=$(assert)
endif
ifneq ($(assert2), not-set)
    CONFIG_FLAGS += -DASSERT2=$(assert2)
endif
ifneq ($(debug), not-set)
    CONFIG_FLAGS += -DDEBUG=$(debug)
endif
ifneq ($(gprof), not-set)
    CONFIG_FLAGS += -DGPROF=$(gprof)
endif
ifneq ($(openmp), not-set)
    CONFIG_FLAGS += -DOPENMP=$(openmp)
endif
ifneq ($(prefix), not-set)
    CONFIG_FLAGS += -DCMAKE_INSTALL_PREFIX=$(prefix)
endif
ifneq ($(shared), not-set)
    CONFIG_FLAGS += -DSHARED=1
endif
ifneq ($(cc), not-set)
    CONFIG_FLAGS += -DCMAKE_C_COMPILER=$(cc)
endif
---%<---
Actually, it may be better to add a libsuffix extra
option to the Makefile instead of invoking cmake.

8. There is a missing/wrong check for openmp. It does
   not find an FindOpenMP cmake config file. Probably
   it is searching for this:
http://www.openmesh.org/svnrepo/OpenMesh/trunk/cmake/FindOpenMP.cmake
So, you may want to experiment with defining OPENMP and/or __OPENMP__
and having -fopenmp in CFLAGS and CXXFLAGS.

9. PCRE is also being disabled by default. Should at
least investigate it.

10. Instead of moving files after install of GKlib you may
want to experiment with patching GKlib/CMakeLists.txt, untested:
-install(FILES ${GKlib_includes} DESTINATION include)
-install(FILES ${GKlib_includes} DESTINATION include/metis)

11. I suggest adding -pthread to CFLAGS and CXXFLAGS, but
when adding a %check section any issues should be made
visible. This is because of the weird message when running
the test for __thread, but GKlib sources declare __thread
variables.

12. Binaries are being installed with 775 permission. Check
it, should be 755.

13. Consider running help2man or manually generating manpages
for the installed binaries.

Comment 5 Antonio T. (sagitter) 2013-03-16 14:26:38 UTC
(In reply to comment #4)
> 
> 
> 7. My suggestion to invoke cmake directly was due
>    to no other way to set some options, but you should
>    follow the pattern in the toplevel Makefile, that has:
> ---%<---
> CONFIG_FLAGS = -DCMAKE_VERBOSE_MAKEFILE=1
> ifeq ($(gklib_path), not-set)
>     gklib_path = GKlib
> endif
> CONFIG_FLAGS += -DGKLIB_PATH=$(abspath $(gklib_path))
> ifneq ($(gdb), not-set)
>     CONFIG_FLAGS += -DGDB=$(gdb)
> endif
> ifneq ($(assert), not-set)
>     CONFIG_FLAGS += -DASSERT=$(assert)
> endif
> ifneq ($(assert2), not-set)
>     CONFIG_FLAGS += -DASSERT2=$(assert2)
> endif
> ifneq ($(debug), not-set)
>     CONFIG_FLAGS += -DDEBUG=$(debug)
> endif
> ifneq ($(gprof), not-set)
>     CONFIG_FLAGS += -DGPROF=$(gprof)
> endif
> ifneq ($(openmp), not-set)
>     CONFIG_FLAGS += -DOPENMP=$(openmp)
> endif
> ifneq ($(prefix), not-set)
>     CONFIG_FLAGS += -DCMAKE_INSTALL_PREFIX=$(prefix)
> endif
> ifneq ($(shared), not-set)
>     CONFIG_FLAGS += -DSHARED=1
> endif
> ifneq ($(cc), not-set)
>     CONFIG_FLAGS += -DCMAKE_C_COMPILER=$(cc)
> endif
> ---%<---
> Actually, it may be better to add a libsuffix extra
> option to the Makefile instead of invoking cmake.
> 

I don't understand what you mean here.

Comment 6 Paulo Andrade 2013-03-16 15:46:29 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > 
> > 
> > 7. My suggestion to invoke cmake directly was due
> >    to no other way to set some options, but you should
> >    follow the pattern in the toplevel Makefile, that has:
> > ---%<---
> > CONFIG_FLAGS = -DCMAKE_VERBOSE_MAKEFILE=1
> > ifeq ($(gklib_path), not-set)
> >     gklib_path = GKlib
> > endif
[...]
> > ---%<---
> > Actually, it may be better to add a libsuffix extra
> > option to the Makefile instead of invoking cmake.
> > 
> 
> I don't understand what you mean here.

It was a suggestion to add one (or more) extra option(s)
appended to CONFIG_FLAGS, because the toplevel Makefile
calls cmake aready, in this chunk:

mkdir -p $(BUILDDIR)
cd $(BUILDDIR) && cmake $(CURDIR) $(CONFIG_FLAGS)

Untested patch to add to the toplevel Makefile
would be:
 libsuffix = not-set
[...]
 ifneq ($(cc), not-set)
     CONFIG_FLAGS += -DCMAKE_C_COMPILER=$(cc)
 endif
 ifneq ($(libsuffix), not-set)
     CONFIG_FLAGS += -DLIB_SUFFIX=$(libsuffix)
 endif

and then invoke the toplevel Makefile with the
extra option:
make ... libsuffix=64
if on a 64 bit arch.

Comment 7 Antonio T. (sagitter) 2013-03-17 19:03:54 UTC
Spec URL: http://sagitter.fedorapeople.org/metis/metis.spec
SRPM URL: http://sagitter.fedorapeople.org/metis/metis-5.0.3-4.fc18.src.rpm

>--------------------------------------------------
>5. You should not need to install the graphs subdir to
>   %{_datadir}/%{name}/graphs, but instead, use them, as
>   they are actually example input files, in a %check
>   section. Also, should add GKlib/test binaries to a
>   %check run.

Onestly I don't see any GKlib/test binaries. :)


>12. Binaries are being installed with 775 permission. Check
>it, should be 755.

They seem me all with 755:

$ ll /usr/bin | grep metis
-rwxr-xr-x.   1 root root       43024 Mar 17 19:55 gpmetis
-rwxr-xr-x.   1 root root       31040 Mar 17 19:55 m2gmetis
-rwxr-xr-x.   1 root root       43152 Mar 17 19:55 mpmetis
-rwxr-xr-x.   1 root root       40624 Mar 17 19:55 ndmetis

Comment 8 Paulo Andrade 2013-03-17 19:14:46 UTC
(In reply to comment #7)
> Spec URL: http://sagitter.fedorapeople.org/metis/metis.spec
> SRPM URL: http://sagitter.fedorapeople.org/metis/metis-5.0.3-4.fc18.src.rpm
> 
> >--------------------------------------------------
> >5. You should not need to install the graphs subdir to
> >   %{_datadir}/%{name}/graphs, but instead, use them, as
> >   they are actually example input files, in a %check
> >   section. Also, should add GKlib/test binaries to a
> >   %check run.
> 
> Onestly I don't see any GKlib/test binaries. :)

If you run make in the GKlib directory it creates test binaries:
$ pwd
/home/pcpa/rpmbuild/BUILD/metis-5.0.3/GKlib
$ ls test/*.c
test/fis.c  test/gkgraph.c  test/gksort.c  test/rw.c  test/strings.c
$ ls build/test
CMakeFiles  cmake_install.cmake  fis  gkgraph  gksort  Makefile  rw  strings
$ ldd build/test/fis
        linux-vdso.so.1 =>  (0x00007fff907fe000)
        libGKlib.so.5.0.3 => /home/pcpa/rpmbuild/BUILD/metis-5.0.3/GKlib/build/libGKlib.so.5.0.3 (0x00007f85e6521000)
        libm.so.6 => /lib64/libm.so.6 (0x00000035e3400000)
        libc.so.6 => /lib64/libc.so.6 (0x00000035e3000000)
        /lib64/ld-linux-x86-64.so.2 (0x00000035e2c00000)

> >12. Binaries are being installed with 775 permission. Check
> >it, should be 755.
> 
> They seem me all with 755:
> 
> $ ll /usr/bin | grep metis
> -rwxr-xr-x.   1 root root       43024 Mar 17 19:55 gpmetis
> -rwxr-xr-x.   1 root root       31040 Mar 17 19:55 m2gmetis
> -rwxr-xr-x.   1 root root       43152 Mar 17 19:55 mpmetis
> -rwxr-xr-x.   1 root root       40624 Mar 17 19:55 ndmetis

Try running fedora-review, tail of review.txt:
Generated by fedora-review 0.4.0 (660ce56) last change: 2013-01-29
Buildroot used: fedora-rawhide-x86_64
Command line :/usr/bin/fedora-review -r -v -n metis

and review.txt says:
Rpmlint (installed packages)
----------------------------
# rpmlint metis metis-devel
metis.x86_64: W: spelling-error %description -l en_US multi -> mulch, mufti
metis.x86_64: E: non-standard-executable-perm /usr/bin/cmpfillin 0775L
metis.x86_64: E: non-standard-executable-perm /usr/bin/mpmetis 0775L
metis.x86_64: E: non-standard-executable-perm /usr/lib64/metis/libmetis.so 0775L
metis.x86_64: E: non-standard-executable-perm /usr/bin/m2gmetis 0775L
metis.x86_64: E: non-standard-executable-perm /usr/bin/ndmetis 0775L
metis.x86_64: E: non-standard-executable-perm /usr/bin/graphchk 0775L
metis.x86_64: E: non-standard-executable-perm /usr/bin/gpmetis 0775L
metis.x86_64: W: no-manual-page-for-binary cmpfillin
metis.x86_64: W: no-manual-page-for-binary gpmetis
metis.x86_64: W: no-manual-page-for-binary graphchk
metis.x86_64: W: no-manual-page-for-binary mpmetis
metis.x86_64: W: no-manual-page-for-binary ndmetis
metis.x86_64: W: no-manual-page-for-binary m2gmetis
2 packages and 0 specfiles checked; 7 errors, 7 warnings.
# echo 'rpmlint-done:'

If I recall correctly, fedora-review (or mock?) on purpose
use a non standard umask to trigger cases of builds relying
on a default umask.

Comment 9 Antonio T. (sagitter) 2013-03-18 18:24:59 UTC
(In reply to comment #8)
> 
> If you run make in the GKlib directory it creates test binaries:
> $ pwd
> /home/pcpa/rpmbuild/BUILD/metis-5.0.3/GKlib
> $ ls test/*.c
> test/fis.c  test/gkgraph.c  test/gksort.c  test/rw.c  test/strings.c
> $ ls build/test
> CMakeFiles  cmake_install.cmake  fis  gkgraph  gksort  Makefile  rw  strings
> $ ldd build/test/fis
>         linux-vdso.so.1 =>  (0x00007fff907fe000)
>         libGKlib.so.5.0.3 =>
> /home/pcpa/rpmbuild/BUILD/metis-5.0.3/GKlib/build/libGKlib.so.5.0.3
> (0x00007f85e6521000)
>         libm.so.6 => /lib64/libm.so.6 (0x00000035e3400000)
>         libc.so.6 => /lib64/libc.so.6 (0x00000035e3000000)
>         /lib64/ld-linux-x86-64.so.2 (0x00000035e2c00000)

These binaries seem be used exclusively by upstream (as developer confirmed to me); also they could lead to false positives in future.

Maybe we can ignore them. :)

Comment 10 Paulo Andrade 2013-03-18 18:33:35 UTC
(In reply to comment #9)
> > $ ldd build/test/fis
> >         linux-vdso.so.1 =>  (0x00007fff907fe000)
> >         libGKlib.so.5.0.3 =>
> > /home/pcpa/rpmbuild/BUILD/metis-5.0.3/GKlib/build/libGKlib.so.5.0.3
> > (0x00007f85e6521000)
> >         libm.so.6 => /lib64/libm.so.6 (0x00000035e3400000)
> >         libc.so.6 => /lib64/libc.so.6 (0x00000035e3000000)
> >         /lib64/ld-linux-x86-64.so.2 (0x00000035e2c00000)
> 
> These binaries seem be used exclusively by upstream (as developer confirmed
> to me); also they could lead to false positives in future.

This comes back to my comment in #4:
---%<---
First, I have a feeling GKlib is not supposed to be installed,
well, the .c files are compiled and added to libmetis.so, so
anything linking to libmetis and libGKlib will fail due to
duplicated symbols. And upstream may not want the header files
installed to not have a compromise with an API/ABI.
---%<---

> Maybe we can ignore them. :)

Can you confirm the GKlib headers are not supposed to be installed?
Note that this still does not mean running the test binaries in
%check is not required :-) They are supposed to work in the build
environment, and would make it easier to validate that the package
works as intended.

Comment 11 Antonio T. (sagitter) 2013-03-18 19:26:50 UTC
(In reply to comment #10)
> 
> Can you confirm the GKlib headers are not supposed to be installed?
> Note that this still does not mean running the test binaries in
> %check is not required :-) 

Confirmed. You are right. 
So, if I understood fine, all gk*.h files can be not included on the package.

Comment 12 Antonio T. (sagitter) 2013-03-19 18:09:35 UTC
Added %check section and removed GKlib installation.

Spec URL: http://sagitter.fedorapeople.org/metis/metis.spec
SRPM URL: http://sagitter.fedorapeople.org/metis/metis-5.0.3-5.fc18.src.rpm

Comment 13 Paulo Andrade 2013-03-20 01:41:15 UTC
Appears good looking in the spec, but when trying
fedora-review, it fails in %check because the binaries
are not in $PATH:

+ cd /builddir/build/BUILD
~/build/BUILD/metis-5.0.3/graphs ~/build/BUILD/metis-5.0.3
+ cd metis-5.0.3
+ pushd graphs
+ ndmetis mdual.graph
/var/tmp/rpm-tmp.lerKCw: line 30: ndmetis: command not found

Comment 14 Antonio T. (sagitter) 2013-03-20 17:09:50 UTC
(In reply to comment #13)
> Appears good looking in the spec, but when trying
> fedora-review, it fails in %check because the binaries
> are not in $PATH:
> 
> + cd /builddir/build/BUILD
> ~/build/BUILD/metis-5.0.3/graphs ~/build/BUILD/metis-5.0.3
> + cd metis-5.0.3
> + pushd graphs
> + ndmetis mdual.graph
> /var/tmp/rpm-tmp.lerKCw: line 30: ndmetis: command not found

Not for me.(In reply to comment #13)
> Appears good looking in the spec, but when trying
> fedora-review, it fails in %check because the binaries
> are not in $PATH:
> 
> + cd /builddir/build/BUILD
> ~/build/BUILD/metis-5.0.3/graphs ~/build/BUILD/metis-5.0.3
> + cd metis-5.0.3
> + pushd graphs
> + ndmetis mdual.graph
> /var/tmp/rpm-tmp.lerKCw: line 30: ndmetis: command not found

Not for me.
I show you the build.log from my fedora-review output.

Comment 15 Antonio T. (sagitter) 2013-03-20 17:11:37 UTC
Created attachment 713341 [details]
build.log from fedora-reviw results directory

Comment 16 Paulo Andrade 2013-03-20 17:18:11 UTC
Probably you have it installed in the mock chroot.
Try "mock --clean" and run fedora-review again.

Comment 17 Antonio T. (sagitter) 2013-03-20 18:52:54 UTC
Spec URL: http://sagitter.fedorapeople.org/metis/metis.spec
SRPM URL: http://sagitter.fedorapeople.org/metis/metis-5.0.3-6.fc18.src.rpm

It should be functioning.

Comment 18 Paulo Andrade 2013-03-20 19:30:15 UTC
Package Review
==============

Key:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- Permissions on files are set properly.
  Note: See rpmlint output
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions
- Spec file lacks Packager, Vendor, PreReq tags.
  Note: Found : Packager: pcpa <paulo.cesar.pereira.de.andrade>
  See: (this test has no URL)


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package complies to the Packaging Guidelines
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "BSD", "BSD (2 clause)", "LGPL (v2.1 or later) (with incorrect FSF
     address)", "Unknown or generated". 4 files have unknown license. Detailed
     output of licensecheck in /home/pcpa/920518-metis/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 419840 bytes in 4 files.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Fully versioned dependency in subpackages, if present.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

===== SHOULD items =====

Generic:
[x]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: Scriptlets must be sane, if used.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: metis-5.0.3-6.fc19.x86_64.rpm
          metis-devel-5.0.3-6.fc19.x86_64.rpm
metis.x86_64: W: spelling-error %description -l en_US multi -> mulch, mufti
metis.x86_64: W: shared-lib-calls-exit /usr/lib64/libmetis.so.0 exit.5
2 packages and 0 specfiles checked; 0 errors, 2 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint metis metis-devel
metis.x86_64: W: spelling-error %description -l en_US multi -> mulch, mufti
metis.x86_64: E: non-standard-executable-perm /usr/bin/cmpfillin 0775L
metis.x86_64: E: non-standard-executable-perm /usr/bin/mpmetis 0775L
metis.x86_64: E: non-standard-executable-perm /usr/lib64/metis/libmetis.so 0775L
metis.x86_64: E: non-standard-executable-perm /usr/bin/m2gmetis 0775L
metis.x86_64: E: non-standard-executable-perm /usr/bin/ndmetis 0775L
metis.x86_64: E: non-standard-executable-perm /usr/bin/graphchk 0775L
metis.x86_64: E: non-standard-executable-perm /usr/bin/gpmetis 0775L
metis.x86_64: W: no-manual-page-for-binary cmpfillin
metis.x86_64: W: no-manual-page-for-binary gpmetis
metis.x86_64: W: no-manual-page-for-binary graphchk
metis.x86_64: W: no-manual-page-for-binary mpmetis
metis.x86_64: W: no-manual-page-for-binary ndmetis
metis.x86_64: W: no-manual-page-for-binary m2gmetis
2 packages and 0 specfiles checked; 7 errors, 7 warnings.
# echo 'rpmlint-done:'



Requires
--------
metis (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    ld-linux-x86-64.so.2()(64bit)
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.3.1)(64bit)
    libgomp.so.1()(64bit)
    libgomp.so.1(GOMP_1.0)(64bit)
    libgomp.so.1(OMP_1.0)(64bit)
    libm.so.6()(64bit)
    libmetis.so.0()(64bit)
    libpthread.so.0()(64bit)
    rtld(GNU_HASH)

metis-devel (rpmlib, GLIBC filtered):
    libmetis.so.0()(64bit)
    metis(x86-64)



Provides
--------
metis:
    libmetis.so.0()(64bit)
    metis
    metis(x86-64)

metis-devel:
    metis-devel
    metis-devel(x86-64)



MD5-sum check
-------------
http://glaros.dtc.umn.edu/gkhome/fetch/sw/metis/metis-5.0.3.tar.gz :
  CHECKSUM(SHA256) this package     : 38e57e36baada41ddf2d69e0b0070aa47b5f4a72776f9677d1d0575b56ec67a6
  CHECKSUM(SHA256) upstream package : 38e57e36baada41ddf2d69e0b0070aa47b5f4a72776f9677d1d0575b56ec67a6


Generated by fedora-review 0.4.0 (660ce56) last change: 2013-01-29
Buildroot used: fedora-rawhide-x86_64
Command line :/usr/bin/fedora-review -v -b 920518

Comment 19 Paulo Andrade 2013-03-20 19:34:06 UTC
The permissions warning appears bogus, and should
be caused by not installing files owned by root:

$ rpm2cpio metis-5.0.3-6.fc19.x86_64.rpm |cpio -id
2077 blocks
$ ll usr/bin/ndmetis
-rwxr-xr-x 1 pcpa pcpa 40648 Mar 20 16:25 usr/bin/ndmetis

The missing manual page warning is due to explicitly
compressing them. Should not do it, just copy the
manual pages to the proper directory and let rpmbuild
compress them. Also, change this:

mkdir %{buildroot}%{_datadir}
mkdir %{buildroot}%{_mandir} ; mkdir %{buildroot}%{_mandir}/man1

into

mkdir -p %{buildroot}%{_mandir}/man1

Comment 20 Paulo Andrade 2013-03-20 19:36:18 UTC
It is up to you if wish to make a new package for review with
the man page correction, otherwise I see no other problems,
just make sure to add the correction to the final package.

I consider the package APPROVED.


BTW, I see you did not yet populate the MUMPS git tree:
http://pkgs.fedoraproject.org/cgit/MUMPS.git/
Do you need some help with it?

Comment 21 Antonio T. (sagitter) 2013-03-20 20:12:55 UTC
(In reply to comment #20)
> It is up to you if wish to make a new package for review with
> the man page correction, otherwise I see no other problems,
> just make sure to add the correction to the final package.

I still have some doubts about license (see comment#0).

> 
> I consider the package APPROVED.

Okay.

Comment 22 Paulo Andrade 2013-03-20 20:36:25 UTC
You should write it as

License: ASL 2.0 and BSD and LGPLv2+

I did look at gkregex GKlib headers and sources
earlier in the review, and they are not built;
it uses system regex (regcomp/regexec), that
in Linux just happens to be updated versions
of those GKlib files.

It does not use random.c by default either.

But, it "compiles" all the files, just that
does not generate any code, unless some
preprocessor symbol is defined, for example
USE_GKREGEX to use LGPL regex.

But it actually builds a few, like getopt.c,
it just renames the symbols adding a gk_ prefix.
It also uses glibc qsort macros to implement
several specialized quick sort routines.

Comment 23 Antonio T. (sagitter) 2013-03-20 21:11:48 UTC
(In reply to comment #22)
> You should write it as
> 
> License: ASL 2.0 and BSD and LGPLv2+
> 

Fixed. 
Thank you.

Spec URL: http://sagitter.fedorapeople.org/metis/metis.spec
SRPM URL: http://sagitter.fedorapeople.org/metis/metis-5.0.3-7.fc18.src.rpm

Comment 24 Antonio T. (sagitter) 2013-03-20 21:22:51 UTC
New Package SCM Request
=======================
Package Name: metis
Short Description: Serial Graph Partitioning and Fill-reducing Matrix Ordering
Owners: sagitter
Branches: f17 f18 f19 el6

Comment 25 Gwyn Ciesla 2013-03-21 12:18:45 UTC
Git done (by process-git-requests).

Comment 26 Fedora Update System 2013-03-24 22:55:03 UTC
metis-5.0.3-9.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/metis-5.0.3-9.fc17

Comment 27 Fedora Update System 2013-03-24 22:55:17 UTC
metis-5.0.3-9.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/metis-5.0.3-9.el6

Comment 28 Fedora Update System 2013-03-24 22:55:28 UTC
metis-5.0.3-9.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/metis-5.0.3-9.fc18

Comment 29 Fedora Update System 2013-03-31 20:39:15 UTC
metis-5.0.3-10.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/metis-5.0.3-10.fc18

Comment 30 Fedora Update System 2013-03-31 20:39:29 UTC
metis-5.0.3-10.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/metis-5.0.3-10.el6

Comment 31 Fedora Update System 2013-03-31 20:39:39 UTC
metis-5.0.3-10.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/metis-5.0.3-10.fc17

Comment 32 Fedora Update System 2013-04-10 01:34:21 UTC
metis-5.0.3-10.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 33 Fedora Update System 2013-04-10 01:38:01 UTC
metis-5.0.3-10.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 34 Fedora Update System 2013-04-14 19:20:28 UTC
metis-5.1.0-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/metis-5.1.0-1.fc17

Comment 35 Fedora Update System 2013-04-14 19:20:43 UTC
metis-5.1.0-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/metis-5.1.0-1.fc18

Comment 36 Fedora Update System 2013-04-14 19:20:54 UTC
metis-5.1.0-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/metis-5.1.0-1.el6


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