Bug 487365

Thanks for the submission.  Here's the review.  Lines beginning with X need attention; those beginning with * are okay:

* verify the final provides and requires of the binary RPMs
X make sure lines are <= 80 characters
  - please add some line continuations to fix this
* package successfully compiles and builds
* BuildRequires are proper
* macros fine
* package is named appropriately
* it is legal for Fedora to distribute this
* license field matches the actual license.
* license is open source-compatible.
* specfile name matches %{name}
* md5sum matches upstream
  - other than timestamp differences, my generated tarball matches
* skim the summary and description for typos, etc.
X summary and description good
  - please add Eclipse somewhere in the Summary.  Something like "Eclipse plugin for OProfile".
  - please remove " (Incubation)" from the summary
  - remove "powerful" in the description.  The description could also mention the CDT.
* correct buildroot
* %{?dist} used correctly
* license text included in package and marked with %doc
* packages meets FHS (http://www.pathname.com/fhs/)
X rpmlint on <this package>.srpm gives no output

$ rpmlint eclipse-oprofile-0.1.0-1.fc10.src.rpm
eclipse-oprofile.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 4)
eclipse-oprofile.src: W: strange-permission eclipse-oprofile-fetch-src.sh 0775

Please fix both of these things.  Just make the fetch script 644 or something and modify the instructions for generating the tarball to be:  "sh ./eclipse-oprofile-fetch-src.sh".

* changelog format okay
* Summary tag does not end in a period
* no PreReq
* specfile is legible
* specfile written in American English
* no -doc sub-package necessary
* one native bit has no rpath, static linking, etc.
* no config files
* not a GUI app
* no -devel necessary
* install section begins with rm -rf $RPM_BUILD_ROOT or %{buildroot}
* no translations so no locale handling
* no Requires(pre,post)
* package not relocatable
* package contains code
* package owns all directories and files
* no %files duplicates
* file permissions fine
* %clean present
* %doc files do not affect runtime
* not a web app
* package includes license text in the package and marks it with %doc
* run rpmlint on the binary RPMs => no output

$ rpmlint eclipse-oprofile-*
eclipse-oprofile.x86_64: W: non-conffile-in-etc /etc/security/console.apps/opcontrol
eclipse-oprofile.x86_64: W: non-conffile-in-etc /etc/pam.d/opcontrol
2 packages and 0 specfiles checked; 0 errors, 2 warnings.

This is fine 'cause it needs to be there for correct use of pam/consolehelper, right?

* I verified that it installs and that the oprofile feature is available.  Could you post a test project to try to verify that the functionallity works?  I'm getting the OProfile view but not seeing any results.

Updated spec and srpm uploaded (same url).

>X make sure lines are <= 80 characters
>  - please add some line continuations to fix this

for some lines this introduces errors, eg:
 
%define corepath %{buildroot}%{install_loc}/eclipse/plugins/org.eclipse.linuxtools.oprofile.core_%{ver_qual}

can not be split. For others (eg: very long paths) it seems to harm readability, but other than that line continuations added where possible.

>X summary and description good
>  - please add Eclipse somewhere in the Summary.  Something like "Eclipse

Fixed.

>X rpmlint on <this package>.srpm gives no output

Fixed.

>This is fine 'cause it needs to be there for correct use of pam/consolehelper,
>right?

Indeed, they are not config files per-se, since there is no configuration to be done by the user. Once placed there they never need to be touched.

Thanks.  I've verified that the rpmlint issues are gone and the line length stuff is fixed.  My only remaining nit is the description:  please include the CDT somehow and drop the "powerful".  Something like "... profiling capabilities with the CDT."

Otherwise, we're good to go.

> My only remaining nit is the description:  please include the
> CDT somehow and drop the "powerful".  Something like "... profiling
> capabilities with the CDT."

Fixed.

Thanks.  This package is approved.

Kent, please follow the procedure here:

https://fedoraproject.org/wiki/Package_Review_Process

which, as a next step has you follow this:

https://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure

New Package CVS Request
=======================
Package Name: eclipse-oprofile
Short Description: Eclipse plugin for OProfile integration
Owners: ksebasti
Branches: F-10
InitialCC: overholt

Kent: I don't see you in the packager group. Is this your first package?

Kent:  I'll sponsor you.  Read the instructions here:

https://fedoraproject.org/wiki/Join_the_package_collection_maintainers

(In reply to comment #8)
> Kent: I don't see you in the packager group. Is this your first package?

First package indeed. I applied to the packager group, and overholt has since sponsored :)

Thanks. 

cvs done.

Builded in repos.

Builded in repos.