Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
Summary: Merge Review: chkconfig Product: Fedora Extras Version: devel Platform: All OS/Version: Linux Status: NEW Severity: normal Priority: normal Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: nobody@fedoraproject.org QAContact: fedora-package-review@redhat.com CC: notting@redhat.com
Fedora Merge Review: chkconfig
http://cvs.fedora.redhat.com/viewcvs/devel/chkconfig/ Initial Owner: notting@redhat.com
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
ruben@rubenkerkhof.com changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|nobody@fedoraproject.org |ruben@rubenkerkhof.com Flag| |fedora-review?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
ruben@rubenkerkhof.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|ruben@rubenkerkhof.com |notting@redhat.com CC| |ruben@rubenkerkhof.com Flag|fedora-review? |fedora-review-
------- Additional Comments From ruben@rubenkerkhof.com 2007-02-03 16:49 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From notting@redhat.com 2007-02-03 23:49 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/
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From ruben@rubenkerkhof.com 2007-02-04 06:27 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From pertusus@free.fr 2007-02-04 07:48 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From notting@redhat.com 2007-02-04 13:19 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...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From ruben@rubenkerkhof.com 2007-02-04 14:02 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From pertusus@free.fr 2007-02-04 17:40 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From notting@redhat.com 2007-02-05 15:04 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).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From notting@redhat.com 2007-02-05 15:12 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From ruben@rubenkerkhof.com 2007-02-05 15:58 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)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From notting@redhat.com 2007-02-05 17:14 EST ------- Spec and tarball uploaded.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From notting@redhat.com 2007-02-05 17:15 EST ------- Whoops. Spec and tarball uploaded to http://people.redhat.com/notting/review/. :)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From pertusus@free.fr 2007-02-05 18:21 EST ------- Created an attachment (id=147417) --> (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=147417&action=vie...) 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
pertusus@free.fr changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #147417|0 |1 is obsolete| |
------- Additional Comments From pertusus@free.fr 2007-02-05 18:29 EST ------- Created an attachment (id=147418) --> (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=147418&action=vie...) updated patch
The previous one was wrong...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From pertusus@free.fr 2007-02-05 18:34 EST ------- The dependency on chkconfig should certainly be fully versionned:
Requires: chkconfig = %{version}-%{release}
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From notting@redhat.com 2007-02-06 11:29 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.)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From ruben@rubenkerkhof.com 2007-02-06 16:28 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From notting@redhat.com 2007-02-06 21:44 EST ------- Uploaded.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
ruben@rubenkerkhof.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review- |fedora-review+
------- Additional Comments From ruben@rubenkerkhof.com 2007-02-07 15:50 EST ------- Thanks.
I don't see any blockers, so this package is approved.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
------- Additional Comments From ruben@rubenkerkhof.com 2007-03-18 06:05 EST ------- Assigning ticket to myself as per http://fedoraproject.org/wiki/PackageReviewProcess
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
ruben@rubenkerkhof.com changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|notting@redhat.com |ruben@rubenkerkhof.com
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: chkconfig
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225641
bugzilla@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |medium Priority|normal |medium
ruben@rubenkerkhof.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |RAWHIDE
package-review@lists.fedoraproject.org