Bug 487312 - Review Request: tuned - A dynamic adaptive system tuning daemon
Summary: Review Request: tuned - A dynamic adaptive system tuning daemon
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Thomas Woerner
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: F11PowerSaving
TreeView+ depends on / blocked
 
Reported: 2009-02-25 13:24 UTC by Phil Knirsch
Modified: 2015-03-05 01:20 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-03-03 10:03:45 UTC
Type: ---
Embargoed:
twoerner: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Phil Knirsch 2009-02-25 13:24:40 UTC
Spec URL: http://pknirsch.fedorapeople.org/src/tuned.spec
SRPM URL: http://pknirsch.fedorapeople.org/src/tuned-0.1.0-1.fc10.src.rpm
Description: The tuned package contains a daemon that tunes system settings dynamically. It is part of the Fedora 11 Power Management feature (see https://fedoraproject.org/wiki/Features/PowerManagement)

Comment 1 Thomas Woerner 2009-02-25 18:14:58 UTC
$ rpmlint tuned-0.1.0-1.fc10.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
OK

$ rpmlint tuned-0.1.0-1.fc10.noarch.rpm
tuned.noarch: W: incoherent-subsys /etc/rc.d/init.d/tuned $prog
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
OK

$ rpmlint tuned-utils-0.1.0-1.fc10.noarch.rpm
tuned-utils.noarch: W: no-documentation
tuned-utils.noarch: E: devel-dependency kernel-debuginfo
1 packages and 0 specfiles checked; 1 errors, 1 warnings.

Hm, I wonder why there is a devel-dependency for kernel-debuginfo. I see that the scripts are systemtap scripts. Therefore this should be OK.

Packaging guidelines:

Can you please fix the URL for the git tree to be consistent and maybe add a description how to get the proper version from git? Please also fix the url in the wiki page. (See: https://fedoraproject.org/wiki/Packaging/SourceURL)

Can you please use "%(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)" for Buildroot? (See: BuildRoot tag at https://fedoraproject.org/wiki/Packaging/Guidelines)

Please fix whitespaces in the GPL headers (example: tuned).

Why is tuningplugins/__init__.py empty?

Why is there a reference to configure in INSTALL. There is no configure script at all.

Comment 2 Phil Knirsch 2009-02-26 12:48:16 UTC
(In reply to comment #1)
> 
> Hm, I wonder why there is a devel-dependency for kernel-debuginfo. I see that
> the scripts are systemtap scripts. Therefore this should be OK.
> 

Yea, i was wondering about that as well. Seems that rpmlint assumes that if you install a debuginfo package you want to to development instead of debugging. ;)

> Packaging guidelines:
> 
> Can you please fix the URL for the git tree to be consistent and maybe add a
> description how to get the proper version from git? Please also fix the url in
> the wiki page. (See: https://fedoraproject.org/wiki/Packaging/SourceURL)
> 

Added the following over the Source tag:

# The source for this package was pulled from upstream git.  Use the
# following commands to get the corresponding tarball:
#  git clone git://fedorapeople.org/~pknirsch/tuned.git/
#  cd tuned
#  git checkout v%{version}
#  make archive

(similar to the upstream CVS description in the SourceURL docu)

> Can you please use "%(mktemp -ud
> %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)" for Buildroot? (See:
> BuildRoot tag at https://fedoraproject.org/wiki/Packaging/Guidelines)
> 

Fixed.

> Please fix whitespaces in the GPL headers (example: tuned).
> 

Fixed.

> Why is tuningplugins/__init__.py empty?
> 

Intentionally left empty. ;) But in all seriousness, Python packages require you to install at least an empty __init__.py to work, see http://docs.python.org/tutorial/modules.html#packages. I've added a comment line to both __init__.py files now.

> Why is there a reference to configure in INSTALL. There is no configure script
> at all.

Uhm, yea, thats was just the standard GNU INSTALL file. I've stripped it down "a little" now so it only contains what really can be done.

New packages are now up:

Spec URL: http://pknirsch.fedorapeople.org/src/tuned.spec
SRPM URL: http://pknirsch.fedorapeople.org/src/tuned-0.1.1-1.fc10.src.rpm

Comment 3 Thomas Woerner 2009-02-26 15:14:53 UTC
MUST Items
----------

[WARN] rpmlint output

$ rpmlint tuned-0.1.1-1.fc10.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint tuned-0.1.1-1.fc11.noarch.rpm
tuned.noarch: W: incoherent-subsys /etc/rc.d/init.d/tuned $prog
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

[OK] The warning here can be ignored, it is not incoherent, because $prog contains the correct value.

$  rpmlint tuned-utils-0.1.1-1.fc11.noarch.rpm 
tuned-utils.noarch: W: no-documentation
tuned-utils.noarch: E: devel-dependency kernel-debuginfo
1 packages and 0 specfiles checked; 1 errors, 1 warnings.

[WARN] There is no description for the utils package.
[OK] The error can be ignored, the kernel-debuginfo package is requires to use systemtap.

[OK] Named according to the  Package Naming Guidelines.
[OK] The spec file name must match the base package.
[FAIL/INFO] The package must meet the  Packaging Guidelines.

FAIL: The description is not more than the summary. Please add some more information maybe on how it is working or how it should be used.
INFO: There is a README and README.txt. What is the difference?

[OK] Meets the Licensing Guidelines (GPLv2+).
[OK] License field in the package spec file matches the actual license.
[OK] License file included in %doc.
[OK] Spec file written in American English.
[OK] Spec file is legible.
[OK] Source matches upstream.
[OK] Package successfully compiles and builds at least one primary architecture.
[OK] No build dependencies.
[OK] No localized files, therefore no locale support needed.
[OK] No schared libs, therefore no ldconfig needed.
[OK] Not relocatable.
[OK] Package ownes all directories it creates.
[OK] Does not contain any duplicate files in the %files listing.
[OK] Permissions on files are set properly.
[OK] Has a correct %clean section.
[OK] Macros consistently used.
[OK] Code or permissible content.
[OK] No large documentation files, therefore no -doc subpackage needed.
[OK] %doc files don't affect runtime.
[OK] No header files and no libraries, therefore no -devel subpackage needed.
[OK] No static libs, therefore no -static subpackage needed.
[OK] No pkgconfig file, therefore no requires for pkgconfig needed.
[OK] No GUI applications, therefore no %{name}.desktop file needed.
[OK] Does not own files or directories already owned by other packages.
[OK] Cleanup at the beginning of %install.
[OK] All filenames in rpm packages are valid UTF-8.

SHOULD Items
------------

[OK] License file included.
[BAD] No translations for Non-English languages.
[OK] Builds in mock.
[OK] Should compile and build into binary rpms on all supported architectures.
[BAD] Package functions as described: Not enough information for this.
[OK] Scriptlets seem to be sane.
[OK] No -devel subpackage, therefore no requirement for base package needed.
[OK] -utils subpackage is independent, therefore no requirement for base package needed.
[OK] No pkgconfig files, therefore no placement needed.
[OK] No file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin.

Comment 4 Phil Knirsch 2009-02-26 17:43:19 UTC
(In reply to comment #3)
> MUST Items
> ----------
> 
> [OK] Named according to the  Package Naming Guidelines.
> [OK] The spec file name must match the base package.
> [FAIL/INFO] The package must meet the  Packaging Guidelines.
> 
> FAIL: The description is not more than the summary. Please add some more
> information maybe on how it is working or how it should be used.
> INFO: There is a README and README.txt. What is the difference?
> 

Extended the description of both the main and the utils package to contain more useful information about the packages.
Also renamed the README.txt to DESIGN.txt as it is a rough draft of the design of the tuned.

> 
> SHOULD Items
> ------------
> 
> [OK] License file included.
> [BAD] No translations for Non-English languages.

Will come in a future version.

> [BAD] Package functions as described: Not enough information for this.

Extended the README and the tuned manpage, added a tuned.conf manpage and added a README.utils that describes in detail what the systemtap scripts do.

Thanks a lot for the review so far!

Regards, Phil

New packages are now up:

Spec URL: http://pknirsch.fedorapeople.org/src/tuned.spec
SRPM URL: http://pknirsch.fedorapeople.org/src/tuned-0.1.2-1.fc10.src.rpm

Comment 5 Thomas Woerner 2009-03-02 11:30:09 UTC
MUST Items
----------

[FAIL] rpmlint

$ rpmlint tuned-0.1.2-1.fc10.src.rpm 
tuned.src:78: E: files-attr-not-set
1 packages and 0 specfiles checked; 1 errors, 0 warnings.

[FAIL] Attrs not set: This is the result of adding the %doc before the %defattr in the -utils subpackage.

$ rpmlint tuned-0.1.2-1.fc11.noarch.rpm 
tuned.noarch: W: incoherent-subsys /etc/rc.d/init.d/tuned $prog
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

[OK] The warning here can be ignored, it is not incoherent, because $prog
contains the correct value.

$ rpmlint tuned-utils-0.1.2-1.fc11.noarch.rpm 
tuned-utils.noarch: E: devel-dependency kernel-debuginfo
1 packages and 0 specfiles checked; 1 errors, 0 warnings.

[OK] The error can be ignored, the kernel-debuginfo package is requires to use
systemtap scripts.


[OK] Named according to the  Package Naming Guidelines.
[OK] The spec file name must match the base package.
[OK] The package must meet the Packaging Guidelines.
[INFO] Meets the Licensing Guidelines (GPLv2+).

[INFO] COPYING file is not the latest version. Also the headers in the files are the old address version. (The automake packages, up to 1.10, contain the old v2 version, which is bad).

[OK] License field in the package spec file matches the actual license.
[OK] License file included in %doc.
[OK] Spec file written in American English.
[OK] Spec file is legible.
[INFO] Source matches upstream.

[INFO] Cloning the git tree will not checkout the latest version, but v0.1.1.

[OK] Package successfully compiles and builds at least one primary architecture.
[OK] No build dependencies, therefore no BuildRequires needed.
[OK] No localized files, therefore no locale support needed.
[OK] No schared libs, therefore no ldconfig needed.
[OK] Not relocatable.
[OK] Package ownes all directories it creates.
[OK] Does not contain any duplicate files in the %files listing.
[FAIL] Permissions on files are set properly.

See rpminfo section.

[OK] Has a correct %clean section.
[OK] Macros consistently used.
[OK] Code or permissible content.
[OK] No large documentation files, therefore no -doc subpackage needed.
[OK] %doc files do not affect runtime.
[OK] No header files and no libraries, therefore no -devel subpackage needed.
[OK] No static libs, therefore no -static subpackage needed.
[OK] No pkgconfig file, therefore no requires for pkgconfig needed.
[OK] No GUI applications, therefore no %{name}.desktop file needed.
[OK] Does not own files or directories already owned by other packages.
[OK] Cleanup at the beginning of %install.
[OK] All filenames in rpm packages are valid UTF-8.

SHOULD Items
------------

[OK] License file included.
[BAD] No translations for Non-English languages.
[OK] Builds in mock.
[OK] Should compile and build into binary rpms on all supported architectures.
[OK] Package functions as described.
[OK] Scriptlets seem to be sane.
[OK] No -devel subpackage, therefore no requirement for base package needed.
[OK] -utils subpackage is independent, therefore no requirement for base package needed.
[OK] No pkgconfig files, therefore no placement needed.
[OK] No file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin.

Comment 6 Phil Knirsch 2009-03-02 11:39:33 UTC
(In reply to comment #5)
> MUST Items
> ----------
> 
> [FAIL] rpmlint
> 
> $ rpmlint tuned-0.1.2-1.fc10.src.rpm 
> tuned.src:78: E: files-attr-not-set
> 1 packages and 0 specfiles checked; 1 errors, 0 warnings.
> 
> [FAIL] Attrs not set: This is the result of adding the %doc before the %defattr
> in the -utils subpackage.
> 

DOH! Fixed.

Spec URL: http://pknirsch.fedorapeople.org/src/tuned.spec
SRPM URL: http://pknirsch.fedorapeople.org/src/tuned-0.1.3-1.fc10.src.rpm

Thanks & regards, Phil

Comment 7 Thomas Woerner 2009-03-02 11:47:18 UTC
The 0.1.3-1 package is good. Therefore it gets my approval.

Comment 8 Phil Knirsch 2009-03-02 13:02:58 UTC
New Package CVS Request
=======================
Package Name: tuned
Short Description: A dynamic adaptive system tuning daemon
Owners: pknirsch
Branches: devel
InitialCC:

Comment 9 Kevin Fenzi 2009-03-03 00:40:53 UTC
cvs done.

Comment 10 Phil Knirsch 2009-03-03 10:03:45 UTC
Package successfully built in dist-f11.

Closing bug as NEXTRELEASE as per Package Review Guideline.

Thanks everyone!

Regards, Phil


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