[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


pertusus at free.fr changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-review+              |fedora-review?




------- Additional Comments From pertusus at free.fr  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.