* RPM name is OK
* This is the latest version
* Builds fine in mock
* File list of chkconfig looks OK
* File list of ntsysv looks OK

Needs work:
* BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
  (wiki: PackagingGuidelines#BuildRoot)
* Missing SMP flags. If it doesn't build with it, please add a comment
  (wiki: PackagingGuidelines#parallelmake)
* Spec file: some paths are not replaced with RPM macros
  (wiki: QAChecklist item 7)

Minor:
* Duplicate BuildRequires: newt (by newt-devel)

Notes:
* Please use {?dist} in the Release tag
* I'm not sure what BuildPrereq does, but can you use BuildReq?
* Can you use make DESTDIR instead of make instroot
* Replace /etc with %{_sysconfdir} and /usr/sbin with %{_sbindir}

rpmlint output is not silent:

[ruben@odin chkconfig]$ cat rpmlint-srpm.log 
W: chkconfig no-url-tag
W: chkconfig buildprereq-use newt newt-devel gettext
W: chkconfig macro-in-%changelog pre
W: chkconfig macro-in-%changelog pre

(In reply to comment #1)
> Needs work:
> * BuildRoot should be
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

Fixed upstream.

>   (wiki: PackagingGuidelines#BuildRoot)
> * Missing SMP flags. If it doesn't build with it, please add a comment

Sure! Although, there's only 4 source files. Can't speed it up
*that* much. :)

>   (wiki: PackagingGuidelines#parallelmake)
> * Spec file: some paths are not replaced with RPM macros

/etc, /usr/sbin are compiled in. Having it as a macro can't really help.

> Minor:
> * Duplicate BuildRequires: newt (by newt-devel)

Fixed.

> Notes:
> * Please use {?dist} in the Release tag

It's not really ever rebased between releases.

> * I'm not sure what BuildPrereq does, but can you use BuildReq?

It's a synonym. Switched, though.

> * Can you use make DESTDIR instead of make instroot

Fixed upstream.

> * Replace /etc with %{_sysconfdir} and /usr/sbin with %{_sbindir}

See above.

> [ruben@odin chkconfig]$ cat rpmlint-srpm.log 
> W: chkconfig no-url-tag

No URL to have.

> W: chkconfig buildprereq-use newt newt-devel gettext
> W: chkconfig macro-in-%changelog pre
> W: chkconfig macro-in-%changelog pre

Fixed.

Uploaded spec @ http://people.redhat.com/notting/review/

Hi Bill,

I just tried a mock build from the spec at http://people.redhat.com/notting/review/, and it failed.

+ make DESTDIR=/var/tmp/chkconfig-1.3.32-1-root-mockbuild MANDIR=/usr/share/man install
[ -d //sbin ] || mkdir -p //sbin
[ -d //usr/sbin ] || mkdir -p //usr/sbin
[ -d //usr/share/man ] || mkdir -p //usr/share/man
[ -d //usr/share/man/man8 ] || mkdir -p //usr/share/man/man8
[ -d //usr/share/man/man5 ] || mkdir -p //usr/share/man/man5
[ -d //var/lib/alternatives ] || mkdir -p -m 755 //var/lib/alternatives
[ -d //etc/alternatives ] || mkdir -p -m 755 //etc/alternatives
install -m 755 chkconfig //sbin/chkconfig
install: cannot remove `//sbin/chkconfig': Permission denied
make: *** [install] Error 1
error: Bad exit status from /var/tmp/rpm-tmp.20880 (%install)

So we can either stick with instroot, or replace instroot with DESTDIR in the Makefile.
One issue is that DESTDIR is not passed through to the Makefile in the po subdirectory.
Please let me know what you think.

Ok, in reply to comment #2:
> /etc, /usr/sbin are compiled in. Having it as a macro can't really help.

They're defined as variables in the makefile.
You could use make DESTDIR=$RPM_BUILDROOT ALTDIR=%{_sysconfdir}/alternatives etc.. but I'm not 
sure it's worth the effort.  Unless we're planning on moving /etc to another directory in FC-8 ;-)

> It's not really ever rebased between releases.

Ok

> No URL to have.

Ok, the URL can be added after the merge, when the tarball is available from the Fedora repo.

Thanks,

Ruben

(In reply to comment #2)

> /etc, /usr/sbin are compiled in. Having it as a macro can't really help.

This shouldn't be so. Can't that be corrected?

(In reply to comment #4)
> (In reply to comment #2)
> 
> > /etc, /usr/sbin are compiled in. Having it as a macro can't really help.
> 
> This shouldn't be so. Can't that be corrected?

I'm not really planning to port the package to autoconf just so /etc could be
moved...

Bill, have you decided yet if you go with DESTDIR or instroot?
It's not a blocker but if you change instroot to DESTDIR you have to adjust some things to let it build in a 
chroot.

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > 
> > > /etc, /usr/sbin are compiled in. Having it as a macro can't really help.
> > 
> > This shouldn't be so. Can't that be corrected?
> 
> I'm not really planning to port the package to autoconf just so /etc could be
> moved...

This isn't needed. Carefully chosen make variables overriden when
running make can do the same.

> It's not a blocker but if you change instroot to DESTDIR you have to adjust
some things to let it build in a 
> chroot.

As stated, it's already fixed in CVS (upstream).

(In reply to comment #7)
> This isn't needed. Carefully chosen make variables overriden when
> running make can do the same.

For /usr/sbin, yes, and that I've fixed in CVS. I have no intention of replacing
/etc with a macro, as that really *is not changable*. It's a fixed path in the
LSB, and it's compiled into the binary.

(In reply to comment #8)

Would it be possible to upload the new spec?
It's not available at http://cvs.fedora.redhat.com/viewcvs/devel/chkconfig/ (yet)

Spec and tarball uploaded.

Whoops. Spec and tarball uploaded to http://people.redhat.com/notting/review/. :)

Created attachment 147417 [details]
allow to override all the paths

I think that it is important to be able to override the
paths when testing a software, such that it is possible
to do things without messing up the system, or without
administrator rights.

Created attachment 147418 [details]
updated patch

The previous one was wrong...

The dependency on chkconfig should certainly be fully versionned:

Requires: chkconfig = %{version}-%{release}

That patch is an issue for upstream, not review.

Release added to requires, although if the translations break in an incompatible
way between a -1 and -2, we've got bigger issues. (That's why the requires is
there.)

I had another look from the spec included in http://people.redhat.com/notting/review/
chkconfig-1.3.33.tar.gz , and it looks ok to me.

Can you please upload your new tarball with the fully versioned Requires?

Patrice is right, it is a MUST, althought the guidelines say "In the vast majority of cases". But I guess that's 
why it's called a guideline.

Uploaded.

Thanks.

I don't see any blockers, so this package is approved.

Assigning ticket to myself as per http://fedoraproject.org/wiki/PackageReviewProcess

cvs fails unrecognized