[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 pertusus at free.fr  2007-04-08 07:07 EST -------
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.

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.

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.

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)

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

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

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.

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

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

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

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

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

* 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...
  - exit 0 is not very pretty
  
  Not a big deal anyway.

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

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

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

* 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


I don't comment on the rpmlint warning and errors that are ignorable
in my opinion.

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