[Bug 225691] Merge Review: dhcp

Mon Apr 9 21:45:33 UTC 2007

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-09 17:45 EST -------
(In reply to comment #17)
> Good job. It is right for the patches now.
> 
> I have a remark on the release-by-ifup patch, maybe it would
> be better not to hardcode /var in /var/run/..., but allow to
> override /var with something like localstatedir.

/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.

> Even if ISC releases are infrequent, it is better to have those patches
> submitted upstream. Also if upstream release are infrequent, you may want
> to coordinate with other big distros for non-fedora specific patches.

Agreed, but I was just wanting to point out that you will likely always see a
large patch set for dhcp, unfortunately.  They accepted things from us 2 years
ago and we have yet to see them in a new release of dhcp.  However, having it
accepted upstream is still an indication of at least some level of communication
with the upstream developers.  I am working on submitting patches we have upstream.

Collaboration with other distributors is something I'd like to get to, but it's
a bit disjoint at the moment.  Something I'd like to improve, but I need to
approach it carefully.

> Now more on the packaging itself:
> 
> 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.

> 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.

> W: dhcp wrong-file-end-of-line-encoding
> /usr/share/doc/dhcp-3.0.5/contrib/ms2isc/readme.txt
> W: dhcp wrong-file-end-of-line-encoding
> /usr/share/doc/dhcp-3.0.5/contrib/ms2isc/Registry.perlmodule
> W: dhcp wrong-file-end-of-line-encoding
> /usr/share/doc/dhcp-3.0.5/contrib/ms2isc/ms2isc.pl
> This may be fixed by 
> sed -i -e 's/\r//' ....

Done.

> W: dhclient summary-ended-with-dot Provides the dhclient ISC DHCP client daemon
> and dhclient-script .
> Just remove the dot

Done.

> W: dhcp-devel spurious-executable-perm /usr/share/man/man3/omshell.3.gz
> W: dhcp-devel spurious-executable-perm /usr/share/man/man3/dhcpctl.3.gz
> W: dhcp-devel spurious-executable-perm /usr/share/man/man3/omapi.3.gz
> You can fix this like how you do for other man pages.

Done.

> W: libdhcp4client-devel no-dependency-on libdhcp4client
> I guess this should be fixed

Done.

> E: dhclient obsolete-not-provided dhcpcd
> Certainly the Provides could be the next version after obsoletion

IMHO, all mention of dhcpcd should be removed from the spec file anyway.  That
package hasn't been around for many years.

> Suggestions:
> * use wildcards for man patches to catch no compression and other 
>   compression modes, like
> %attr(0644,root,root) %{_mandir}/man1/omshell.1*

I don't like that.  I prefer to explicitly list the files I'm including, that
way I have no surprises when it comes time to rebase the package on a new release.

> * In the patch name, don't use %{version}, but instead hardcode the
>   current version when the patch was added, it serves an historical
>   purpose like that.

Done.

> * prefix Source files with simple names with dhcp- to disambiguate
>   from other files in the SOURCE dir (in my opinion this is relevant for
>   README.ldap linux.dbus-example linux Makefile.dist).

I don't like that.  I prefer to keep the source file names exactly as they will
appear in the source tree.  No munging.

> * your scriptlets seem right to me but I suggest using the standard
>   scriptlets, if only for consistency
>
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-97754e2c646616c5f6222f0cfc6923c60765133e
> 
>   Pros of the standard scriptlets may be 
>   - using paths for /sbin/service and in the corresponding requires makes
>     it more robust with regard with package rename/split/merge...

Changed the scriptlets to match the packaging guidelines.

>   - 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?

> * I think that messing with files in %prep when it is not to fix 
>   issues with upstream bad packaging (like CVS dirs, executable source 
>   files) is not right, therefore I think that
> # Ensure we don't pick up Perl as a dependency from the scripts and modules
> # in the contrib directory (we copy this to /usr/share/doc in the final
> # package).
> %{__chmod} -x contrib/3.0b1-lease-convert
> %{__chmod} -x contrib/dhcpd-conf-to-ldap
> %{__cp} -p contrib/ms2isc/Registry.pm contrib/ms2isc/Registry.perlmodule
> %{__rm} -f contrib/ms2isc/Registry.pm
>   should be in %install.

I'm not a big fan of this approach.  I prefer the %prep section to be all things
necessary to prepare the source tree for compilation and installation.

>   Moreover I don't like to modify the original dirs when it is only to
>   cope with fedora specific stuff. You may not find it right, but I 
>   would have done:
> 
> rm -rf __fedora_contrib
> mkdir  __fedora_contrib
> cp -a contrib __fedora_contrib
> %{__chmod} -x __fedora_contrib/contrib/3.0b1-lease-convert
> %{__chmod} -x __fedora_contrib/contrib/dhcpd-conf-to-ldap
> %{__cp} -p __fedora_contrib/contrib/ms2isc/Registry.pm
> contrib/ms2isc/Registry.perlmodule
> %{__rm} -f __fedora_contrib/contrib/ms2isc/Registry.pm
>
> (as a side note the cp followed by a rm should better be a mv in
> my opinion).

Changed.

> And then in %doc, use
> %doc __fedora_contrib/contrib

Done.

> * Unless I am wrong rpm spec variables substitution happens
>   before anything else so the following is simpler and works too:
> %{__sed} 's/@DHCP_VERSION@/%{version}/' < %SOURCE5 > libdhcp4client.pc

Done.

Now, I've committed these changes to CVS, but have not built a new set of
packages yet.  I already rebuilt dhcp today to fix a dhclient problem.  I want
that to land in rawhide before pushing a new dhcp out.  So expect a rebuild of
dhcp tomorrow to include these spec changes.  For now you can look at it in CVS.

-- 
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.