[Bug 225691] Merge Review: dhcp

[bugzilla at 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: dhcp


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225691





------- Additional Comments From dcantrell at redhat.com  2007-04-11 16:11 EST -------
(In reply to comment #19)
> (In reply to comment #18)
> 
> > /var/run is documented in the LFS as the location for these files.  There should
> > be no reason that we need rpmbuild to provide that information at build time.
> > 
> > Technically, I should be using /var/run/dhclient, but that's a patch for after
> > Fedora 7.
> 
> I think that even if it is specified in the FHS, it is nice to
> let a user rebuilding the package be able to specify completely
> its install macros. 
> 
> Moreover if it is a patch that should be submitted upstream it is 
> even more important to be able to specify another location
> at build time. Not a blocker for the review in any case.

I'll look in to this post-F7.

> > > Issues:
> > > W: dhcp strange-permission linux.dbus-example 0775
> > > W: dhcp strange-permission dhcpd-conf-to-ldap 0775
> > > W: dhcp strange-permission linux 0775
> > > This should be fixed if possible.
> > 
> > Done.
> 
> No, it is not fixed, because it is the perms in the src.rpm,
> but I think it cannot be fixed (because it is in cvs already).

I removed them and re-added them to CVS with 0644 permissions.

> > > If it is really the case, you can fix the following
> > > W: dhcp mixed-use-of-spaces-and-tabs (spaces: line 11, tab: line 324)
> > 
> > I'm not seeing this.
> 
> In fact it is line 654 now, in the changelog:
> - fix bug 176615: fix DDNS update when Windows-NT client sends
>                   host-name with trailing nul
> ^^^^^^^^

I fixed up line 654 and a few other lines with tabs that shouldn't have had them
(previous maintainer).

> > IMHO, all mention of dhcpcd should be removed from the spec file anyway.  That
> > package hasn't been around for many years.
> 
> I think it is nice to keep Obsoletes/Provides for more than many 
> years, when they are properly versionned. Still not a blocker, though.

An Obsoletes is fine.  But what are you suggesting for a Provides line?

> > >   - exit 0 is not very pretty
> > 
> > Not that it matters, but why is it not pretty?  It's pointless since the shell
> > is already doing that, but why is it not pretty?
> 
> It is not pointless, it is certainly there to for an exit code
> of 0 even if the preceding command had an exit code different from
> 0. I say it is not pretty because in my opinion it is better to 
> avoid having an exit code of 1 in the first place.

Right, exit 0 forces a clean exit of the scriptlet which meant a failure of the
preceeding operation doesn't matter.  Using 'exit 0' is common practice for
this, but we're really just splitting hairs here because we're both advocating
the same thing...avoid exiting with a value other than 0.  What's there now
accomplishes that as did what was there before.

> * source match upstream, but source timestamp different.
>   this should be fixed next time (by using spectoo -g, wget 
>   -N or the like).
> -rw-rw-r-- 1 dumas dumas 876591 nov  7 16:28 dhcp-3.0.5.tar.gz
> -rw-rw-r-- 1 dumas dumas 876591 nov  6 00:01 dhcp-3.0.5.tar.gz-orig
> 
> ce5d30d4645e4eab1f54561b487d1ec7  dhcp-3.0.5.tar.gz

I can't fix that.  The archive is the build system's lookaside cache and since
the checksum isn't changing, I can't override it.  If it really is a *big* deal,
I can look in to getting it removed and reupload it to the lookaside cache.

> Minor:
> according to the newest guideline, the *.a should be in 
> a distinct package, named, for example 
> dhcp-static-devel or the like, with 
> Requires: %{name}-devel = %{epoch}:%{version}-%{release}

Done.  Created dhcp-devel-static and libdhcp4client-devel-static.

> APPROVED

Hooray.

> Reassigning to me since it should be assigned to reviewer.
> Michael, you can reassign to you since you did the first review.

I think you should stay as the reviewer for this package.  You seem to have done
a bulk of the reviewing.

Also, I still have other reviews that no one has touched (225690, 225692,
225853, 226230, and 226337  :)

-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.