Bug 191743 - Review Request: sysprof - a sampling CPU profiler
Summary: Review Request: sysprof - a sampling CPU profiler
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Parag AN(पराग)
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On: 191745
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-05-15 15:32 UTC by Gianluca Sforna
Modified: 2007-11-30 22:11 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2006-10-08 08:51:56 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Gianluca Sforna 2006-05-15 15:32:38 UTC
Spec URL: http://giallu.interfree.it/fedora/sysprof.spec
SRPM URL: http://giallu.interfree.it/fedora/sysprof-1.0.2-1.src.rpm

Description:
Sysprof is a sampling CPU profiler for Linux that uses a kernel module
to profile the entire system, not just a single application.
Sysprof handles shared libraries and applications do not need to be
recompiled. In fact they don't even have to be restarted.

Comment 1 Parag AN(पराग) 2006-06-02 04:43:53 UTC
Some Comments:-
rpmling gives error as
E: sysprof configure-without-libdir-spec

Comment 2 Gianluca Sforna 2006-06-02 10:36:48 UTC
I am leaving for a couple of weeks, so I can not look into this until I come back.

thanks for your patience...


Comment 3 Gianluca Sforna 2006-06-22 08:47:40 UTC
I fixed the rpmlint error by using the %configure macro and also updated the
package to version 1.0.3

Spec URL: http://giallu.interfree.it/fedora/sysprof.spec
SRPM URL: http://giallu.interfree.it/fedora/sysprof-1.0.3-1.src.rpm

Comment 4 Dan Horák 2006-06-26 19:42:59 UTC
The package should own the directory %{_datadir}/sysprof and this line will be
sufficient in the files section. Then there is also no need to specify each file
explicitly.

Comment 5 Dan Horák 2006-06-26 20:39:45 UTC
%{_datadir}/%{name} will be even better, similar package the binary as
%{_bindir}/%{name}

Comment 6 Gianluca Sforna 2006-06-28 08:08:34 UTC
Thanks. I added a %dir %{_datadir}/sysprof line to the spec, but I prefer to
maintain the explicit list of files, since there are really few of them.

I am waiting a bit for new comments/requests before updating the .spec and
src.rpm files.

Comment 7 Gianluca Sforna 2006-07-07 21:58:29 UTC
Forgot to mention this is my first package submission so I am seeking for a sponsor

Comment 8 Stewart Adam 2006-09-18 21:28:57 UTC
I'm not a reviewer, but I can help:
This is shown by rpmlin on the SRPMS...

E: sysprof configure-without-libdir-spec
A configure script is run without specifying the libdir. configure
options must be augmented with something like --libdir=%{_libdir}.

Comment 9 Gianluca Sforna 2006-09-29 23:12:44 UTC
New package addressing comment #1 (and comment #8...)

http://giallu.homelinux.org/fedora/sysprof.spec
http://giallu.homelinux.org/fedora/sysprof-1.0.3-2.src.rpm

Please note I have been sponsored, so now it only need an official review from
another regular contributor to have this piece of software in Fedora.


Comment 10 Parag AN(पराग) 2006-09-30 06:23:42 UTC
I suggest you to change SPEC name to sysprof-kmod-common.spec and resubmit
package. This is according to kernel module packaging guidelines. Thoug its
given that userlan package should 
Provides: %{name}- kmod-common = %{version}
it will be good idea to have similar name given to SPEC also.


Comment 11 Parag AN(पराग) 2006-09-30 06:38:13 UTC
Anyway i tried with latest uploaded SRPM link. Mock build is failed with error
checking for cplus_demangle_opname in -liberty... no
configure: error: libiberty is required to compile sysprof
error: Bad exit status from /var/tmp/rpm-tmp.63043 (%build)


Comment 12 Ville Skyttä 2006-09-30 10:58:43 UTC
(In reply to comment #10)
> I suggest you to change SPEC name to sysprof-kmod-common.spec and resubmit
> package. This is according to kernel module packaging guidelines.

Nope, see
http://fedoraproject.org/wiki/Packaging/KernelModules#head-164fc2703b23579d258b39d675a92643669507e0
In this case such a rename would sound plain wrong to me.

Provides: sysprof-kmod-common is lacking a version, though.

Comment 13 Parag AN(पराग) 2006-10-01 08:59:52 UTC
so i can give any name to userlan package SPEC file with following MUST right?
      - MUST: The package needs to require the belonging kernel-module with
something like 'Requires: %{name}- kmod = %{version}'

      - MUST: The package needs to provide %{name}-kmod-common with something
like 'Provides: %{name}- kmod-common = %{version}' or the name of the package
must be %{name}-kmod-common

Comment 14 Ville Skyttä 2006-10-01 16:15:40 UTC
(In reply to comment #13)
> so i can give any name to userlan package SPEC file

Within the usual package naming guidelines, yes.  (I assume you're talking 
about the userland package name, not only the specfile filename.)

>       - MUST: The package needs to require the belonging kernel-module with
> something like 'Requires: %{name}- kmod = %{version}'

">=" is better than "=" here - the kmod guidelines are in need of some 
updates.

Comment 15 Kevin Fenzi 2006-10-01 21:12:43 UTC
Parag: Seems you are reviewing this package, so I am changing the blocker to FE-
REVIEW. If thats not the case you can change it back to FE-NEW and reassign to 
nobody

Comment 16 Gianluca Sforna 2006-10-01 22:29:22 UTC
New package: this one builds correctly in mock (-devel)

http://giallu.homelinux.org/fedora/sysprof.spec
http://giallu.homelinux.org/fedora/sysprof-1.0.3-3.src.rpm

Ville: I am not sure why the Require should read ">=": I assumed kmod- and
kmod-common should always be updated in sync, while the ">=" seems to imply I
could update only the kmod- retaining the older kmod-common. Am I missing something?

Anyway, if you like I could also update the wiki page.

Comment 17 Ville Skyttä 2006-10-02 15:46:06 UTC
The exact details escape me at the moment, but while = would be closer to the 
intent, >= here IIRC helps in some upgrade scenarios.  It's also possible that 
comment 14 was a brainfart - thl, do you remember better?  Anyway, upgrading 
only the kmod package is prevented by the specfile emitted by kmodtool; it 
produces a >= dependency to the corresponding kmod-common in the kmod package.

Comment 18 Thorsten Leemhuis 2006-10-02 17:28:58 UTC
(In reply to comment #17)
> The exact details escape me at the moment, but while = would be closer to the 
> intent, >= here IIRC helps in some upgrade scenarios.  It's also possible that 
> comment 14 was a brainfart - thl, do you remember better?

/me scratches his head and tries to remember but fails for now, too

Anyway: I agree ">= here IIRC helps in some upgrade scenarios" and that should
be used.

Comment 19 Parag AN(पराग) 2006-10-03 09:36:52 UTC
Review:
+ package builds in mock (development i386).
+ source files match upstream.
8949fe32a073b84cb2abb7f9d608f755  sysprof-1.0.3.tar.gz
+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written
+ dist tag is present.
+ build root is correct.
+ license is open source-compatible.  License text included in package.
+ BuildRequires are proper.
+ %clean is present.
+ package installs properly
+ rpmlint is silent.
+ Provides: sysprof-kmod-common = 1.0.3
+ Requires: kmod-sysprof >= 1.0.3 
libatk-1.0.so.0 
libc.so.6 
libc.so.6(GLIBC_2.0) 
libc.so.6(GLIBC_2.1) 
libc.so.6(GLIBC_2.2.3) 
libc.so.6(GLIBC_2.3.4) 
libc.so.6(GLIBC_2.4) 
libcairo.so.2 
libdl.so.2 
libdl.so.2(GLIBC_2.0) 
libdl.so.2(GLIBC_2.1) 
libgdk-x11-2.0.so.0 
libgdk_pixbuf-2.0.so.0 
libglade-2.0.so.0 
libglib-2.0.so.0 
libgmodule-2.0.so.0 
libgobject-2.0.so.0 
libgthread-2.0.so.0 
libgtk-x11-2.0.so.0
libm.so.6 
libpango-1.0.so.0
libpangocairo-1.0.so.0
libpangoft2-1.0.so.0
libpthread.so.0 
libpthread.so.0(GLIBC_2.0) 
libxml2.so.2 
libz.so.1 
rtld(GNU_HASH)

+ owns the directories it creates.
+ doesn't own any directories it shouldn't.
+ no duplicates in %files.
+ file permissions are appropriate.
+ no scriptlets present.
+ documentation is small, so no -docs subpackage is necessary.
+ Not a GUI app

APPROVED.

Comment 20 Parag AN(पराग) 2006-10-03 09:43:00 UTC
Don't forget to close this NEXTRELASE when you have imported and built it.

Comment 21 Gianluca Sforna 2006-10-04 11:12:45 UTC
(In reply to comment #19)
> + Not a GUI app

???
it is actually a GNOME program... and this let me realize I did not add a
.desktop file (sorry, this was my first packaging attempt).

I will import a fixed spec. Thanks a lot

Comment 22 Parag AN(पराग) 2006-10-04 11:35:49 UTC
(In reply to comment #21)
> (In reply to comment #19)
> > + Not a GUI app
> 
> ???
> it is actually a GNOME program... and this let me realize I did not add a
> .desktop file (sorry, this was my first packaging attempt).
> 

Yup By mistake i wrote that. in fact that was from my template for review which
i just copied but not erased. I in fact check thet GUI APP already. That was
because i could not found any .desktop file.

> I will import a fixed spec. Thanks a lot

Yes .desktop file is not available in this package. Add it and then import your
package.


Comment 23 Gianluca Sforna 2006-10-08 08:51:56 UTC
Package imported and built


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