Bug 804666 - Review Request: libpfm - Library to encode performance events for use by perf tool
Review Request: libpfm - Library to encode performance events for use by perf...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Gwyn Ciesla
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-19 10:41 EDT by William Cohen
Modified: 2012-06-12 16:46 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-06-12 16:46:32 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
wcohen: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description William Cohen 2012-03-19 10:41:27 EDT
Spec URL: http://people.redhat.com/wcohen/libpfm/libpfm.spec
SRPM URL: http://people.redhat.com/wcohen/libpfm/libpfm-4.2.0-2.fc18.src.rpm
Description: 

libpfm4 is a library to help encode events for use with operating system
kernels performance monitoring interfaces. The current version provides support
for the perf_events interface available in upstream Linux kernels since v2.6.31.
Comment 1 William Cohen 2012-03-19 10:43:05 EDT
The current version of papi builds libpfm bundled into the rpm. This review is being submitted to unbundle libpfm from papi and allow libpfm to a separate rpm.
Comment 2 Michael Schwendt 2012-03-25 16:18:32 EDT
* https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


* https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries


* https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag


* https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean


> %{!?with_python: %global with_python 1}

You may want to look into using "%bcond_without python" instead.


> %defattr(644,root,root,755)
> %doc README

https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions


> %attr(755,root,root) %{_libdir}/lib*.so*

Ownership is root:root by default. If the mode is not 0755 for the installed lib, I highly recommend fixing it upstream and temporarily during %install. Usage of the eye-catching %attr should be limited to non-ordinary file permissions as not to overuse it.
Comment 3 William Cohen 2012-03-28 16:30:23 EDT
Thanks for the review. I have attempted to address each of the points mentioned in the review in libpfm-4.2.0-3.fc16.src.rpm and libpfm.spec available at:

http://people.redhat.com/wcohen/libpfm/

A scratch build is also available at:

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



* https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

Change requires to:

Requires: %{name}%{?_isa} = %{version}-%{release}


* https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

There are cases where HPC developer want static versions of the library (the compute node may not have shared libraries installed). I have split the static library into a libpfm-static sub package.


* https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

Removed the unneeded Buildroot tag


* https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean

Removed the unneeded %clean section


> %{!?with_python: %global with_python 1}

You may want to look into using "%bcond_without python" instead.

convered the libpfm.spec to use %bcond_without. 


For the file permissions it looks like the permissions were correct so removed the %defattr.


> %defattr(644,root,root,755)
> %doc README

https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions



> %attr(755,root,root) %{_libdir}/lib*.so*

Ownership is root:root by default. If the mode is not 0755 for the installed
lib, I highly recommend fixing it upstream and temporarily during %install.
Usage of the eye-catching %attr should be limited to non-ordinary file
permissions as not to overuse it.
Comment 4 William Cohen 2012-03-28 17:39:22 EDT
Note rpmlint complains about a couple things in libpfm-python rpm:

libpfm-python.x86_64: E: non-executable-script /usr/lib64/python2.6/site-packages/perfmon/pmu.py 0644L /usr/bin/env
libpfm-python.x86_64: E: non-executable-script /usr/lib64/python2.6/site-packages/perfmon/session.py 0644L /usr/bin/env

Checking with upstream developers whether those should be executable files or whether #! should be removed from the files.
Comment 5 William Cohen 2012-06-06 11:08:16 EDT
Would it be possible to get this package reviewed and into Rawhide?

Updated the libpfm.spec file to include a patch for the python files. The new libpfm.spec and libpfm-4.2.0-4.fc17.src.rpm are available at 

http://people.redhat.com/wcohen/libpfm/


The results of rpmlint are pretty minor:

$ rpmlint ../SRPMS/libpfm-4.2.0-4.fc17.src.rpm ../RPMS/x86_64/libpfm-*
libpfm.src: W: spelling-error Summary(en_US) perf -> pref, per, perv
libpfm.src: W: spelling-error %description -l en_US perf -> pref, per, perv
libpfm.src:70: W: rpm-buildroot-usage %build %global python_config CONFIG_PFMLIB_NOPYTHON=n PYTHON_PREFIX=$RPM_BUILD_ROOT/%{python_prefix}
libpfm.x86_64: W: spelling-error Summary(en_US) perf -> pref, per, perv
libpfm.x86_64: W: spelling-error %description -l en_US perf -> pref, per, perv
libpfm.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libpfm.so
libpfm-devel.x86_64: W: spelling-error Summary(en_US) perf -> pref, per, perv
libpfm-devel.x86_64: W: spelling-error %description -l en_US perf -> pref, per, perv
libpfm-python.x86_64: W: spelling-error Summary(en_US) perf -> pref, per, perv
libpfm-python.x86_64: W: spelling-error %description -l en_US perf -> pref, per, perv
libpfm-python.x86_64: W: no-documentation
libpfm-static.x86_64: W: spelling-error Summary(en_US) perf -> pref, per, perv
libpfm-static.x86_64: W: spelling-error %description -l en_US perf -> pref, per, perv
libpfm-static.x86_64: W: no-documentation
6 packages and 0 specfiles checked; 0 errors, 14 warnings.
Comment 6 Gwyn Ciesla 2012-06-07 13:22:50 EDT
Good:

- rpmlint checks return:

The spelling errors are no problem, but rpm-build-root and devel-file-in-non-devel-package should be fixed.  Move the libpfm.so to -devel, and remove $RPM_BUILD_ROOT from line 70.  Still builds with this change.

- package meets naming guidelines
- package meets packaging guidelines
- license ( MIT ) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
FIX package compiles on devel (x86_64)

Builds fine on f17, but dies in mock for rawhide.

End of relevant log section:

cc  -g -Wall -Werror -I. -I/builddir/build/BUILD/libpfm-4.2.0/perf_examples/../include -DCONFIG_PFMLIB_DEBUG -DCONFIG_PFMLIB_OS_LINUX -I. -D_GNU_SOURCE -pthread -c notify_self.c
notify_self.c:50:29: error: 'struct siginfo' declared inside parameter list [-Werror]
notify_self.c:50:29: error: its scope is only this definition or declaration, which is probably not what you want [-Werror]
notify_self.c: In function 'sigio_handler':
notify_self.c:59:10: error: dereferencing pointer to incomplete type
notify_self.c:67:10: error: dereferencing pointer to incomplete type
notify_self.c:70:39: error: dereferencing pointer to incomplete type
notify_self.c:72:49: error: dereferencing pointer to incomplete type
notify_self.c:93:18: error: dereferencing pointer to incomplete type
notify_self.c: In function 'main':
notify_self.c:133:39: error: assignment from incompatible pointer type [-Werror]
cc1: all warnings being treated as errors


- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- devel package ok
- no .la files
- post/postun ldconfig ok
- devel requires base package n-v-r 

So it's the two things above.  Also I was wondering, do you really need to ship the static libs, since it builds a solib?  Does papi or something else need it?
Comment 7 William Cohen 2012-06-07 15:14:15 EDT
The newer version of glibc does not have struct siginfo, and ust siginfo_t should be used instead. Made a patch and sent it to the upstream 
perfmon2-devel@lists.sourceforge.net mailing list. The patched version of the libpfm-4.2.0 now compile successfully on Fedora Rawhide.

The revised libpfm.spec and libpfm-4.2.0-5.fc17.src.rpm are at http://people.redhat.com/wcohen/libpfm/


Most people will just use the shared libraries, but there are people in High Performance Computing (HPC) that may not have much in the way of shared libraries on the compute nodes and they would like to have static versions of the papi/libpfm to support that environment.
Comment 9 Gwyn Ciesla 2012-06-07 15:24:53 EDT
Ok, rawhide build is good, and I concur on static libs, but you still need to remove the buildroot tag from %build and move the .so to -devel.
Comment 10 William Cohen 2012-06-07 15:53:04 EDT
Fixed up the buildroot tag and moved the .so to -devel in libpfm-4.2.0-6.fc17.src.rpm on people.redhat.com/wcohen/libpfm/.  Results of rpmlint:

$ rpmlint ../SRPMS/libpfm-4.2.0-6.fc17.src.rpm ../RPMS/x86_64/libpfm-*4.2.0-6*
libpfm.src: W: spelling-error Summary(en_US) perf -> pref, per, perv
libpfm.src: W: spelling-error %description -l en_US perf -> pref, per, perv
libpfm.x86_64: W: spelling-error Summary(en_US) perf -> pref, per, perv
libpfm.x86_64: W: spelling-error %description -l en_US perf -> pref, per, perv
libpfm-devel.x86_64: W: spelling-error Summary(en_US) perf -> pref, per, perv
libpfm-devel.x86_64: W: spelling-error %description -l en_US perf -> pref, per, perv
libpfm-python.x86_64: W: spelling-error Summary(en_US) perf -> pref, per, perv
libpfm-python.x86_64: W: spelling-error %description -l en_US perf -> pref, per, perv
libpfm-python.x86_64: W: no-documentation
libpfm-static.x86_64: W: spelling-error Summary(en_US) perf -> pref, per, perv
libpfm-static.x86_64: W: spelling-error %description -l en_US perf -> pref, per, perv
libpfm-static.x86_64: W: no-documentation
6 packages and 0 specfiles checked; 0 errors, 12 warnings.
Comment 11 Gwyn Ciesla 2012-06-08 08:42:38 EDT
Excellent, APPROVED.
Comment 12 William Cohen 2012-06-08 09:41:06 EDT
New Package SCM Request
=======================
Package Name: libpfm
Short Description: Library to encode performance events for use by perf tool
Owners: wcohen
Branches: 
InitialCC:
Comment 13 Gwyn Ciesla 2012-06-08 09:44:53 EDT
Turns out this already existed but was retired.  I unretired it, take
ownership in pkgdb, and request branches other than rawhide if you need
them.
Comment 14 William Cohen 2012-06-08 10:34:59 EDT
Okay I have taken ownership of it and imported the updated srpm.  When trying to build in f18 I get:

(x86-07.phx2.fedoraproject.org) -> FAILED: BuildError: package libpfm is blocked for tag f18

Is there some flag that needs to be set for F18/rawhide?
Comment 15 Gwyn Ciesla 2012-06-08 10:50:19 EDT
Oh, I forgot, you need to log a trac with rel-eng to get it unblocked.

https://fedorahosted.org/rel-eng/
Comment 16 William Cohen 2012-06-08 17:20:03 EDT
Package Change Request
======================
Package Name: libpfm
New Branches: f18
Owners: wcohen

Unretiring the libpfm package so can avoid having papi build a bundled version of libpfm.
Comment 17 Gwyn Ciesla 2012-06-09 13:18:44 EDT
Invalid branch, f18==rawhide==devel==master.
Comment 18 William Cohen 2012-06-12 16:46:32 EDT
The libpfm package is now in fedora rawhide.

http://koji.fedoraproject.org/koji/buildinfo?buildID=324030

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