Bug 226109 - Merge Review: ltrace
Merge Review: ltrace
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Miroslav Lichvar
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 14:35 EST by Nobody's working on this, feel free to take it
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version: F-8
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-10-08 14:47:14 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mlichvar: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 14:35:47 EST
Fedora Merge Review: ltrace

http://cvs.fedora.redhat.com/viewcvs/devel/ltrace/
Initial Owner: pmachata@redhat.com
Comment 1 Petr Machata 2007-02-07 13:08:22 EST
Tidied up version commited, not built.
rpmlint is silent, for both source and binary rpm.
Comment 2 Miroslav Lichvar 2007-09-05 08:35:16 EDT
I'll post some comments shortly.
Comment 3 Miroslav Lichvar 2007-09-05 09:28:55 EDT
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
Comment 4 Miroslav Lichvar 2007-09-05 11:29:03 EDT
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.
Comment 5 Petr Machata 2007-09-12 13:55:37 EDT
(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.
Comment 6 Miroslav Lichvar 2007-10-05 08:24:16 EDT
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.
Comment 7 Petr Machata 2007-10-08 14:47:14 EDT
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!
Comment 8 Miroslav Lichvar 2007-10-09 03:36:05 EDT
Ok, approved.

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