[Bug 225691] Merge Review: dhcp

Wed Feb 28 11:08:55 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 dcantrell at redhat.com  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.