Bug 602574 - Review Request: patchelf - a utility for patching ELF binaries
Summary: Review Request: patchelf - a utility for patching ELF binaries
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Martin Gieseking
QA Contact: Fedora Extras Quality Assurance
: 625491 (view as bug list)
Depends On:
TreeView+ depends on / blocked
Reported: 2010-06-10 08:40 UTC by Jeremy Sanders
Modified: 2010-08-25 21:22 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2010-08-25 21:22:47 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
martin.gieseking: fedora-review+
kevin: fedora-cvs+

Attachments (Terms of Use)

Description Jeremy Sanders 2010-06-10 08:40:43 UTC
Spec URL: http://barmag.net/tmp/patchelf.spec
SRPM URL: http://barmag.net/tmp/patchelf-0.5-2.fc12.src.rpm
Description: PatchELF is a simple utility for modifying existing ELF executables
and libraries.  It can change the dynamic loader ("ELF interpreter")
of executables and change the RPATH of executables and libraries.

Comment 1 Martin Gieseking 2010-06-14 14:37:51 UTC
Hi Jeremy,

here are some initial comments:

- according to file README, the license is GPLv3+, not GPLv3 only

- to simplify things, I suggest to replace the rm and rmdir line with 
  rm -rf %{buildroot}/usr/share/doc/

- remove "%doc" from "%doc %{_mandir}/man1/patchelf.1*"

- don't mix tabs and spaces for indentation

- The tarball contains a couple of tests. It's probably a good idea to run them (with "make check" in a %check section).

$ rpmlint /var/lib/mock/fedora-13-i386/result/*.rpm
patchelf.i686: W: spelling-error %description -l en_US executables -> executable, executable s, executrices
patchelf.src: W: spelling-error %description -l en_US executables -> executable, executable s, executrices
patchelf.src: W: no-buildroot-tag
patchelf.src:10: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 10)
3 packages and 0 specfiles checked; 0 errors, 4 warnings.

The missing buildroot tag can be ignored if you plan to maintain this package for Fedora only. Otherwise, you should add one and also clean the buildroot in %install.

Comment 2 Jeremy Sanders 2010-06-14 19:15:59 UTC
Hi Martin - thanks for the helpful comments. An updated version of the package is here:

Spec URL: http://barmag.net/tmp/patchelf.spec
SRPM URL: http://barmag.net/tmp/patchelf-0.5-3.fc12.src.rpm

 * Now GPLv3+ in spec file
 * Replaced separate rm and rmdir (was worried might delete everything if was wrong) with rm -rf
 * Removed %doc from man page
 * Removed tabs and replaced with spaces
 * Added %check section - seems to pass fine
 * Added buildroot so that it would be easy to put in EPEL

I've sent the man page upstream - it will get included in the next release.

Comment 3 Martin Gieseking 2010-06-15 14:22:57 UTC
Hi Jeremy,

I just found two more things that need some attention:

- The tarball contains a bundled version of elf.h. I suggest to remove it and use the one provided by glibc-headers. It should be picked up automatically if the bundled file is not present.

- The tests fail for ppc/ppc64:

According to the project homepage (http://nixos.org/patchelf.html) patchelf is actually supposed to work on PowerPCs too. You should ask upstream whether this is a test-only issue or a bug in the program code.

Comment 4 Martin Gieseking 2010-08-09 07:00:08 UTC
Jeremy, do you have any news on the above mentioned issues (comment #3)? Are you still interested in this package?

Comment 5 Jeremy Sanders 2010-08-09 09:18:43 UTC
Sorry for the delay. I've fixed the elf.h issue. I've also contacted upstream about the second issue. He wanted some debugging data which I have provided. Still no word from upstream for a several weeks.

Here are the versions that delete elf.h before building:

Comment 6 Martin Gieseking 2010-08-09 10:20:56 UTC
OK, thanks for the feedback. I think we should wait for a comment from the upstream developer before finishing the review. Maybe you can contact him again and ask for a status update. If he doesn't respond, I suggest to exclude the ppc archs for now.

Comment 7 Petr Pisar 2010-08-20 07:12:31 UTC
*** Bug 625491 has been marked as a duplicate of this bug. ***

Comment 8 Jeremy Sanders 2010-08-24 09:46:36 UTC
Upstream don't have the resources to investigate the ppc/ppc64 problems: see http://mail.cs.uu.nl/pipermail/nix-dev/2010-August/005085.html

I've disabled ppc and ppc64 architectures in the spec file. Do we need a separate bug to file this problem? I've put a comment in the spec file pointing to here.

Latest spec file and SRPM are here:

I've made a scratch build here:

Comment 9 Martin Gieseking 2010-08-24 12:11:43 UTC
(In reply to comment #8)
> I've disabled ppc and ppc64 architectures in the spec file. Do we need a
> separate bug to file this problem? I've put a comment in the spec file pointing
> to here.

OK, fine. You don't need to file the ppc issue separately. If possible, it should be fixed upstream in a future version. Maybe you can work together with the developer to find the problem, e.g. by using the ppc test machine [1].

I'm going to do the formal review later today.

[1] http://fedoraproject.org/wiki/Test_Machine_Resources_For_Package_Maintainers

Comment 10 Martin Gieseking 2010-08-24 18:17:28 UTC
Here's the formal review. Since I couldn't find any further issues, the package is ready now. 

After having had another look at the packaging guidelines, I noticed that you should file Bugzilla tickets for the ppc issue (now as the package has been approved). The corresponding bug numbers should then be added to the spec file. See [1] for further details.

[1] https://fedoraproject.org/wiki/Packaging/Guidelines#Architecture_Build_Failures

$ rpmlint /var/lib/mock/fedora-13-x86_64/result/*.rpm
patchelf.src: W: spelling-error %description -l en_US executables -> executable, executable s, executants
patchelf.x86_64: W: spelling-error %description -l en_US executables -> executable, executable s, executants
3 packages and 0 specfiles checked; 0 errors, 2 warnings.

The spelling errors are false positive and can be ignored.


[+] OK
[.] OK, not applicable
[X] needs work

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
[+] MUST: The License field in the package spec file must match the actual license.
[+] MUST: The file containing the text of the license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum patchelf-0.5.tar.bz2*
    c41fc98091d15dc93ba876c3ef11f43c  patchelf-0.5.tar.bz2
    c41fc98091d15dc93ba876c3ef11f43c  patchelf-0.5.tar.bz2.1

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[X] MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture.
    - File two Bugzilla tickets for the ppc and ppc64 issue.
    - Add the corresponding bug numbers to the spec file.

[+] MUST: All build dependencies must be listed in BuildRequires.
[.] MUST: The spec file MUST handle locales properly.
[.] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in %files.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[.] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[.] MUST: If a package contains library files with a suffix ...
[.] MUST: devel packages must require the base package. 
[+] MUST: Packages must NOT contain any .la libtool archives.
[.] MUST: Packages containing GUI applications must include a %{name}.desktop file.
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

[.] SHOULD: If the source package does not include license text(s), ...
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] 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.
[.] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin ...


Comment 11 Jeremy Sanders 2010-08-25 08:55:06 UTC
Thanks for the approval Martin.

Admins: please can you add the GIT repositories for the new package patchelf as it has now been accepted.

Petr Pisar has agreed to be comaintainer of this package.

I will file a new bug to track the PPC/PPC64 issues once we have a bugzilla entry for this package and put it in the spec file.

New Package SCM Request
Package Name: patchelf
Short Description: A utility for patching ELF binaries
Owners: jsanders ppisar
Branches: f12 f13 f14
InitialCC: jsanders ppisar

Comment 12 Kevin Fenzi 2010-08-25 17:31:48 UTC
Git done (by process-git-requests).

Comment 13 Jeremy Sanders 2010-08-25 21:22:47 UTC
Package successfully added to repository, built and queued up in bohdi for updates testing (F12, F13 and F14).

Thanks very much for Martin Gieseking's hard work.

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