[Bug 226190] Merge Review: netatalk
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: netatalk https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226190 --- Additional Comments From [EMAIL PROTECTED] 2007-06-28 08:56 EST --- Please take a look at CVS, your patch is applied. Thanks for review. -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 226190] Merge Review: netatalk
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: netatalk https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226190 [EMAIL PROTECTED] changed: What|Removed |Added Status|ASSIGNED|CLOSED Resolution||RAWHIDE Flag|fedora-review? |fedora-review+ --- Additional Comments From [EMAIL PROTECTED] 2007-06-28 20:43 EST --- 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. -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 226190] Merge Review: netatalk
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: netatalk https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226190 --- Additional Comments From [EMAIL PROTECTED] 2007-06-21 10:59 EST --- 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. -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 226190] Merge Review: netatalk
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: netatalk https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226190 [EMAIL PROTECTED] changed: What|Removed |Added Product|Fedora Extras |Fedora [EMAIL PROTECTED] changed: What|Removed |Added Status|CLOSED |ASSIGNED Keywords||Reopened Resolution|RAWHIDE | --- Additional Comments From [EMAIL PROTECTED] 2007-06-20 12:53 EST --- 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. -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 226190] Merge Review: netatalk
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: netatalk https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226190 --- Additional Comments From [EMAIL PROTECTED] 2007-06-18 08:58 EST --- 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. -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 226190] Merge Review: netatalk
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: netatalk https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226190 [EMAIL PROTECTED] changed: What|Removed |Added Status|NEEDINFO|CLOSED Resolution||RAWHIDE Flag|needinfo?([EMAIL PROTECTED]| |m) | -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 226190] Merge Review: netatalk
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: netatalk https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226190 --- Additional Comments From [EMAIL PROTECTED] 2007-06-15 10:53 EST --- 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.) -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 226190] Merge Review: netatalk
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: netatalk https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226190 [EMAIL PROTECTED] changed: What|Removed |Added Status|ASSIGNED|NEEDINFO Flag||needinfo?([EMAIL PROTECTED] ||m) -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 226190] Merge Review: netatalk
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.
Summary: Merge Review: netatalk
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226190
--- Additional Comments From [EMAIL PROTECTED] 2007-05-05 17:09 EST ---
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.
--
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the QA contact for the bug, or are watching the QA contact.
___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 226190] Merge Review: netatalk
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: netatalk https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226190 --- Additional Comments From [EMAIL PROTECTED] 2007-05-05 20:14 EST --- Created an attachment (id=154214) -- (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=154214action=view) Patch fixing remaining review issues. -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 226190] Merge Review: netatalk
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: netatalk https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226190 [EMAIL PROTECTED] changed: What|Removed |Added Severity|normal |medium Priority|normal |medium [EMAIL PROTECTED] changed: What|Removed |Added Status|NEEDINFO|ASSIGNED Flag|needinfo?([EMAIL PROTECTED]| |m) | --- Additional Comments From [EMAIL PROTECTED] 2007-04-23 16:30 EST --- Most of problems are fixed now in rawhide. Others will be resolved soon. -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 226190] Merge Review: netatalk
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: netatalk https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226190 [EMAIL PROTECTED] changed: What|Removed |Added Status|ASSIGNED|NEEDINFO Flag||needinfo?([EMAIL PROTECTED] ||m) -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 226190] Merge Review: netatalk
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.
Summary: Merge Review: netatalk
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226190
--- Additional Comments From [EMAIL PROTECTED] 2007-02-21 21:01 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
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) =
[Bug 226190] Merge Review: netatalk
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: netatalk https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226190 [EMAIL PROTECTED] changed: What|Removed |Added Status|NEW |ASSIGNED AssignedTo|[EMAIL PROTECTED]|[EMAIL PROTECTED] Flag||fedora-review? -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
