Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=920518 --- Comment #4 from Paulo Andrade <paulo.cesar.pereira.de.andrade@xxxxxxxxx> --- 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. -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=odqnOKHGj0&a=cc_unsubscribe _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review