Fedora Merge Review: ltrace http://cvs.fedora.redhat.com/viewcvs/devel/ltrace/ Initial Owner: pmachata
Tidied up version commited, not built. rpmlint is silent, for both source and binary rpm.
I'll post some comments shortly.
The package looks ok, there are just few easy to fix issues. - rpmlint is silent - the package is named according to the Package Naming Guidelines - the spec file name matches the base package %{name} X the package must meet the Packaging Guidelines - please add %{?_smp_mflags} to the make command - the line before %configure doesn't seem to be necessary. If it is, please add a comment. Also would be nice to have a comment for the relro linker flag - the package has a test suite, please enable it in %check or add a comment if there is a reason why it's disabled - the package is licensed with a Fedora approved license (GPLv2+) - the License field in the package spec file matches the actual license - the file containing the text of the license(s) for the package is included in %doc - the spec file is written in American English - the spec file for the package is legible X source URL isn't specified in this package - upstream doesn't seem to provide tarball for 0.5, please add a comment to spec how to generate the tarball. Also, the tarball name probably should contain the svn string. If it's a prerelease for 0.5, the release tag for the package should start with 0. We don't want to bump epoch to fix this, so it's better to keep it as it is. - all build dependencies are listed in BuildRequires - the package owns all directories that it creates - the package does not contain any duplicate files in the %files listing - permissions on files are set properly - the package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT X the package must consistently use macros - in %files section should be %{_bindir}/ltrace, without %{_prefix} - %{_sysconfdir}/ltrace.conf instead of /etc/ - the package contains code, or permissible content - files included as %doc don't affect the runtime of the application - the package does not own files or directories already owned by other packages - at the beginning of %install, the package runs rm -rf $RPM_BUILD_ROOT - all filenames in rpm packages are valid UTF-8 - the package builds in mock - the package compiles and builds into binary rpms on all supported architectures - the package functions as described
Another thing that should be fixed is the make install line. mandir doesn't need to be specified, but bindir does as it's not set by configure script.
(In reply to comment #3) > X the package must meet the Packaging Guidelines > - please add %{?_smp_mflags} to the make command > - the line before %configure doesn't seem to be necessary. If it is, please > add a comment. Also would be nice to have a comment for the relro linker flag relro flag is not really useful the line fetching -m32/-m64 from RPM_OPT_FLAGS is necessary because the build machinery doesn't honor CFLAGS/LDFLAGS everywhere, and the compiler then may try to mix 32bit and 64bit objects. It doesn't happen in koji, but it may happen on the machine in the wild. (And actually happens of the one I have in hand.) I think there are some testsuite failures ATM. I'll look at these and see if I can fix them, but I seem to recall they were related to building in koji. Since there is no mechanism to grab a build root and experiment inside, it's quite hard to debug such problems. I'll enable the suite if possible. I commited the rest of recommended cleanups, but didn't build the package.
Any update on the testsuite? Adding a comment that it's currently broken and keeping it disabled is ok with me. CC="%CC" isn't needed for %configure, it will use the environment variable. But that's nitpicking. Everything else looks good.
I've taken a look at this. Testsuite failures are consistent in koji, and non-existent on "real" machines. I blame kernel version koji uses to do its thing, but that's basically a wild guess. I'm disabling the suite while still leaving it handy in spec for development purposes. The package has been built. Thanks for review!
Ok, approved.