Bug 543840 - Review Request: udis86 - A x86 disassembler library
Summary: Review Request: udis86 - A x86 disassembler library
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Peter Lemenkov
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-12-03 09:46 UTC by Scott Tsai
Modified: 2010-07-30 01:06 UTC (History)
4 users (show)

Fixed In Version: udis86-1.7-3.el5
Clone Of:
Environment:
Last Closed: 2010-02-09 05:02:42 UTC
Type: ---
Embargoed:
lemenkov: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)
diff-r1-r2 (2.03 KB, patch)
2009-12-03 14:44 UTC, Scott Tsai
no flags Details | Diff
diff-r2-r3 (1.81 KB, patch)
2009-12-03 23:39 UTC, Scott Tsai
no flags Details | Diff

Description Scott Tsai 2009-12-03 09:46:04 UTC
NOTE: This is my first package and I'm seeking a sponsor.

Spec URL: http://scottt.tw/fedora/udis86.spec
SRPM URL: http://scottt.tw/fedora/udis86-1.7-1.fc12.src.rpm
Description: udis86 is a disassembler library (libudis86) for x86 and x86-64.
The primary intent is to aid binary code analysis.

The no-documentation warning from rpmlint:
udis86.x86_64: W: no-documentation
is there because udis86 has no COPYING or AUTHORS file and its README file only contains a single line "See docs/udis86.pdf" and the sited udis86.pdf is included in the -devel package.

Comment 1 Susi Lehtola 2009-12-03 13:32:34 UTC
A few notes:

- The source url should be
 http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
see http://fedoraproject.org/wiki/Packaging/SourceURL. (Mark this as Source0.)

- Remove the commented empty Requires: line.

- The summary should read "A disassembler Library for x86 and x86-64" (not just for x86).

- Move
 # test the libudis86 we just built this is the only part that requires yasm
 export LD_LIBRARY_PATH=$(pwd)/libudis86/.libs
 make check
to the %check phase.

- Remove the documentation installed by make install at the end of %install with
 rm -rf %{buildroot}%{_docdir}
and list the relevant files instead as %doc, i.e.

%doc docs/x86optable.* docs/udis86.* docs/index.html docs/ss.jpg docs/style.css

Comment 2 Scott Tsai 2009-12-03 14:44:38 UTC
Created attachment 375797 [details]
diff-r1-r2

spec file diff after incorporating fixes suggested in comment #1

Comment 3 Scott Tsai 2009-12-03 14:46:11 UTC
(In reply to comment #1)
Thanks a lot for the quick feed back!
Fixed here:
Spec URL: http://scottt.tw/fedora/udis86.spec
SRPM URL: http://scottt.tw/fedora/udis86-1.7-2.fc12.src.rpm

Comment 4 Susi Lehtola 2009-12-03 22:27:44 UTC
- You still have the empty Requires: line that has been commented out in the spec. Please remove it.

- Normally the %build and %install phases are placed consecutively in the spec file, and the %check phase is somewhere near the end (e.g. before %clean). This is due to the fact that %check is ran after %install.

- I'm wondering about the arrangement of the files. I don't think udcli should be in the -devel package. It would make a lot more sense, if udcli was in the main package. (Or a subpackage named udcli, as called by upstream)
  If the libs are used by other software, too, then one can separate the library into a subpackage. (-libs or libudis86 as called by upstream)

Comment 5 Susi Lehtola 2009-12-03 22:34:21 UTC
rpmlint output:
udis86.src: W: strange-permission udis86-1.7.tar.gz 0600
udis86.x86_64: W: no-documentation
udis86-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/udis86-1.7/libudis86/input.c
udis86-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/udis86-1.7/libudis86/types.h
udis86-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/udis86-1.7/libudis86/syn-att.c
udis86-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/udis86-1.7/libudis86/udis86.c
udis86-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/udis86-1.7/libudis86/syn-intel.c
udis86-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/udis86-1.7/libudis86/decode.c
udis86-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/udis86-1.7/udcli/udcli.c
udis86-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/udis86-1.7/libudis86/syn.c
udis86-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/udis86-1.7/libudis86/syn.h
4 packages and 0 specfiles checked; 0 errors, 11 warnings.

The first warning is caused by you having odd permissions on the source tarball when you have built the srpm.

The debuginfo warnings are caused by odd permissions in the files within the tarball, and they can be fixed e.g. by adding
 find -name *.c -exec chmod 644 {} \;
 find -name *.h -exec chmod 644 {} \;
in %prep, after %setup.

Fix these.

Comment 6 Scott Tsai 2009-12-03 23:39:33 UTC
Created attachment 375961 [details]
diff-r2-r3

spec file diff after incorporating fixes suggested in comment #4 and #5

Comment 7 Scott Tsai 2009-12-03 23:45:15 UTC
(In reply to comment #5)
Fixed here:
Spec URL: http://scottt.tw/fedora/udis86.spec
SRPM URL: http://scottt.tw/fedora/udis86-1.7-3.fc12.src.rpm

Regarding where to put 'udcli', after some thought I decided to move it to the main package. The rationale being that 'objdump' and 'libopcodes' are both in binutils.

Comment 8 Scott Tsai 2010-01-07 10:17:22 UTC
Dear all, this package review has gone a month without seeing new comments.
Am I correct to assume that the RPM packaging review part can be considered finished and the package can go into Fedora once I find a sponsor?

Comment 9 Susi Lehtola 2010-01-07 12:07:23 UTC
No. No review has yet been made. Once you get a sponsor, s/he'll do the formal review on the package (and possibly on the other submissions you may have, too).

Comment 10 Peter Lemenkov 2010-01-11 18:48:31 UTC
Unblocking FE-NEEDSPONSOR - I just sponsored Scott.

We are really sorry for the delays during packaging cycle, Scott, but they're almost inevitable in the volunteering project like this.

Comment 11 Scott Tsai 2010-01-12 05:43:26 UTC
(In reply to comment #10)
Peter, thanks! I completely understand about it taking some time to process the many sponsorship and review requests. I'll patiently wait for this 'udis86' package to get formally approved then work through the steps in:
https://fedoraproject.org/wiki/PackageMaintainers/Join#Get_Sponsored

Comment 12 Peter Lemenkov 2010-01-26 11:31:37 UTC
REVIEW:

+ rpmlint is almows silent:

[petro@Sulaco ppc]$ rpmlint udis86-*
udis86.ppc: W: no-documentation
2 packages and 0 specfiles checked; 0 errors, 1 warnings.
[petro@Sulaco ppc]$ 

+ The package is named according to the Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec .
+ The package meets the Packaging Guidelines .
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines .
+ The License field in the package spec file matches the actual license (BSD).
0 The source package doesn't include the text of the license(s) in its own file (although it's present in project's SCM).
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package matches the upstream source:

[petro@Sulaco SOURCES]$ sha256sum udis86-1.7.tar.gz*
6128d266abcabed6077fdeebd2fbb7fb48eb599efbdae98922de2f6acd82ce3a  udis86-1.7.tar.gz
6128d266abcabed6077fdeebd2fbb7fb48eb599efbdae98922de2f6acd82ce3a  udis86-1.7.tar.gz.1
[petro@Sulaco SOURCES]$

+ The package successfully compiles and builds into binary rpms on at least one primary architecture.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1945034

+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
+ The package (or subpackage) calls ldconfig in %post and %postun.
+ The package does NOT bundle copies of system libraries.
0 The package does not designed to be relocatable
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
+ Header files are in a -devel package.
0 No static libraries.
0 The package does not contain pkgconfig(.pc) files.
+ The library files that ends in .so (without suffix) are in a -devel package. 
+ The devel sub-package requires the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ All filenames in rpm packages are valid UTF-8.


APPROVED.

Comment 13 Scott Tsai 2010-01-26 13:37:44 UTC
New Package CVS Request
=======================
Package Name: udis86
Short Description: udis86 is a disassembler library for x86 and x86-64.
Owners: scottt
Branches: F-11 F-12 EL-5
InitialCC:

Comment 14 Jason Tibbitts 2010-01-27 05:03:16 UTC
CVS done (by process-cvs-requests.py).

Comment 15 Fedora Update System 2010-01-27 07:54:55 UTC
udis86-1.7-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/udis86-1.7-3.fc11

Comment 16 Fedora Update System 2010-01-27 07:54:59 UTC
udis86-1.7-3.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/udis86-1.7-3.el5

Comment 17 Fedora Update System 2010-01-27 07:55:04 UTC
udis86-1.7-3.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/udis86-1.7-3.fc12

Comment 18 Fedora Update System 2010-01-28 00:56:43 UTC
udis86-1.7-3.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update udis86'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2010-1143

Comment 19 Fedora Update System 2010-01-28 01:03:33 UTC
udis86-1.7-3.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update udis86'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2010-1171

Comment 20 Fedora Update System 2010-01-28 21:15:44 UTC
udis86-1.7-3.el5 has been pushed to the Fedora EPEL 5 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update udis86'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/EL-5/FEDORA-EPEL-2010-0143

Comment 21 Fedora Update System 2010-02-09 03:57:34 UTC
udis86-1.7-3.el5 has been pushed to the Fedora EPEL 5 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update udis86'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/EL-5/FEDORA-EPEL-2010-0143

Comment 22 Fedora Update System 2010-02-09 05:02:37 UTC
udis86-1.7-3.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 23 Fedora Update System 2010-02-09 05:03:46 UTC
udis86-1.7-3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 24 Fedora Update System 2010-07-30 01:06:21 UTC
udis86-1.7-3.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.


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