[Bug 509965] Review Request: snmptt - SNMPTT (SNMP Trap Translator) is an SNMP trap handler written in Perl

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=509965





--- Comment #2 from Gary T. Giesen <giesen at snickers.org>  2009-07-07 11:47:54 EDT ---
(In reply to comment #1)
> (You don't need the NEEDSPONSOR tag since I've already agreed to sponsor you.)
> 
> - Drop "SNMPTT (SNMP Trap Translator) is" from the summary.
> 
Fixed.

> - Change the Requires: /sbin/chkconfig to Requires: chkconfig.

Fixed.

> - The character set conversion should be
>  iconv -f ISO-8859-1 -t UTF-8 ChangeLog > ChangeLog.utf8 && \
>  touch -r ChangeLog ChangeLog.utf8 && \
>  mv ChangeLog{.utf8,}
> 
I had actually used the convention here:

http://fedoraproject.org/wiki/PackageMaintainers/Packaging_Tricks#Convert_encoding_to_UTF-8

But I'll use your method in the future. I've corrected the spec

> - Drop the
>  exit 0
> from %build.

Fixed.
> 
> - %{_initdir} should be %{_initddir}. What's the idea behind the following??
>  %if 0%{?rhel} > 5 || 0%{?fedora} > 9
>  install -D -p -m 0755 snmptt-init.d %{buildroot}%{_initdir}/snmptt
>  %else
>  install -D -p -m 0755 snmptt-init.d %{buildroot}%{_initrddir}/snmptt
>  %endif
> 
> 
This was a typo here. (I guess that's what I get for writing a package at 3am)

It should have been:

%if 0%{?rhel} > 5 || 0%{?fedora} > 9
  install -D -p -m 0755 snmptt-init.d %{buildroot}%{_initddir}/snmptt
%else
  install -D -p -m 0755 snmptt-init.d %{buildroot}%{_initrddir}/snmptt
%endif

Since, according to https://fedoraproject.org/wiki/Packaging/RPMMacros
%{_initddir} is deprecated, I was trying to have use %{_initrddir} on platforms
that support it. Let me know which way you prefer I package (ie. just use
%{_initrddir} all the time or use the conditional version above)

> - You can use install to create directories as well with "install -d
> /path/to/dir"
> 
I had original done it that way, but changed it to make it easier to read. I've
changed it back and I'll use 'install -d' going forward

> - Use the standard script to create the user
> https://fedoraproject.org/wiki/Packaging/UsersAndGroups
> 
Fixed

> - Same thing for the initscript stuff
> https://fedoraproject.org/wiki/Packaging/SysVInitScript  

Fixed.

I'll post the update spec/SRPM as soon as I hear back from you on the init.d
script issue above.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.