Bug 225691

Initial rpmlint scan:

[root@mdehaan devel]# rpmlint *.src.rpm
W: dhcp invalid-license distributable

Given this is the nonstandard ISC license, ok.

W: dhcp unversioned-explicit-obsoletes dhcpcd

ok.

E: dhcp configure-without-libdir-spec

I'm not familiar enough with details to say whether or not this is ok.

W: dhcp macro-in-%changelog d
W: dhcp macro-in-%changelog preun
W: dhcp macro-in-%changelog postun
W: dhcp mixed-use-of-spaces-and-tabs (spaces: line 9, tab: line 253)

AFAIK, these shouldn't break anything but if they can be corrected it will make
rpmlint happier.

W: dhcp patch-not-applied Patch13: dhcp-3.0.5-xen-checksum.patch

This patch is commented out in the spec file.  I would suggest removing it from
the list of patches?

---

Spec file looks good at first glance, though I'll look over this in greater
depth next week.  Leaving as "?" to indicate review still in progress.

(In reply to comment #1)
> Initial rpmlint scan:
> 
> [root@mdehaan devel]# rpmlint *.src.rpm
> W: dhcp invalid-license distributable
> 
> Given this is the nonstandard ISC license, ok.

Would something else be better in the License: field than 'distributible'?

> E: dhcp configure-without-libdir-spec
> 
> I'm not familiar enough with details to say whether or not this is ok.

The script called 'configure' in the source tree is not a standard GNU configure
script that most people know.  It's just named configure, but it's entirely
different.  Directory locations are specified in the site.conf file, which is
populated at the beginning of the %build block.

> W: dhcp macro-in-%changelog d
> W: dhcp macro-in-%changelog preun
> W: dhcp macro-in-%changelog postun
> W: dhcp mixed-use-of-spaces-and-tabs (spaces: line 9, tab: line 253)
> 
> AFAIK, these shouldn't break anything but if they can be corrected it will make
> rpmlint happier.

I made them all %% in the changelog entries to fix the warnings.
 
> W: dhcp patch-not-applied Patch13: dhcp-3.0.5-xen-checksum.patch
> 
> This patch is commented out in the spec file.  I would suggest removing it from
> the list of patches?

The Xen patch is an ongoing work-in-progress that was only recently disabled by
me.  I'd like to keep it in the RPM for now, but I'm not currently using it.

> Spec file looks good at first glance, though I'll look over this in greater
> depth next week.  Leaving as "?" to indicate review still in progress.

Thanks.

(In reply to comment #1)

> W: dhcp unversioned-explicit-obsoletes dhcpcd
> 
> ok.

It would be better to obsolete only packages with a version <= latest
packaged version of dhcpcd.

(In reply to comment #2)

> Would something else be better in the License: field than 'distributible'?

I guess that Distributable should shut rpmlint up. But it may be better 
to have a rpmlint warning than a meaningless license. So maybe 'ISC', 
or 'ISC License' could be right if it is something known.

(In reply to comment #3)
> (In reply to comment #1)
> 
> > W: dhcp unversioned-explicit-obsoletes dhcpcd
> > 
> > ok.
> 
> It would be better to obsolete only packages with a version <= latest
> packaged version of dhcpcd.

The last version packaged was 1.3.22 in a rawhide tree many years ago.  The last
shipping version was 1.3.20 in RHEL 2.1.  I went ahead and added <= 1.3.22 to
the Obsoletes line.

(In reply to comment #4)
> (In reply to comment #2)
> 
> > Would something else be better in the License: field than 'distributible'?
> 
> I guess that Distributable should shut rpmlint up. But it may be better 
> to have a rpmlint warning than a meaningless license. So maybe 'ISC', 
> or 'ISC License' could be right if it is something known.

The common name is the ISC license, but that doesn't appear on
www.opensource.org.  The info is here:

http://www.isc.org/index.pl?/sw/dhcp/dhcp-copyright.php

I would vote for saying 'ISC' as the license.

Ok, change the license to ISC and I'll approve this.

Done.

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 ?

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

(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?

I have applied more style updates to the dhcp.spec file and I've broken out some
of the files that appeared in patches but that you thought would be better
listed as Source files.  I've added some comments in places where you suggested
they be added.

Still working on breaking out patches in to individual features.

For the ldap patch it would seem even better to use directly

PatchX: http://home.ntelos.net/~masneyb/dhcp-3.0.5-ldap-patch

instead of dhcpd-conf-to-ldap.pl dhcp-3.0.5-ldap-configuration.patch
README.ldap draft-ietf-dhc-ldap-schema-01.txt.

It would have been better to split it if there was no upstream
for the patch, but if the upstream for the patch is usable, I think
it is better to use it. Sorry for the extra work ;-(
In case the http://home.ntelos.net/~masneyb/dhcp-3.0.5-ldap-patch
patch is not usable, I would find better to have things broken out as 
you did.

That patch at least works with the latest OpenLDAP API, so I don't have a
problem with it.  The first patch I was pointed to had to be updated to work
with the new OpenLDAP API.

I'll consider this upstream for the LDAP feature unless we have to diverge
(e.g., make it work with Fedora Directory Server and not OpenLDAP).

Thanks.

Well, I take that back.  This LDAP patch is unacceptable because it doesn't work
correctly with the latest OpenLDAP on all architectures.  It also fails to
compile with -Werror.  This is the same problem I hit with the other version of
this patch I found.  I modified it to work correctly on Fedora.  Going to stick
with that.

The last big change I wanted to make to the dhcp package was to reorganize the
patches by feature/bugfix.  I've done that.  Each patch is documented in the
%prep section as to what it changes.  I plan to work on getting as many of these
upstream as possible, but releases of ISC dhcp are so infrequent, that it may
prove pointless.  The previous package maintainer had notes of sending patches
upstream that were even accepted, but haven't shown up in a release...and that's
2 years later.

Good job. It is right for the patches now.

I have a remark on the release-by-ifup patch, maybe it would
be better not to hardcode /var in /var/run/..., but allow to
override /var with something like localstatedir.

Even if ISC releases are infrequent, it is better to have those patches
submitted upstream. Also if upstream release are infrequent, you may want
to coordinate with other big distros for non-fedora specific patches.

Now more on the packaging itself:

Issues:
W: dhcp strange-permission linux.dbus-example 0775
W: dhcp strange-permission dhcpd-conf-to-ldap 0775
W: dhcp strange-permission linux 0775
This should be fixed if possible.

If it is really the case, you can fix the following
W: dhcp mixed-use-of-spaces-and-tabs (spaces: line 11, tab: line 324)

W: dhcp wrong-file-end-of-line-encoding
/usr/share/doc/dhcp-3.0.5/contrib/ms2isc/readme.txt
W: dhcp wrong-file-end-of-line-encoding
/usr/share/doc/dhcp-3.0.5/contrib/ms2isc/Registry.perlmodule
W: dhcp wrong-file-end-of-line-encoding
/usr/share/doc/dhcp-3.0.5/contrib/ms2isc/ms2isc.pl
This may be fixed by 
sed -i -e 's/\r//' ....

W: dhclient summary-ended-with-dot Provides the dhclient ISC DHCP client daemon
and dhclient-script .
Just remove the dot

W: dhcp-devel spurious-executable-perm /usr/share/man/man3/omshell.3.gz
W: dhcp-devel spurious-executable-perm /usr/share/man/man3/dhcpctl.3.gz
W: dhcp-devel spurious-executable-perm /usr/share/man/man3/omapi.3.gz
You can fix this like how you do for other man pages.

W: libdhcp4client-devel no-dependency-on libdhcp4client
I guess this should be fixed

E: dhclient obsolete-not-provided dhcpcd
Certainly the Provides could be the next version after obsoletion

Suggestions:
* use wildcards for man patches to catch no compression and other 
  compression modes, like
%attr(0644,root,root) %{_mandir}/man1/omshell.1*

* In the patch name, don't use %{version}, but instead hardcode the
  current version when the patch was added, it serves an historical
  purpose like that.

* prefix Source files with simple names with dhcp- to disambiguate
  from other files in the SOURCE dir (in my opinion this is relevant for
  README.ldap linux.dbus-example linux Makefile.dist).

* your scriptlets seem right to me but I suggest using the standard
  scriptlets, if only for consistency
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-97754e2c646616c5f6222f0cfc6923c60765133e

  Pros of the standard scriptlets may be 
  - using paths for /sbin/service and in the corresponding requires makes
    it more robust with regard with package rename/split/merge...
  - exit 0 is not very pretty
  
  Not a big deal anyway.

* I think that messing with files in %prep when it is not to fix 
  issues with upstream bad packaging (like CVS dirs, executable source 
  files) is not right, therefore I think that
# Ensure we don't pick up Perl as a dependency from the scripts and modules
# in the contrib directory (we copy this to /usr/share/doc in the final
# package).
%{__chmod} -x contrib/3.0b1-lease-convert
%{__chmod} -x contrib/dhcpd-conf-to-ldap
%{__cp} -p contrib/ms2isc/Registry.pm contrib/ms2isc/Registry.perlmodule
%{__rm} -f contrib/ms2isc/Registry.pm
  should be in %install.

  Moreover I don't like to modify the original dirs when it is only to
  cope with fedora specific stuff. You may not find it right, but I 
  would have done:

rm -rf __fedora_contrib
mkdir  __fedora_contrib
cp -a contrib __fedora_contrib
%{__chmod} -x __fedora_contrib/contrib/3.0b1-lease-convert
%{__chmod} -x __fedora_contrib/contrib/dhcpd-conf-to-ldap
%{__cp} -p __fedora_contrib/contrib/ms2isc/Registry.pm
contrib/ms2isc/Registry.perlmodule
%{__rm} -f __fedora_contrib/contrib/ms2isc/Registry.pm

(as a side note the cp followed by a rm should better be a mv in
my opinion).

And then in %doc, use
%doc __fedora_contrib/contrib

* Unless I am wrong rpm spec variables substitution happens
  before anything else so the following is simpler and works too:
%{__sed} 's/@DHCP_VERSION@/%{version}/' < %SOURCE5 > libdhcp4client.pc


I don't comment on the rpmlint warning and errors that are ignorable
in my opinion.

(In reply to comment #17)
> Good job. It is right for the patches now.
> 
> I have a remark on the release-by-ifup patch, maybe it would
> be better not to hardcode /var in /var/run/..., but allow to
> override /var with something like localstatedir.

/var/run is documented in the LFS as the location for these files.  There should
be no reason that we need rpmbuild to provide that information at build time.

Technically, I should be using /var/run/dhclient, but that's a patch for after
Fedora 7.

> Even if ISC releases are infrequent, it is better to have those patches
> submitted upstream. Also if upstream release are infrequent, you may want
> to coordinate with other big distros for non-fedora specific patches.

Agreed, but I was just wanting to point out that you will likely always see a
large patch set for dhcp, unfortunately.  They accepted things from us 2 years
ago and we have yet to see them in a new release of dhcp.  However, having it
accepted upstream is still an indication of at least some level of communication
with the upstream developers.  I am working on submitting patches we have upstream.

Collaboration with other distributors is something I'd like to get to, but it's
a bit disjoint at the moment.  Something I'd like to improve, but I need to
approach it carefully.

> Now more on the packaging itself:
> 
> Issues:
> W: dhcp strange-permission linux.dbus-example 0775
> W: dhcp strange-permission dhcpd-conf-to-ldap 0775
> W: dhcp strange-permission linux 0775
> This should be fixed if possible.

Done.
 
> If it is really the case, you can fix the following
> W: dhcp mixed-use-of-spaces-and-tabs (spaces: line 11, tab: line 324)

I'm not seeing this.
 
> W: dhcp wrong-file-end-of-line-encoding
> /usr/share/doc/dhcp-3.0.5/contrib/ms2isc/readme.txt
> W: dhcp wrong-file-end-of-line-encoding
> /usr/share/doc/dhcp-3.0.5/contrib/ms2isc/Registry.perlmodule
> W: dhcp wrong-file-end-of-line-encoding
> /usr/share/doc/dhcp-3.0.5/contrib/ms2isc/ms2isc.pl
> This may be fixed by 
> sed -i -e 's/\r//' ....

Done.
 
> W: dhclient summary-ended-with-dot Provides the dhclient ISC DHCP client daemon
> and dhclient-script .
> Just remove the dot

Done.
 
> W: dhcp-devel spurious-executable-perm /usr/share/man/man3/omshell.3.gz
> W: dhcp-devel spurious-executable-perm /usr/share/man/man3/dhcpctl.3.gz
> W: dhcp-devel spurious-executable-perm /usr/share/man/man3/omapi.3.gz
> You can fix this like how you do for other man pages.

Done.

> W: libdhcp4client-devel no-dependency-on libdhcp4client
> I guess this should be fixed

Done.
 
> E: dhclient obsolete-not-provided dhcpcd
> Certainly the Provides could be the next version after obsoletion

IMHO, all mention of dhcpcd should be removed from the spec file anyway.  That
package hasn't been around for many years.

> Suggestions:
> * use wildcards for man patches to catch no compression and other 
>   compression modes, like
> %attr(0644,root,root) %{_mandir}/man1/omshell.1*

I don't like that.  I prefer to explicitly list the files I'm including, that
way I have no surprises when it comes time to rebase the package on a new release.
 
> * In the patch name, don't use %{version}, but instead hardcode the
>   current version when the patch was added, it serves an historical
>   purpose like that.

Done.
 
> * prefix Source files with simple names with dhcp- to disambiguate
>   from other files in the SOURCE dir (in my opinion this is relevant for
>   README.ldap linux.dbus-example linux Makefile.dist).

I don't like that.  I prefer to keep the source file names exactly as they will
appear in the source tree.  No munging.
 
> * your scriptlets seem right to me but I suggest using the standard
>   scriptlets, if only for consistency
>
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-97754e2c646616c5f6222f0cfc6923c60765133e
> 
>   Pros of the standard scriptlets may be 
>   - using paths for /sbin/service and in the corresponding requires makes
>     it more robust with regard with package rename/split/merge...

Changed the scriptlets to match the packaging guidelines.

>   - exit 0 is not very pretty

Not that it matters, but why is it not pretty?  It's pointless since the shell
is already doing that, but why is it not pretty?

> * I think that messing with files in %prep when it is not to fix 
>   issues with upstream bad packaging (like CVS dirs, executable source 
>   files) is not right, therefore I think that
> # Ensure we don't pick up Perl as a dependency from the scripts and modules
> # in the contrib directory (we copy this to /usr/share/doc in the final
> # package).
> %{__chmod} -x contrib/3.0b1-lease-convert
> %{__chmod} -x contrib/dhcpd-conf-to-ldap
> %{__cp} -p contrib/ms2isc/Registry.pm contrib/ms2isc/Registry.perlmodule
> %{__rm} -f contrib/ms2isc/Registry.pm
>   should be in %install.

I'm not a big fan of this approach.  I prefer the %prep section to be all things
necessary to prepare the source tree for compilation and installation.
 
>   Moreover I don't like to modify the original dirs when it is only to
>   cope with fedora specific stuff. You may not find it right, but I 
>   would have done:
> 
> rm -rf __fedora_contrib
> mkdir  __fedora_contrib
> cp -a contrib __fedora_contrib
> %{__chmod} -x __fedora_contrib/contrib/3.0b1-lease-convert
> %{__chmod} -x __fedora_contrib/contrib/dhcpd-conf-to-ldap
> %{__cp} -p __fedora_contrib/contrib/ms2isc/Registry.pm
> contrib/ms2isc/Registry.perlmodule
> %{__rm} -f __fedora_contrib/contrib/ms2isc/Registry.pm
>
> (as a side note the cp followed by a rm should better be a mv in
> my opinion).

Changed.

> And then in %doc, use
> %doc __fedora_contrib/contrib

Done.
 
> * Unless I am wrong rpm spec variables substitution happens
>   before anything else so the following is simpler and works too:
> %{__sed} 's/@DHCP_VERSION@/%{version}/' < %SOURCE5 > libdhcp4client.pc

Done.


Now, I've committed these changes to CVS, but have not built a new set of
packages yet.  I already rebuilt dhcp today to fix a dhclient problem.  I want
that to land in rawhide before pushing a new dhcp out.  So expect a rebuild of
dhcp tomorrow to include these spec changes.  For now you can look at it in CVS.

(In reply to comment #18)

> /var/run is documented in the LFS as the location for these files.  There should
> be no reason that we need rpmbuild to provide that information at build time.
> 
> Technically, I should be using /var/run/dhclient, but that's a patch for after
> Fedora 7.

I think that even if it is specified in the FHS, it is nice to
let a user rebuilding the package be able to specify completely
its install macros. 

Moreover if it is a patch that should be submitted upstream it is 
even more important to be able to specify another location
at build time. Not a blocker for the review in any case.


> > Issues:
> > W: dhcp strange-permission linux.dbus-example 0775
> > W: dhcp strange-permission dhcpd-conf-to-ldap 0775
> > W: dhcp strange-permission linux 0775
> > This should be fixed if possible.
> 
> Done.

No, it is not fixed, because it is the perms in the src.rpm,
but I think it cannot be fixed (because it is in cvs already).

> > If it is really the case, you can fix the following
> > W: dhcp mixed-use-of-spaces-and-tabs (spaces: line 11, tab: line 324)
> 
> I'm not seeing this.

In fact it is line 654 now, in the changelog:
- fix bug 176615: fix DDNS update when Windows-NT client sends
                  host-name with trailing nul
^^^^^^^^

> IMHO, all mention of dhcpcd should be removed from the spec file anyway.  That
> package hasn't been around for many years.

I think it is nice to keep Obsoletes/Provides for more than many 
years, when they are properly versionned. Still not a blocker, though. 
 

> >   - exit 0 is not very pretty
> 
> Not that it matters, but why is it not pretty?  It's pointless since the shell
> is already doing that, but why is it not pretty?

It is not pointless, it is certainly there to for an exit code
of 0 even if the preceding command had an exit code different from
0. I say it is not pretty because in my opinion it is better to 
avoid having an exit code of 1 in the first place.
 


Let's do a reduced formal review:

* rpmlint output ignorable (or not blocking)
W: dhcp invalid-license ISC
W: dhcp strange-permission linux.dbus-example 0775
W: dhcp strange-permission dhcpd-conf-to-ldap 0775
W: dhcp strange-permission linux 0775
E: dhcp configure-without-libdir-spec
W: dhcp mixed-use-of-spaces-and-tabs (spaces: line 11, tab: line 654)
W: dhcp invalid-license ISC
E: dhcp zero-length /var/lib/dhcpd/dhcpd.leases
W: dhclient invalid-license ISC
W: dhcp-devel invalid-license ISC
W: libdhcp4client invalid-license ISC
W: libdhcp4client no-documentation
W: libdhcp4client-devel invalid-license ISC
W: libdhcp4client-devel no-documentation
W: dhcp-debuginfo invalid-license ISC
* source match upstream, but source timestamp different.
  this should be fixed next time (by using spectoo -g, wget 
  -N or the like).
-rw-rw-r-- 1 dumas dumas 876591 nov  7 16:28 dhcp-3.0.5.tar.gz
-rw-rw-r-- 1 dumas dumas 876591 nov  6 00:01 dhcp-3.0.5.tar.gz-orig

ce5d30d4645e4eab1f54561b487d1ec7  dhcp-3.0.5.tar.gz

* follow guidelines
* specfile very legible and well commented
* scriptlets right
* %files section right


Minor:
according to the newest guideline, the *.a should be in 
a distinct package, named, for example 
dhcp-static-devel or the like, with 
Requires: %{name}-devel = %{epoch}:%{version}-%{release}

APPROVED

Reassigning to me since it should be assigned to reviewer.
Michael, you can reassign to you since you did the first review.

(In reply to comment #19)
> (In reply to comment #18)
> 
> > /var/run is documented in the LFS as the location for these files.  There should
> > be no reason that we need rpmbuild to provide that information at build time.
> > 
> > Technically, I should be using /var/run/dhclient, but that's a patch for after
> > Fedora 7.
> 
> I think that even if it is specified in the FHS, it is nice to
> let a user rebuilding the package be able to specify completely
> its install macros. 
> 
> Moreover if it is a patch that should be submitted upstream it is 
> even more important to be able to specify another location
> at build time. Not a blocker for the review in any case.

I'll look in to this post-F7.

> > > Issues:
> > > W: dhcp strange-permission linux.dbus-example 0775
> > > W: dhcp strange-permission dhcpd-conf-to-ldap 0775
> > > W: dhcp strange-permission linux 0775
> > > This should be fixed if possible.
> > 
> > Done.
> 
> No, it is not fixed, because it is the perms in the src.rpm,
> but I think it cannot be fixed (because it is in cvs already).

I removed them and re-added them to CVS with 0644 permissions.

> > > If it is really the case, you can fix the following
> > > W: dhcp mixed-use-of-spaces-and-tabs (spaces: line 11, tab: line 324)
> > 
> > I'm not seeing this.
> 
> In fact it is line 654 now, in the changelog:
> - fix bug 176615: fix DDNS update when Windows-NT client sends
>                   host-name with trailing nul
> ^^^^^^^^

I fixed up line 654 and a few other lines with tabs that shouldn't have had them
(previous maintainer).

> > IMHO, all mention of dhcpcd should be removed from the spec file anyway.  That
> > package hasn't been around for many years.
> 
> I think it is nice to keep Obsoletes/Provides for more than many 
> years, when they are properly versionned. Still not a blocker, though.

An Obsoletes is fine.  But what are you suggesting for a Provides line?

> > >   - exit 0 is not very pretty
> > 
> > Not that it matters, but why is it not pretty?  It's pointless since the shell
> > is already doing that, but why is it not pretty?
> 
> It is not pointless, it is certainly there to for an exit code
> of 0 even if the preceding command had an exit code different from
> 0. I say it is not pretty because in my opinion it is better to 
> avoid having an exit code of 1 in the first place.

Right, exit 0 forces a clean exit of the scriptlet which meant a failure of the
preceeding operation doesn't matter.  Using 'exit 0' is common practice for
this, but we're really just splitting hairs here because we're both advocating
the same thing...avoid exiting with a value other than 0.  What's there now
accomplishes that as did what was there before.

> * source match upstream, but source timestamp different.
>   this should be fixed next time (by using spectoo -g, wget 
>   -N or the like).
> -rw-rw-r-- 1 dumas dumas 876591 nov  7 16:28 dhcp-3.0.5.tar.gz
> -rw-rw-r-- 1 dumas dumas 876591 nov  6 00:01 dhcp-3.0.5.tar.gz-orig
> 
> ce5d30d4645e4eab1f54561b487d1ec7  dhcp-3.0.5.tar.gz

I can't fix that.  The archive is the build system's lookaside cache and since
the checksum isn't changing, I can't override it.  If it really is a *big* deal,
I can look in to getting it removed and reupload it to the lookaside cache.

> Minor:
> according to the newest guideline, the *.a should be in 
> a distinct package, named, for example 
> dhcp-static-devel or the like, with 
> Requires: %{name}-devel = %{epoch}:%{version}-%{release}

Done.  Created dhcp-devel-static and libdhcp4client-devel-static.

> APPROVED

Hooray.

> Reassigning to me since it should be assigned to reviewer.
> Michael, you can reassign to you since you did the first review.

I think you should stay as the reviewer for this package.  You seem to have done
a bulk of the reviewing.

Also, I still have other reviews that no one has touched (225690, 225692,
225853, 226230, and 226337  :)

(In reply to comment #20)

> An Obsoletes is fine.  But what are you suggesting for a Provides line?

Maybe
Obsoletes: dhcpcd <= 1.3.22pl1-7
Provides: dhcpcd = 1.3.22pl1-8

http://fedoraproject.org/wiki/Packaging/NamingGuidelines?highlight=%28Obsolete%29#head-3cfc1ea19d28975faad9d56f70a6ae55661d3c3d


> I can't fix that.  The archive is the build system's lookaside cache and since
> the checksum isn't changing, I can't override it.  If it really is a *big* deal,
> I can look in to getting it removed and reupload it to the lookaside cache.

Not a big deal at all, I said to do that for the next time.

> Also, I still have other reviews that no one has touched (225690, 225692,
> 225853, 226230, and 226337  :)

:) I don't have time to devote to more Merge reviews currently, but
later, who knows.

OK, I've made all the necessary changes.  Is this review approved now?  If so,
can the bug be closed?

It is approved. I have set the fedora-review flag to +. I don't know
what should be done next (well I know for normal review requests
but for merge review I don't know). Maybe nothing needs to be done
since things are already in CVS.

One more comment, upon reading the guidelines, the static subpackage
name should better be dhcp-static and libdhcp4client-static, but
it is not a big deal. I attach a patch in case you want to change
it.

Created attachment 153188 [details]
use -static instead of devel-static

(In reply to comment #24)
> One more comment, upon reading the guidelines, the static subpackage
> name should better be dhcp-static and libdhcp4client-static, but
> it is not a big deal. I attach a patch in case you want to change
> it.

Fixed.

I think that you can close the bug.