Bug 226475

I'd be happy to review this package. Look for a full review in a few. 

See below - Package meets naming and packaging guidelines
See below - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
7d5d61c026122ab791ac04c8a84db967  sysvinit-2.86.tar.gz
7d5d61c026122ab791ac04c8a84db967  sysvinit-2.86.tar.gz.1
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
See below - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.

See below - Headers/static libs in -devel subpackage.

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
See below - Should have dist tag
OK - Should package latest version
5 bugs - check for outstanding bugs on package.

Issues:

1. Ok, I have to ask... why does this package use StUdLyCaps?
The upstream package is called 'sysvinit'. I know it's been that way
forever, but perhaps we could fix that now?

2. Might include the LICENSE file, which is not the GPL, but at least explains
that this package is released under the GPL and where to get it.
I suppose you could also bug upstream about including a copy.

3. Use the approved buildroot:
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

4. There is a single include file shipped here in the main package:

/usr/include/initreq.h

It seems useless to make a -devel package for one header, but does it make
any sense to ship it at all? Perhaps that should just get dropped?

5. Our pal rpmlint says:
a)
W: SysVinit summary-ended-with-dot Programs which control basic system processes.

Suggest: remove . at the end of the summary?

b)
W: SysVinit no-url-tag

Suggest: add "URL: ftp://ftp.cistron.nl/pub/people/miquels/sysvinit/"
Not very informative, but better than nothing...

c)
E: SysVinit setgid-binary /usr/bin/wall tty 02555
E: SysVinit non-standard-executable-perm /usr/bin/wall 02555
E: SysVinit non-standard-executable-perm /usr/bin/wall 02555

Suggest: ignore, these are needed.

d)
W: SysVinit devel-file-in-non-devel-package /usr/include/initreq.h

Suggest: Stop shipping this? Or ignore.

e)
W: SysVinit dangerous-command-in-%post ln

Suggest: Don't see an easy way to avoid the ln. Do you?

6. Upstream seems not very active (last release 2004), but would it still
be worth trying to push some of these patches upstream? There are a lot of
them here...

7. Minor: Might replace the /usr/bin in %files with %{_bindir}

8. Minor: add dist tag?

9. The 5 outstanding bugs on this package don't seem to be package
related, but you might take a quick look at them to see if they can
easily be solved while making the above changes?

(In reply to comment #2)
> Issues:
> 
> 1. Ok, I have to ask... why does this package use StUdLyCaps?
> The upstream package is called 'sysvinit'. I know it's been that way
> forever, but perhaps we could fix that now?

History. I suppose we could change it. Would need the usual
obsoletes/provides stuff.

> 2. Might include the LICENSE file, which is not the GPL, but at least explains
> that this package is released under the GPL and where to get it.
> I suppose you could also bug upstream about including a copy.

Done.

> 3. Use the approved buildroot:
>       %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

Yeah, was already fixed in CVS. Should have built that.

> 4. There is a single include file shipped here in the main package:
> 
> /usr/include/initreq.h
> 
> It seems useless to make a -devel package for one header, but does it make
> any sense to ship it at all? Perhaps that should just get dropped?

See bug 119039; it's just a structure definition.

> 5. Our pal rpmlint says:
> a)
> W: SysVinit summary-ended-with-dot Programs which control basic system processes.
> b)
> W: SysVinit no-url-tag

Fixed.

> e)
> W: SysVinit dangerous-command-in-%post ln
> 
> Suggest: Don't see an easy way to avoid the ln. Do you?

Taken out and shot. initrunlvl hasn't been supported by init for nearly 4 years.
Oops.

> 6. Upstream seems not very active (last release 2004), but would it still
> be worth trying to push some of these patches upstream? There are a lot of
> them here...

Most everything has been sent at one point or another.

> 7. Minor: Might replace the /usr/bin in %files with %{_bindir}

Would have to patch the makefiles. 

> 8. Minor: add dist tag?

Well, we never rebase/update it, so, not sure what it gains.

New stuff uploaded at http://people.redhat.com/notting/review/

1. I would really like to see it changed to match upstream myself... ;) 

2. good. ok. 

3. good. ok. 

4. ok. fine. 

5. a) and b) and e) good. ok. 

6. ok, so I don't suppose it's worth trying to push them again?

7. yeah, thats a minor issue, so it can be ignored I suppose. 

8. Agreed, thats minor and not a blocker. 

So, the only outstanding issue I see is the rename? 
Would you be willing to do that? or would you prefer not to?

Sure, why not? New stuff uploaded at http://people.redhat.com/notting/review/ -
please double-check.

Note that I'm *not* going to commit the rename until the package is moved -
realistically, if we're doing a rename, it should be a new CVS module with the
new name, etc., and I'd only rather do that once.

ok, that version looks good to me. Thanks for the rename. 
Everything else looks good, and I see no further blockers, so this package is
APPROVED. 

Thanks again for the speedy fixes. 

Per the new offical review guidelines, I am reassigning to me (the reviewer). 
I will leave this open until it's imported over. 

Fixed in -17.