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-02-28 06:08 EST ------- (In reply to comment #9) > This package cannot be accepted as is; there are still must > fix issues. rpmlint is not silent. And overall many other things > to check. rpmlint is not perfect. > Here we go: > > * there is a huge pile of patches. Can't some of them be submitted > upstream? I have some remarks on some of them: No. ISC won't take everything we need in dhcp. You want to use NetworkManager? You need these packages. Before I took over the dhcp packages, there were almost 100 patches. I've reduced it to the set you see now. > - dhcp-3.0.5-ldap-configuration.patch: is the last chunk really > needed? Is this patch from fedora or was it found somwhere else? > and has it been submitted upstream? Probably not needed, but overridden by %prep anyway. The ldap patch is a very common dhcp addition floating around the net that ISC won't take. > - dhcp-3.0.5-client.patch: the comment should explain what this patch > is about. There seems to be very specific fedora things mixed up > with new features, and these new features seem to be diverse. Maybe > this patch could be split? Probably so, but considering it's taken me months to clean up the train wreck that was there, this isn't high on my priority list. It's a manageable patch set now. Splitting it back out to feature patches is a goal. Everything contained in the 'client' patch patches code in the client subdirectory and related scripts. > - first chunk of dhcp-3.0.5-common.patch is very dubious (there are other > places with similar dubious code). dubious? > Most of the patch corrects compile-time warning, but there is also > a patch for a man page describing new features. Shouldn't this part > be split out and put in the patch where the features are added? Probably, like I said it's a manageable patch set for me now and _way_ better than what we had before I took it over. > - dhcp-3.0.5-dhcpctl.patch dhcp-3.0.5-dst.patch, > dhcp-3.0.5-fix-warnings.patch, dhcp-3.0.5-minires.patch > dhcp-3.0.5-omapip.patch are mostly build fixes. Shouldn't those > patches be grouped together? Probably, but these patches were made after my grand clean up of the dhcp package. > - dhcp-3.0.5-extended-new-option-info.patch has a new script in the > beginning. Is it really clean to have it mixed with the remaining? This patch is the first new separated-out feature patch I've after cleaning up the dhcp package. It enables the features in dhclient necessary for NetworkManager to work. > - dhcp-3.0.5-includes.patch mixes build fixes, and different new > features (seems that some are in extended-new-option-info, others > in dhcp-3.0.5-client.patch My first step in cleaning up the package was to make a rollup patch for each subdirectory in the source tree and break it out from there. With almost 100 patches when I took it over, it was quite unmanageable as the patches changed things, changed them back, then changed them again later on. > - libdhcp4client and timeouts patches seems clean to me, although it > seems to me that they should be merged. Has them been > submitted upstream? Can't go upstream. > - dhcp-3.0.5-server.patch seems clean too me, but there should be > a comment describing what is done in this patch. Has this been > submitted upstream? Can't go upstream. > To summarize it seems to me that the patches should be grouped such > that each new feature is in one patch and there is a patch containing > all the build fixes. You're right, but that's not going to happen overnight. It's manageable for me now. > * Regarding the patch dhcp-3.0.5-Makefile.patch, wouldn't it be > better to override the LFLAGS make variable instead of patching? Eh...that kind of stuff doesn't matter to me. Override or patch it, the end result is the same. > And why are all those link flags added? The package owner before me did that and I have yet to remove them. > What is the aim of the dhcp-3.0.5/minires/Makefile.dist patch? To not install the incorrect dhcpctl.3 man page. > Is libdst really needed by something? What? Where are you seeing this. It's an internal library built and linked in to the client and server. > The changelog mentions bugs I am not allowed to view for those 2 > items... Yeah, there are *a lot* of RHEL requirements for the dhcp package. > * setting RPM_OPT_FLAGS to RPM_OPT_FLAGS with other options is > ugly. Don't do that spec, won't be legible. OK, I'll pass them with --copts > * there are many places where rpm macros should be used, for > sysconfdir and also others. You should also make sure that > in the code sysconfdir value is used. OK, I'll change that. > * use $RPM_BUILD_ROOT or %{buildroot} consistently Changed to %{buildroot} > * keep timestamps for data files, even those shipped in the srpm Done. > * the ln -s in scriptlet for man pages seems wrong. What is it for? The dhcp-options.5 and dhcp-eval.5 man pages apply to dhclient and dhcpd. The previous maintainer created copies of each man page for each package (dhclient and dhcpd). The symlinks created in the postinstall scriptlets are there so people will still be able to find dhcp-options and dhcp-eval by name regardless of what package is installed. I don't like this solution, but I'm leaving it as is right now because it's not that critical. > * libdhcp4client-devel should Requires: pkgconfig Done. > * Why is -fvisibility=hidden used by default? Because of the way the previous maintainer of this package wrote libdhcp4client, I need to avoid symbol collisions on a global scale. This is the easiest way to hide everything and expose only those symbols needed. Eventually this library will go away, but for now we are using it. > * In my opinion > /etc/rc.d/init.d/* > shouldn't be marked as %config Done. > * dhcp should not depend on perl Oh yeah, it used to, but I removed the perl script the previous maintainer included. Removed the perl dependency. > * -devel should requires main package with version-release Done. > Suggestion: > use %defattr(-,root,root,-) Done. > I reset the flag to ? I've made some changes, but a lot of these requests are unreasonable for the Core/Extras merge review. The dhcp package is special and while I don't like the patch layout at the moment, that's not something I want to run through really quickly. I need time to break up the features appropriately. The other style changes are fine by me and I've made those changes, but I cannot modify the patch layout before the merge. That's far too time consuming to rush through. ISC does not generally accept the patches we need for dhcp. They are unlike most open source projects and working upstream with them is _extremely_ difficult if not impossible. -- 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