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 pertusus@xxxxxxx changed: What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review+ |fedora-review? ------- Additional Comments From pertusus@xxxxxxx 2007-02-27 19:22 EST ------- This package cannot be accepted as is; there are still must fix issues. rpmlint is not silent. And overall many other things to check. 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: - 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? - 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? - first chunk of dhcp-3.0.5-common.patch is very dubious (there are other places with similar dubious code). 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? - 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? - 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? - 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 - libdhcp4client and timeouts patches seems clean to me, although it seems to me that they should be merged. Has them been submitted 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? 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. * Regarding the patch dhcp-3.0.5-Makefile.patch, wouldn't it be better to override the LFLAGS make variable instead of patching? And why are all those link flags added? What is the aim of the dhcp-3.0.5/minires/Makefile.dist patch? Is libdst really needed by something? The changelog mentions bugs I am not allowed to view for those 2 items... * setting RPM_OPT_FLAGS to RPM_OPT_FLAGS with other options is ugly. Don't do that spec, won't be legible. * 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. * use $RPM_BUILD_ROOT or %{buildroot} consistently * keep timestamps for data files, even those shipped in the srpm * the ln -s in scriptlet for man pages seems wrong. What is it for? * libdhcp4client-devel should Requires: pkgconfig * Why is -fvisibility=hidden used by default? * In my opinion /etc/rc.d/init.d/* shouldn't be marked as %config * dhcp should not depend on perl * -devel should requires main package with version-release Suggestion: use %defattr(-,root,root,-) I reset the flag to ? -- 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