Bug 225641 - Merge Review: chkconfig
Merge Review: chkconfig
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ruben Kerkhof
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 12:49 EST by Nobody's working on this, feel free to take it
Modified: 2012-12-22 19:35 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-17 02:41:40 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ruben: fedora‑review+


Attachments (Terms of Use)
allow to override all the paths (5.47 KB, patch)
2007-02-05 18:21 EST, Patrice Dumas
no flags Details | Diff
updated patch (6.37 KB, patch)
2007-02-05 18:29 EST, Patrice Dumas
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 12:49:35 EST
Fedora Merge Review: chkconfig

http://cvs.fedora.redhat.com/viewcvs/devel/chkconfig/
Initial Owner: notting@redhat.com
Comment 1 Ruben Kerkhof 2007-02-03 16:49:34 EST
* 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
Comment 2 Bill Nottingham 2007-02-03 23:49:11 EST
(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/

Comment 3 Ruben Kerkhof 2007-02-04 06:27:19 EST
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

Comment 4 Patrice Dumas 2007-02-04 07:48:16 EST
(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?
Comment 5 Bill Nottingham 2007-02-04 13:19:08 EST
(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...
Comment 6 Ruben Kerkhof 2007-02-04 14:02:27 EST
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.
Comment 7 Patrice Dumas 2007-02-04 17:40:38 EST
(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.
Comment 8 Bill Nottingham 2007-02-05 15:04:10 EST
> 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).
Comment 9 Bill Nottingham 2007-02-05 15:12:15 EST
(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.
Comment 10 Ruben Kerkhof 2007-02-05 15:58:53 EST
(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)
Comment 11 Bill Nottingham 2007-02-05 17:14:46 EST
Spec and tarball uploaded.
Comment 12 Bill Nottingham 2007-02-05 17:15:55 EST
Whoops. Spec and tarball uploaded to http://people.redhat.com/notting/review/. :)

Comment 13 Patrice Dumas 2007-02-05 18:21:57 EST
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.
Comment 14 Patrice Dumas 2007-02-05 18:29:33 EST
Created attachment 147418 [details]
updated patch

The previous one was wrong...
Comment 15 Patrice Dumas 2007-02-05 18:34:56 EST
The dependency on chkconfig should certainly be fully versionned:

Requires: chkconfig = %{version}-%{release}
Comment 16 Bill Nottingham 2007-02-06 11:29:51 EST
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.)
Comment 17 Ruben Kerkhof 2007-02-06 16:28:02 EST
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.
Comment 18 Bill Nottingham 2007-02-06 21:44:48 EST
Uploaded.
Comment 19 Ruben Kerkhof 2007-02-07 15:50:23 EST
Thanks.

I don't see any blockers, so this package is approved.
Comment 20 Ruben Kerkhof 2007-03-18 06:05:33 EDT
Assigning ticket to myself as per http://fedoraproject.org/wiki/PackageReviewProcess
Comment 21 Maurizio 2012-12-22 19:35:36 EST
cvs fails unrecognized

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