Bug 553649 (rhn-custom-info) - Review Request: rhn-custom-info - Set and list custom values for RHN-enabled machines
Summary: Review Request: rhn-custom-info - Set and list custom values for RHN-enabled ...
Keywords:
Status: CLOSED ERRATA
Alias: rhn-custom-info
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Garrett Holmstrom
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: F-Spacewalk space13
TreeView+ depends on / blocked
 
Reported: 2010-01-08 14:40 UTC by Miroslav Suchý
Modified: 2010-12-08 21:41 UTC (History)
5 users (show)

Fixed In Version: rhn-custom-info-5.4.5-1.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-11-23 15:20:37 UTC
Type: ---
Embargoed:
gholms: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)
Review for new package rhn-custom-info-5.4.2-1.fc14 (9.49 KB, text/plain)
2010-11-17 00:01 UTC, Garrett Holmstrom
no flags Details
Review for new package rhn-custom-info-5.4.4-1.fc15 (9.14 KB, text/plain)
2010-11-18 18:23 UTC, Garrett Holmstrom
no flags Details
Review for new package rhn-custom-info-5.4.5-1.fc15 (8.79 KB, text/plain)
2010-11-21 21:27 UTC, Garrett Holmstrom
no flags Details

Description Miroslav Suchý 2010-01-08 14:40:43 UTC
SRPM:
http://miroslav.suchy.cz/fedora/rhn-custom-info/rhn-custom-info-5.4.1-1.src.rpm
SPEC: http://miroslav.suchy.cz/fedora/rhn-custom-info/rhn-custom-info.spec

Description:
Allows for the setting and listing of custom key/value pairs for 
an RHN-enabled system.

rpmlint /tmp/spacewalk-build/rpmbuild-rhn-custom-info-5.4.1-1/noarch/rhn-custom-info-5.4.1-1.noarch.rpm
rhn-custom-info.noarch: E: explicit-lib-dependency rhnlib
1 packages and 0 specfiles checked; 1 errors, 0 warnings.

Which should be safe since rhnlib is not classic lib.

Koji scratch build:
 http://koji.fedoraproject.org/koji/taskinfo?taskID=1909325

Comment 1 Garrett Holmstrom 2010-01-16 05:07:00 UTC
I'm not an approved packager yet, so I'll give you an informal review in hopes that it will help.

See below for rpmlint output

ok - Package meets naming guidelines
ok - Spec file matches base package name
NO - Meets Packaging Guidelines
ok - License (GPLv2 and Python)
ok - License field in spec matches
ok - License file included in package
ok - Spec in American English
ok - Spec is legible
ok - Sources match upstream md5sum:
7b291f04f68beba0e1b3b4285cee92ce  rhn-custom-info-5.4.1.tar.gz
7b291f04f68beba0e1b3b4285cee92ce  rhn-custom-info-5.4.1.tar.gz.upstream

NO - BuildRequires correct
na - Spec handles locales/find_lang
na - Package has .so files in %{_libdir} and runs ldconfig in %post and %postun
ok - Package does not bundle system libs
na - Package relocatability is justified
ok - No duplicate files in %files
ok - Spec has %defattr in each %files section
NO - File permissions are sane
ok - Spec has a correct %clean section
ok - Spec has rm -rf %{buildroot} at top of %install
ok - Spec has consistant macro usage
ok - Package is code or permissible content
ok - Spec has correct buildroot
ok - File names valid UTF-8

ok - %doc files don't affect runtime
na - Headers go in -devel package
na - Static libs go in -static package
ok - Package contains no .la files
na - Package is a GUI app and has a .desktop file installed w/ desktop-file-install
ok - Package owns all directories it creates
ok - Package's files and directories don't conflict with others'
ok - Package obeys FHS, except libexecdir and /usr/target

ok - Compiles and builds on at least one arch (builds on f13 and el5)
ok - Compiles and builds on all archs or has ExcludeArch + bugs filed

SHOULD Items:

na - Query upstream for license inclusion
no - Translations of description and summary
ok - Builds in mock
na - Builds on all supported archs (noarch)
na - Scriptlets are sane
na - Non-devel subpackages require base w/ fully-versioned dependency
na - pkgconfig (.pc) files go in -devel package
ok - Latest version
ok - Has dist tag
ok - No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin

########################################

* rpmlint output

rhn-custom-info.noarch: E: explicit-lib-dependency rhnlib

This is justified in the spec file.

* Meets Packaging Guidelines

0%{?fedora} evaluates to 13 in Rawhide, so your condition on line 16 fails, causing the resulting rpm to Require up2date instead of yum-rhn-plugin.  While yum-rhn-plugin still Provides it, perhaps you can fix it by changing the "==" to a ">=" in that condition so we don't have to rely on the Provides entry sticking around for dependency solving to work.

python-optik died in el4 when Python 2.3 included it, and the Provides entry for it isn't in Fedora any more.  That makes this package impossible to install on Rawhide.  If you plan on building it for el3 I would make that Requires entry contingent on el3 or less.  Otherwise I recommend just dropping it.

* BuildRequires correct

A build that require Python should BuildRequire python-devel instead of python.

http://fedoraproject.org/wiki/Packaging:Python#BuildRequires

* File permissions are sane

A couple files have g+w permissions:
-rw-rw-r-- 2 root root 6766 Jan 15 22:41 /usr/share/rhn/custominfo/rhn-custom-info.pyc
-rw-rw-r-- 2 root root 6766 Jan 15 22:41 /usr/share/rhn/custominfo/rhn-custom-info.pyo

* Other comments

What's with the '+' characters in front of the latest changelog entry?

Comment 2 Miroslav Suchý 2010-01-18 14:14:41 UTC
All items should be addressed. Thx for your notes.

SRPM:
http://miroslav.suchy.cz/fedora/rhn-custom-info/rhn-custom-info-5.4.2-1.src.rpm
SPEC: http://miroslav.suchy.cz/fedora/rhn-custom-info/rhn-custom-info.spec

Comment 3 Garrett Holmstrom 2010-11-16 23:59:56 UTC
I meant to come back to this one, but it somehow managed to slip through a crack somewhere.  Sorry.  I see only a couple minor issues at the moment, so just fix those up and you should be good to go.  I will attach the full review shortly.

- Changelog in prescribed format

Please add a hyphen to the beginning of line 74.  Sorry for nitpicking.  ;-)

- Requires correct, justified where necessary

If you are building this against EL4 you should Require python with a versioned dependency, and then only after testing that you're building on < EL5.  If you aren't building against EL4 then you should instead drop that Requires directive altogether since rpmbuild will pick that up on its own.

Comment 4 Garrett Holmstrom 2010-11-17 00:01:13 UTC
Created attachment 460960 [details]
Review for new package rhn-custom-info-5.4.2-1.fc14

Comment 5 Miroslav Suchý 2010-11-18 12:46:07 UTC
All items should be addressed. Thx for your notes.

SRPM:
http://miroslav.suchy.cz/fedora/rhn-custom-info/rhn-custom-info-5.4.4-1.el6.src.rpm
SPEC: http://miroslav.suchy.cz/fedora/rhn-custom-info/rhn-custom-info.spec

Comment 6 Garrett Holmstrom 2010-11-18 18:23:21 UTC
Close.  An EPEL-4 Python dependency usually looks like this:

%if 0%{?rhel} <= 4
Requires: python-abi = %(%{__python} -c "import sys; print sys.version[:3]")
%endif

Comment 7 Garrett Holmstrom 2010-11-18 18:23:57 UTC
Created attachment 461352 [details]
Review for new package rhn-custom-info-5.4.4-1.fc15

Comment 8 Miroslav Suchý 2010-11-19 08:06:37 UTC
But this is not binary package. I do not need ABI in specific version... Hmm I see... I need it because python search path where it looks for modules. :(

Corrected.
SRPM:
http://miroslav.suchy.cz/fedora/rhn-custom-info/rhn-custom-info-5.4.5-1.el6.src.rpm
SPEC: http://miroslav.suchy.cz/fedora/rhn-custom-info/rhn-custom-info.spec

Comment 9 Jan Pazdziora 2010-11-19 16:19:20 UTC
Moving to space13.

Comment 10 Garrett Holmstrom 2010-11-21 21:26:55 UTC
That's exactly right.  Everything looks good to go now.  Note that when you make changes to just the spec file you don't need to create another upstream release with its own version number and tarball; you can just bump the Release field in the spec file and rebuild.

Comment 11 Garrett Holmstrom 2010-11-21 21:27:54 UTC
Created attachment 461873 [details]
Review for new package rhn-custom-info-5.4.5-1.fc15

Comment 12 Miroslav Suchý 2010-11-22 13:14:23 UTC
New Package CVS Request
=======================
Package Name: rhn-custom-info
Short Description: Set and list custom values for RHN-enabled machine
Owners: msuchy
Branches: F-14, EL-5, EL-6
InitialCC:

Comment 13 Jason Tibbitts 2010-11-22 13:51:34 UTC
Git done (by process-git-requests).

Comment 14 Fedora Update System 2010-11-22 14:14:00 UTC
rhn-custom-info-5.4.5-1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/rhn-custom-info-5.4.5-1.fc14

Comment 15 Fedora Update System 2010-11-22 14:15:00 UTC
rhn-custom-info-5.4.5-1.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/rhn-custom-info-5.4.5-1.el5

Comment 16 Garrett Holmstrom 2010-11-22 15:52:12 UTC
(In reply to comment #12)
> Branches: F-14, EL-5, EL-6

If you didn't intend on building this for EL-4 you didn't need to add the python-abi dependency at all; rpmbuild on EL-5 and later will detect that automatically.

Comment 17 Fedora Update System 2010-11-22 16:59:19 UTC
rhn-custom-info-5.4.5-1.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 rhn-custom-info'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/rhn-custom-info-5.4.5-1.el5

Comment 18 Miroslav Suchý 2010-11-23 15:20:37 UTC
> If you didn't intend on building this for EL-4

We will be building it in upstream, but I do not plan to build it for EPEL. I removed that line in F15, where it caused build problems.

Comment 19 Fedora Update System 2010-12-07 17:02:11 UTC
rhn-custom-info-5.4.5-1.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 20 Fedora Update System 2010-12-08 21:41:49 UTC
rhn-custom-info-5.4.5-1.fc14 has been pushed to the Fedora 14 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.