Bug 541072 - Review Request: certmonger - certificate status monitor and PKI enrollment client
Summary: Review Request: certmonger - certificate status monitor and PKI enrollment cl...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rob Crittenden
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-11-24 22:10 UTC by Nalin Dahyabhai
Modified: 2010-02-06 00:07 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-01-27 16:08:10 UTC
Type: ---
Embargoed:
rcritten: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Nalin Dahyabhai 2009-11-24 22:10:41 UTC
Spec URL: http://nalin.fedorapeople.org/certmonger.spec
SRPM URL: http://nalin.fedorapeople.org/certmonger-0.13-1.src.rpm
Description: Certmonger is a service which is primarily concerned with getting your system enrolled with a certificate authority (CA) and keeping it enrolled.

Comment 1 Nalin Dahyabhai 2009-12-08 18:22:06 UTC
Please check http://nalin.fedorapeople.org/ (or http://certmonger.fedorahosted.org/) for updated versions as development continues.  Thanks!

Comment 2 Rob Crittenden 2010-01-25 13:42:56 UTC
rpmlint has a few warnings that should be addressed:

% rpmlint -iv ../RPMS/x86_64/certmonger-0.17-1.fc12.x86_64.rpm 
certmonger.x86_64: I: checking
certmonger.x86_64: W: conffile-without-noreplace-flag /etc/dbus-1/system.d/certmonger.conf
A configuration file is stored in your package without the noreplace flag. A
way to resolve this is to put the following in your SPEC file:
%config(noreplace) /etc/your_config_file_here

certmonger.x86_64: W: missing-lsb-keyword Default-Stop in /etc/rc.d/init.d/certmonger
The package contains an init script that does not contain one of the LSB init
script comment block convention keywords that are recommendable for all init
scripts.  If there is nothing to add to a keyword's value, include the keyword
in the script with an empty value.  Note that as of version 3.2, the LSB
specification does not mandate presence of any keywords.

1 packages and 0 specfiles checked; 0 errors, 2 warnings.

The first error is this in the spec file:

%config /etc/dbus-1/system.d/*

I looked at a couple of other packages that put files there and they simply don't mark them as config files. Not sure who is right here. In any case this should use the %{_sysconfdir} macro instead of /etc

For the second I guess you just need to add Default-Start and Default-Stop to the init file.

A suggestion not related to the review: rather than %if 0 for pulling in the BuildRequires for the make check stage would a variable that could be set from rpmbuild make this easier?

Comment 3 Nalin Dahyabhai 2010-01-25 18:28:21 UTC
(In reply to comment #2)
> rpmlint has a few warnings that should be addressed:
> 
> % rpmlint -iv ../RPMS/x86_64/certmonger-0.17-1.fc12.x86_64.rpm 
> certmonger.x86_64: I: checking
> certmonger.x86_64: W: conffile-without-noreplace-flag
> /etc/dbus-1/system.d/certmonger.conf
> A configuration file is stored in your package without the noreplace flag. A
> way to resolve this is to put the following in your SPEC file:
> %config(noreplace) /etc/your_config_file_here
> 
> certmonger.x86_64: W: missing-lsb-keyword Default-Stop in
> /etc/rc.d/init.d/certmonger
> The package contains an init script that does not contain one of the LSB init
> script comment block convention keywords that are recommendable for all init
> scripts.  If there is nothing to add to a keyword's value, include the keyword
> in the script with an empty value.  Note that as of version 3.2, the LSB
> specification does not mandate presence of any keywords.
>
> 1 packages and 0 specfiles checked; 0 errors, 2 warnings.
> 
> The first error is this in the spec file:
> 
> %config /etc/dbus-1/system.d/*
> 
> I looked at a couple of other packages that put files there and they simply
> don't mark them as config files. Not sure who is right here. In any case this
> should use the %{_sysconfdir} macro instead of /etc

I guess if I'm reasonably certain that the names of the interfaces won't change, the D-Bus configuration files being (noreplace) won't become an issue.  Switching to %{_sysconfdir} and (noreplace).

> For the second I guess you just need to add Default-Start and Default-Stop to
> the init file.

An empty Default-Start/Default-Stop in the init script doesn't jive with the packaging guidelines (Packaging:SysVInitScript) on the wiki.  For the moment I'm assuming that the guidelines trump rpmlint.

> A suggestion not related to the review: rather than %if 0 for pulling in the
> BuildRequires for the make check stage would a variable that could be set from
> rpmbuild make this easier?    

I guess that'd work.  The new logic should make the dependencies and the entire %check section conditional on whether or not the 'check' macro is defined, which can be turned on with '--with check' at rpmbuild-time.

.spec file updated.

Comment 4 Rob Crittenden 2010-01-25 19:08:55 UTC
Review:
+ package builds in mock.
+ rpmlint is silent for SRPM
+ rpmlint warns about Default-Start/Default-Stop in RPM but I agree with Nalin that this is correct per the packaging guidelines: "More commonly, the service does not start by default in any runlevel. In this case, the line should be omitted."
+ source files match upstream (sha256sum)
0e1f0e966c17c2e6058ce7152cbb9d167ae42065ec9c45694ca7c75629ff98a8  certmonger-0.17.tar.gz
+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written
+ Spec file is written in American English.
+ Spec file is legible.
+ dist tag is present.
+ build root is correct.
+ license is open source-compatible.
+ License text is included in package.
+ %doc files present.
+ BuildRequires are proper.
+ defattr usage is correct.
+ %clean is present.
+ package installed properly.
+ Macro use consistent.
+ Compiler flags are honored correctly.
+ Package contains code.
+ no static libraries.
+ no .pc file present.
+ no .la files.
+ no translations are available.
+ Does owns the directories it creates.
+ no duplicates in %files.
+ file permissions are appropriate.
+ no scriptlets are used.
+ Not a GUI app.
+ buildroot is cleaned in %install

APPROVED.

Comment 5 Nalin Dahyabhai 2010-01-25 19:28:14 UTC
New Package CVS Request
=======================
Package Name: certmonger
Short Description: certificate monitor and PKI enrollment client
Owners: nalin
Branches: F-11 F-12
InitialCC:

Comment 6 Jason Tibbitts 2010-01-27 04:55:10 UTC
CVS done (by process-cvs-requests.py).

Comment 7 Nalin Dahyabhai 2010-01-27 16:08:10 UTC
Package checked in to source control, closing this bug.  Thanks, everyone!

Comment 8 Fedora Update System 2010-01-27 16:26:14 UTC
certmonger-0.17-2.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/certmonger-0.17-2.fc11

Comment 9 Fedora Update System 2010-01-27 16:26:20 UTC
certmonger-0.17-2.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/certmonger-0.17-2.fc12

Comment 10 Fedora Update System 2010-02-05 23:52:50 UTC
certmonger-0.17-2.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 11 Fedora Update System 2010-02-06 00:07:22 UTC
certmonger-0.17-2.fc11 has been pushed to the Fedora 11 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.