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