Fedora Merge Review: dhcp http://cvs.fedora.redhat.com/viewcvs/devel/dhcp/ Initial Owner: dcantrell
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.