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@xxxxxxx 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review