Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=464424 --- Comment #7 from Dominik 'Rathann' Mierzejewski <rpm@xxxxxxxxxxxxxx> 2008-09-29 19:18:24 EDT --- Full review below. OK'd items omitted. - MUST: rpmlint must be run on every package. The output should be posted in the review. NEEDSFIX $ rpmlint /var/lib/mock//fedora-rawhide-i386/result gromacs-mpi.i386: W: no-documentation gromacs.src: W: mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 1) gromacs.i386: W: no-documentation gromacs.i386: E: no-binary gromacs-double.i386: W: no-documentation gromacs-mpi-double.i386: W: no-documentation gromacs-common.i386: E: zero-length /usr/share/gromacs/template/Makefile.i386-redhat-linux-gnu_double gromacs-common.i386: W: devel-file-in-non-devel-package /usr/share/gromacs/template/template.c gromacs-common.i386: E: zero-length /usr/share/gromacs/template/Makefile.i386-redhat-linux-gnu gromacs-single.i386: W: no-documentation gromacs-devel.i386: W: no-documentation gromacs-mpi-single.i386: W: no-documentation 10 packages and 0 specfiles checked; 3 errors, 9 warnings. Please fix the spaces-and-tabs warning and the zero-length files. - MUST: The package must meet the Packaging Guidelines. NEEDSFIX Please rename the specfile to have the same name as the main package, i.e. gromacs.spec. Either make four devel subpackages, one for each set of libraries or group them together, for example single+double -> gromacs mpi single+double -> gromacs-mpi and then only two -devel packages You should also put libraries into a separate package to allow for multilib installs. autoreconf Is it necessary? Calling autoreconf makes the build fail on rawhide/i386. If you remove that, remove BR: autoconf/automake, too. ../configure --host=%{_host} --build=%{_build} \ --target=%{_target_platform} \ --program-prefix=%{?_program_prefix} \ --prefix=%{_prefix} \ --exec-prefix=%{_exec_prefix} \ --bindir=%{_bindir} \ --sbindir=%{_sbindir} \ --sysconfdir=%{_sysconfdir} \ --datadir=%{_datadir} \ --includedir=%{_includedir} \ --libdir=%{_libdir} \ --libexecdir=%{_libexecdir} \ --localstatedir=%{_localstatedir} \ --sharedstatedir=%{_sharedstatedir} \ --mandir=%{_mandir} \ --infodir=%{_infodir} \ --disable-rpath --enable-shared \ --disable-static --enable-float \ --with-gsl --with-x Maybe add --enable-pthreads? Or a separate pthread-enabled version? Not a blocker, just FYI. You must (if possible) make gromacs use existing Fedora packages (lapack, blas) instead of internal copies. It has --with-external-blas and --with-external-lapack options in configure. See http://fedoraproject.org/wiki/Packaging/Guidelines#Duplication_of_system_libraries # Compress manpages cd %{buildroot}/%{_mandir}/man1 for i in *.1; do gzip $i done No need, this is done automatically by rpmbuild. # Remove .la files \rm %{buildroot}/%{_libdir}/*.la Stray backslash? %_includedir/gromacs -> %{_includedir}/gromacs %files common ... %doc AUTHORS COPYING INSTALL README gromacs-4.0.pdf completion.bash completion.zsh completion.csh. Trailing whitespace, please remove. Also, please put %doc files right after %defattr. It is rather pointless to include the INSTALL file. completion.bash could be put in %{_sysconfdir}/bash_completion.d/ (needs Requires: bash-completion then, maybe a separate package bash-completion-gromacs?). Check for similar possibilities for other shells. This would help loosen the dependencies on various shells. As it is, -common depends on all of them: Requires: /bin/bash /bin/csh /bin/sh /bin/zsh /usr/bin/perl This is not reasonable. At least the csh and zsh dependencies must be removed. %files mpi Empty? - MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines . NEEDSFIX Some files (especially src/gmxlib/gmx_{lapack,blas}/*) have no licensing information, although they look like private copies of lapack and blas, so please verify that. If so, their license is BSD, which is GPLv2+ compatible. Other files with no licensing information: ./src/kernel/gmxcpp.h: *No copyright* UNKNOWN ./src/mdlib/._ebin.c: *No copyright* UNKNOWN ./src/mdlib/perf_est.c: *No copyright* UNKNOWN ./src/mdlib/wall.c: *No copyright* UNKNOWN ./src/mdlib/shellfc.c: *No copyright* UNKNOWN ./src/mdlib/gmx_wallcycle.c: *No copyright* UNKNOWN ./src/gmxlib/nonbonded/nb_free_energy.h: *No copyright* UNKNOWN ./src/gmxlib/nonbonded/nb_kernel_ia64_single/ia64_cpuid.h: *No copyright* UNKNOWN ./src/gmxlib/nonbonded/nb_generic.h: *No copyright* UNKNOWN ./src/gmxlib/nonbonded/nb_kernel_ia64_double/ia64_cpuid.h: *No copyright* UNKNOWN ./src/gmxlib/topsort.c: *No copyright* UNKNOWN ./src/tools/correl.c: *No copyright* UNKNOWN ./src/tools/correl.h: *No copyright* UNKNOWN ./include/mpelogging.h: *No copyright* UNKNOWN ./include/gmx_ana.h: *No copyright* UNKNOWN ./include/gmx_arpack.h: UNKNOWN ./include/shellfc.h: *No copyright* UNKNOWN ./include/perf_est.h: *No copyright* UNKNOWN ./include/types/qmmmrec.h: *No copyright* UNKNOWN ./include/types/ns.h: *No copyright* UNKNOWN ./include/topsort.h: *No copyright* UNKNOWN ./include/gmx_wallcycle.h: *No copyright* UNKNOWN ./include/qmmm.h: *No copyright* UNKNOWN ./include/gmx_lapack.h: *No copyright* UNKNOWN ./include/gmx_blas.h: *No copyright* UNKNOWN ./include/gmx_random.h: *No copyright* UNKNOWN Please ask upstream to add appropriate license headers to these files. - MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this. NEEDSFIX Bad source URL: Source0:<------>ftp://ftp.gromacs.org/pub/gromacs/source/gromacs-4.0_rc2.tar.gz gromacs/source -> beta - MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. This is described in detail in the desktop files section of the Packaging Guidelines . If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. NEEDSFIX(?) I see libX11 in rpm-generated Requires:. Does this package display a GUI? If so, it needs an appropriate desktop file and desktop-file-install call (BR: desktop-file-utils). SHOULD Items: - SHOULD: The reviewer should test that the package builds in mock. See MockTricks for details on how to do this. Doesn't build in mock/devel/i386 due to autoreconf failure. Builds fine in koji/dist-f10 (with autoreconf invocation removed). http://koji.fedoraproject.org/koji/taskinfo?taskID=851228 - SHOULD: The package should compile and build into binary rpms on all supported architectures. - SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example. I haven't actually tested it, but I think you must use --disable-ia32-* for i386 builds, otherwise gromacs will crash with SIGILL when run on a CPU without 3dnow or sse instructions. You can also build separate versions of gromacs libraries with these enabled and put them in %{_libdir}/3dnow and %{_libdir}/sse, respectively. The linker will then select the best version automatically (see atlas package for inspiration). Similarly --disable-ppc-altivec for ppc builds (not all ppc have altivec), but you can build separate altivec-enabled libraries and put them in %{_libdir}/altivec/ -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review