[Bug 225691] Merge Review: dhcp

Wed Feb 28 22:59:58 UTC 2007

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 at free.fr  2007-02-28 17:59 EST -------
(In reply to comment #10)

> rpmlint is not perfect.

Indeed, but its output should be shown in review and commented. This
hasn't been done for binary packages.

> >   - 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.

Is there an url for that patch?

> >   - first chunk of dhcp-3.0.5-common.patch is very dubious (there are other
> >     places with similar dubious code). 
> 
> dubious?

a variable is set but the value is never used??

> >   - 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.

Ok. This patch seems clean to me. However I am not convinced that
having the script created by the patch is the best approach. Isn't
that script provided on the net? Shouldn't it be a Source instead?
It is not a blocker but a suggestioon.

> >   - 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.

And about merging them?

> You're right, but that's not going to happen overnight.  It's manageable for
me now.

In my opinion the patchset is not acceptable as is -- even though
I believe you when you say that it was worse before and it is now
manageable. All the changes I ask and you agreed upon are required,
in my opinion, in order to make those patches easier to understand
and review. So to me it is a must fix. Still I understand 
perfectly that this issue may be low priority for you and I guess
you have enough work already. But I wouldn't let such a patchset
enters former fedora extras so it is the same here. However there
is no need to hurry, there is has never been a time constraint on
fedora review, package quality is the most important thing, and
clearly this issue is not easy to solve. So, my opinion is that
you should just leave this as is until you find time to clean things.
The package won't be approved until them, but since it is already
in fedora it is not a big deal. Another possibility would be for 
you to ask some help from the community, I guess that dhcp is a
very popular package, and it could be possible that somebody helps
you to fix that issue.

> > * 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.

Indeed, but here the same change is done 5 time in the patch,
there is no nice comment in the spec file to explain where the
flags come from, and RPM_OPT_FLAGS is conveniently used in the 
spec. The end result is the same, but in my opinion it would
be more elegant to change LFLAGS on the command line. I won't
make it a blocker.

> >   And why are all those link flags added?
> 
> The package owner before me did that and I have yet to remove them.

That is a blocker (or need an explanation).

> >   What is the aim of the dhcp-3.0.5/minires/Makefile.dist patch?
> 
> To not install the incorrect dhcpctl.3 man page.

Shouldn't it be simpler and less intrusive to %exclude them?

> >   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.

It is in dhcp-devel. I think it shouldn't be shipped, so I ask
why is it shipped?

> >   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.

This renders such changelog entries rather unuseful in fedora. Not
a big deal if things are commented in the spec.

> > * 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

I suggest getting rid of COPTS completely and putting things directly
on the command line. 

> > * 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.

Why don't you simply ship dhcp-options.5* and dhcp-eval.5* in both
packages? 

> > * 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.

Part of this explanation should be in a comment near -fvisibility=hidden
along
# -fvisibility=hidden is needed for libdhcp4client, it is the simplest
# way to avoid collisions by hiding everything and exposing only 
# the symbols needed 

> > * 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.

Mmh, I guess this dependency comes from dhcpd-conf-to-ldap, it is 
still there...

> 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.

As I expanded above there is no need to rush, but the patch layout
is not acceptable. I really can't see why the requests are unreasonable
for the Core/Extras merge review. To me it is the reverse: letting a 
package which isn't perfect (from the packaging point of view, of 
course ;-) be merged would be unreasonable.

> 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.

I see. Is there a formal collaboration between linux package distros
maintainers, and also maybe linux and BSD maintainers?

-- 
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.