Bug 226190 - Merge Review: netatalk
Merge Review: netatalk
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
: Reopened
Depends On:
  Show dependency treegraph
Reported: 2007-01-31 15:15 EST by Nobody's working on this, feel free to take it
Modified: 2011-01-19 16:56 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2007-06-28 20:43:19 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
tibbs: fedora‑cvs+

Attachments (Terms of Use)
Patch fixing remaining review issues. (3.09 KB, patch)
2007-05-05 20:14 EDT, Jason Tibbitts
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 15:15:25 EST
Fedora Merge Review: netatalk

Initial Owner: mbarabas@redhat.com
Comment 1 Jason Tibbitts 2007-02-21 21:01:10 EST
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

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

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.

* source files match upstream:
* 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:
   config(netatalk) = 4:2.0.3-9.fc7
   netatalk = 4:2.0.3-9.fc7
?  /etc/pam.d/system-auth
   config(netatalk) = 4:2.0.3-9.fc7
?  openssl
?  pam >= 0.56
   perl >= 1:5
?  tcp_wrappers

* %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.
Comment 2 Maros Barabas 2007-04-23 16:30:11 EDT
Most of problems are fixed now in rawhide. Others will be resolved soon.
Comment 3 Jason Tibbitts 2007-05-05 17:09:54 EDT
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:
  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

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.
Comment 4 Jason Tibbitts 2007-05-05 20:14:47 EDT
Created attachment 154214 [details]
Patch fixing remaining review issues.
Comment 5 Jason Tibbitts 2007-06-15 10:53:07 EDT
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.)
Comment 6 Maros Barabas 2007-06-18 08:58:50 EDT
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.
Comment 7 Jason Tibbitts 2007-06-20 12:53:53 EDT
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.
Comment 8 Maros Barabas 2007-06-21 02:57:59 EDT
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.
Comment 9 Jason Tibbitts 2007-06-21 10:59:12 EDT
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.
Comment 10 Maros Barabas 2007-06-28 08:56:15 EDT
Please take a look at CVS, your patch is applied. Thanks for review.
Comment 11 Jason Tibbitts 2007-06-28 20:43:19 EDT
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.


I'l go ahead and close this out and save you the trouble.
Comment 12 Steffan 2011-01-13 08:23:17 EST
Can this be branched to EPEL 6?
Comment 13 franklahm 2011-01-13 13:49:51 EST
+1. Netatalk is actually well maintained these days.
Comment 14 Steffan 2011-01-13 14:42:30 EST
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.
Comment 15 Jiri Skala 2011-01-19 10:47:55 EST
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.
Comment 16 Steffan 2011-01-19 11:02:12 EST
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.
Comment 17 Kevin Fenzi 2011-01-19 11:41:19 EST
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. :)
Comment 18 Steffan 2011-01-19 12:14:09 EST
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?
Comment 19 Jason Tibbitts 2011-01-19 16:56:50 EST
Git done (by process-git-requests).

Note You need to log in before you can comment on or make changes to this bug.