Hide Forgot
Fedora Merge Review: netatalk http://cvs.fedora.redhat.com/viewcvs/devel/netatalk/ Initial Owner: mbarabas
I have to admit to being a bit apprehensive about reviewing this package because I know how ancient it is, but it's really not too bad. Let's see what rpmlint has to say: W: netatalk prereq-use /sbin/chkconfig, /sbin/service PreReq doesn't work as intended in RPM; use Requires(post): /sbin/chkconfig Requires(preun): /sbin/chkconfig Requires(preun): /sbin/service Requires(postun): /sbin/service instead. W: netatalk macro-in-%changelog config W: netatalk macro-in-%changelog defattr W: netatalk macro-in-%changelog exclusiveos '%' symbols in %changelog need to be escaped by doubling them. Otherwise they can actually be expanded under some circumstances. W: netatalk mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 9) rpmlint is needlessly picky, although line 9 is a bit odd, containing "space tab space". W: netatalk conffile-without-noreplace-flag /etc/rc.d/init.d/atalk This is OK; there's ongoing discussion about the proper thing to do here but no concensus. W: netatalk devel-file-in-non-devel-package /usr/bin/netatalk-config Any reason why this isn't in the -devel package? E: netatalk wrong-script-interpreter /usr/share/doc/netatalk-2.0.3/ICDumpSuffixMap "perl" This file has nonstandard line endings. It should be either converted or just deleted. If you choose to keep it, you might as well patch the #! line to include a path. E: netatalk executable-marked-as-config-file /etc/rc.d/init.d/atalk Generally init.d files are executable, so this is OK E: netatalk setuid-binary /usr/bin/afppasswd root 04755 E: netatalk non-standard-executable-perm /usr/bin/afppasswd 04755 This is just the way it has to be, as I understand things. W: netatalk incoherent-init-script-name atalk rpmlint wants to see the init script named after the package, but after all this time I can't imagine changing this. W: netatalk-devel no-documentation Not a problem. Other issues found during review: You can use %{_initrddir} instead of defining "initdir" if you like. BuildRoot should be: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) Any reason for not allowing parallel make? It seems to work fine on my 8-way machine. There are a couple of odd dependencies. Given that even the very old RH9 shipped with pam 0.75, it's certainly safe to drop the versioned requirement on pam, although it's not really problematic to keep it. You don't need the explicit openssl or cracklib dependencies as RPM picks up the libcrypto and libcrack dependencies by itself. Also, you have a runtime dependency on tcp_wrappers, but no build-time dependency on it, so the resulting package is built without tcp_wrappers support. I think you should add a BR: on tcp_wrappers-devel and remove the manual runtime tcp_wrappers dependency; RPM will pick up the libwrap dependency which will pull in tcp_wrappers-libs automatically. Also, I'm not sure why there's an explicit requirement for /etc/pam.d/system-auth. This file has been part of pam for as long as I can recall, and having an explicit file dependency causes extra pain for end users in dependency resolution because yum has to pull in filelists.xml. (See http://fedoraproject.org/wiki/PackagingDrafts/FileDeps for more info.) Several libraries are installed into %{_libdir}, but there are no scriptlets which call ldconfig. You should add /sbin/ldconfig calls to %post and %postun and the necessary dependencies. There are several static libraries and .la files in the -devel package. Generally static libraries aren't shipped in Fedora without a really good reason, and if they are shipped, they need to be in a -static subpackage. Libtool .la files shouldn't be shipped at all unless the package breaks without them. Review: * source files match upstream: 25e004732f471de0dd9a21ab129ee799da018fce3b313d4ab5e6f52e6e9e3998 netatalk-2.0.3.tar.bz2 * package meets naming and versioning guidelines. * specfile is properly named, is cleanly written and uses macros consistently. * dist tag is present. X build root is not correct. * license field matches the actual license. * license is open source-compatible. * license text included in package. * latest version is being packaged. * BuildRequires are proper (you shouldn't need to specify pam, as pam-devel should pull it in, and you don't need to specify libtool explicitly.) * compiler flags are appropriate. * %clean is present. * package builds in mock (development, x86_64). * package installs properly * debuginfo package looks complete. X rpmlint has valid complaints. ? final provides and requires are sane: netatalk-2.0.3-9.fc7.x86_64.rpm config(netatalk) = 4:2.0.3-9.fc7 uams_dhx_pam.so()(64bit) uams_dhx_passwd.so()(64bit) uams_gss.so()(64bit) uams_guest.so()(64bit) uams_pam.so()(64bit) uams_passwd.so()(64bit) uams_randnum.so()(64bit) netatalk = 4:2.0.3-9.fc7 = /bin/sh ? /etc/pam.d/system-auth /sbin/chkconfig /sbin/service /usr/bin/perl config(netatalk) = 4:2.0.3-9.fc7 cracklib libcom_err.so.2()(64bit) libcrack.so.2()(64bit) libcrypto.so.6()(64bit) libdb-4.5.so()(64bit) libgssapi_krb5.so.2()(64bit) libgssapi_krb5.so.2(gssapi_krb5_2_MIT)(64bit) libk5crypto.so.3()(64bit) libkrb5.so.3()(64bit) libpam.so.0()(64bit) libpam.so.0(LIBPAM_1.0)(64bit) ? openssl ? pam >= 0.56 perl >= 1:5 perl(Getopt::Std) perl(IO::Socket) perl(Socket) perl(strict) perl(vars) ? tcp_wrappers uams_dhx_pam.so()(64bit) uams_pam.so()(64bit) * %check is not present; no test suite upstream. X shared libraries are added to the regular linker search paths, but ldconfig is not called. * owns the directories it creates. * doesn't own any directories it shouldn't. * no duplicates in %files. * file permissions are appropriate. * no scriptlets present (but there should be) * code, not content. * documentation is small, so no -docs subpackage is necessary. Actually the package is about half documentation, but the whole thing isn't very large. * %docs are not necessary for the proper functioning of the package. * headers are in the -devel package. * no pkgconfig files. X static libraries. X libtool .la droppings.
Most of problems are fixed now in rawhide. Others will be resolved soon.
I checked what's in CVS currently; I'll check over the issues that popped up in the previous review. First, a few remaining rpmlint warnings: W: netatalk devel-file-in-non-devel-package /usr/bin/netatalk-config Actually it's now in both packages, because this: %{_bindir}/* in the regular package pulls it in. An %exclude fixes it up; I moved the manpage over to -devel as well. E: netatalk wrong-script-interpreter /usr/share/doc/netatalk-2.0.3/ICDumpSuffixMap "perl" Still around. Not sure what's up here. I note this file is included as Source4, but it's also in contrib. My suggestion would be to drop Source4 and use the script from contrib, but fix up the line endings and #!perl call. E: netatalk wrong-script-interpreter /usr/share/doc/netatalk-2.0.3/ICDumpSuffixMap "perl" E: netatalk executable-marked-as-config-file /etc/rc.d/init.d/atalk E: netatalk setuid-binary /usr/bin/afppasswd root 04755 E: netatalk non-standard-executable-perm /usr/bin/afppasswd 04755 W: netatalk conffile-without-noreplace-flag /etc/rc.d/init.d/atalk W: netatalk incoherent-init-script-name atalk W: netatalk-devel no-documentation E: netatalk executable-marked-as-config-file /etc/rc.d/init.d/atalk W: netatalk conffile-without-noreplace-flag /etc/rc.d/init.d/atalk The guidelines have actually changed now; init scripts must not be marked as %config. E: netatalk setuid-binary /usr/bin/afppasswd root 04755 E: netatalk non-standard-executable-perm /usr/bin/afppasswd 04755 W: netatalk incoherent-init-script-name atalk W: netatalk mixed-use-of-spaces-and-tabs (spaces: line 2, tab: line 19) These are all OK. * Buildroot is fine now. (In any case, he buildroot guidelines ended up being considerably loosened since the the initial review was done.) * Dependencies look good. * ldconfig is now called as necessary. * We now have guidelines for static libraries; they should not be included at but if there is a justifiable reason for why they absolutely must be included, they must be in a -static subpackage and only then after approval of FESCO. I'll attach a patch which fixes these issues, but it also removes all of the static libaries and .la files. If you're convinced that they're necessary, you'll need to ask FESCO for an exception. I need to run now and it may be a couple of hours before I can get that patch uploaded.
Created attachment 154214 [details] Patch fixing remaining review issues.
Perhaps we could finish the review before closing this ticket? What did you think of the patch I attached in comment #4? What about the static libraries? (BTW, the guidelines relating to static libraries have changed; you can include them as long as you can explain why they're necessary.)
I agree with your patch. Maybe *.a libraries should be in devel package but as you say I can't explain why they are necessary so I think they are not. In my opinion, this patch is good at all.
I'll reopen this ticket. Can you let me know if you've applied that patch or made other changes to the package so that I can take a look? I can grab it out of CVS, but the last thing I see in the changelog is from April 17.
Hi, thanks for notice. Yes, I commit changes to rawhide, but there was a problem with building package. I hope the problem will disapeare in a couple of days. After that I close the bug again.
Actually, the way it works is that this ticket isn't closed until the reviewer approves the package. Usually the package maintainer communicates with the reviewer and says things like "I applied your patch, thanks" or "I've fixed the following issues that you found" and then the reviewer can take a look at what's in CVS and if things are good the fedora-review flag is set to '+' and then the maintainer closes the bug once the package with all of the changes has been built successfully. Under no circumstances that I can think of is the ticket closed without the reviewer signing off on the package.
Please take a look at CVS, your patch is applied. Thanks for review.
OK, the only remaining thing I could possibly complain about is a typo in the last changelog entry, causing this: W: netatalk incoherent-version-in-changelog 4:2.0.4-11 4:2.0.3-11.fc8 (2.0.4 should be 2.0.3). Anyway, that's minor. APPROVED I'l go ahead and close this out and save you the trouble.
Can this be branched to EPEL 6?
+1. Netatalk is actually well maintained these days.
I emailed the author/maintainer on this. Hopefully someone will respond to the request with something. Even a "hell no" would direct me to another resource.
Package Change Request ====================== Package Name: netatalk New Branches: el5 el6 Owners: jskala Additional info: - Netatalk was released last time in RHEL-5.2 update. This package is no more updated in RHEL-5 therefore I'm requesting epel 5 and 6. - The highest suitable version for EPEL 5 is netatalk-2.0.5. This is current release in F13. Versions 2.1.X can't be used due to requirement for DB4 >= 4.6.
What would it take to get the DB4 >= 4.6 on EPEL as well so we can use the version 2.1? The newer version has a bunch of fixes and speed improvements.
netatalk is no longer shipped in RHEL5 that I can see, and never was in RHEL6, so I would think it would be fine to process the request from comment 15. As to db4, that would be a great deal more complex. You would need to submit for review a db47 compat package that was completely parallel installable and didn't bother anything using the default db4 package. This is unlikely to be easy. :)
Forgive my ignorance of process, but, where does it go from here? Is it a complete build and package or is it some kind of simple addition to another repo?
Git done (by process-git-requests).