Bug 443675 - Review Request: sip-redirect - Tiny IPv4 and IPv6 SIP redirect server written in Perl
Review Request: sip-redirect - Tiny IPv4 and IPv6 SIP redirect server written...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-22 15:52 EDT by Robert Scheck
Modified: 2008-11-22 11:49 EST (History)
4 users (show)

See Also:
Fixed In Version: 0.1.2-2
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-11-06 15:55:55 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lemenkov: fedora‑review+
dennis: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Robert Scheck 2008-04-22 15:52:23 EDT
Spec URL: http://labs.linuxnetz.de/bugzilla/sip-redirect.spec
SRPM URL: http://labs.linuxnetz.de/bugzilla/sip-redirect-0.1.2-1.src.rpm
sip-redirect is a tiny SIP redirect server written in Perl. It is IPv4 and IPv6 
capable, but the IPv6 support is optional. The RFC 3261 was the base for this 
simple and very configurable implementation. There is neither TCP nor multicast 
support programmed in.
Comment 1 Peter Lemenkov 2008-04-25 09:41:59 EDT
Can't download sources from your main site w/o registration. It's no go for Fedora.
Comment 2 Robert Scheck 2008-04-25 09:53:59 EDT
Peter, that's a thing which can be changed. If changed, will it switch to a 
"yes go"? ;-)
Comment 3 Jason Tibbitts 2008-06-21 22:37:55 EDT
I agree that the source should if at all possible be obtainable without
registration of any kind.  I can't really fathom why you would want to restrict
downloads like that.  Get a fedorahosted or use your fedorapeople space if
you're really concerned that too many people might download your software.

The %pre scriptlet is missing the final "exit 0" line.

I don't understand why you would need to touch and chown the log directory in
%post.  Why not just put it in the %files list normally?
Comment 4 Robert Scheck 2008-06-22 12:14:40 EDT
My package does not fit 2nd must of https://fedorahosted.org/web/en/terms and 
never will do so.

It's not a log directory, it's a log file. How else to get a empty log file,
which fits "rpm -V" etc.? %ghost doesn't create files, just keeps attributes.
%config isn't IMHO an option for a log file. MySQL afaik does handing of the
log file the same way.
Comment 5 Peter Lemenkov 2008-08-23 04:10:47 EDT
I'll review it.
Comment 6 Peter Lemenkov 2008-08-23 07:31:57 EDT
Notes:

* Please, provide full path to the source tarball. E.g.
Source: http://ftp.robert-scheck.de/linux/%{name}/%{name}-%{version}.tar.gz
instead of
Source: %{name}-%{version}.tar.gz

* Please, add empty %build section to make rpmlint happy :)

* Use %{_initrddir}  instead of %{_sysconfdir}/rc.d/init.d 
BTW useful list of rpm-macros:
  https://fedoraproject.org/wiki/Packaging/RPMMacros

Other things seems sane. Consider applying changes according to these advices, and I'll made formal review.
Comment 7 Robert Scheck 2008-08-25 15:45:56 EDT
(In reply to comment #6)
> * Please, provide full path to the source tarball. E.g.
> Source: http://ftp.robert-scheck.de/linux/%{name}/%{name}-%{version}.tar.gz
> instead of
> Source: %{name}-%{version}.tar.gz

Really? See comment #1 again and AFAIK nirik complained at others about such
things.

> * Please, add empty %build section to make rpmlint happy :)

I will do so, yes.

> * Use %{_initrddir}  instead of %{_sysconfdir}/rc.d/init.d 
> BTW useful list of rpm-macros:
>   https://fedoraproject.org/wiki/Packaging/RPMMacros

AFAIK %{_initrddir} is obsolete since RPM 4.5.90 and %{_initrdir} is only available since RPM 4.5.90 (or the other way round). To avoid nasty if/else
hackings, I'm always and everywhere using the less macrofied way in that
single case (yes, I've e.g. EPEL packages).

> Other things seems sane. Consider applying changes according to these advices,
> and I'll made formal review.

Before putting a new package there, can we clarify the points above and have
a short talk regarding comment #3? Current situation is, that the sip-redirect 
needs to have a writeable log file, which I only can get with touching, as a
%config isn't the right deal and %ghost doesn't create the file. Am I missing
something or do you have a better idea?
Comment 8 Kevin Fenzi 2008-08-25 16:11:15 EDT
If your source package has a upstream full URL, you should use it so people can find your upstream version. If that URL requires some kind of registration or the like there is nothing prohibiting that in the guidelines that I know of. 

You may get an email from my source checker about it being unable to check your source, but it can be blacklisted from checking in the cases like this. 

Personally I would rather see the full URL, even if it can't be downloaded from there without some clicking.
Comment 9 Robert Scheck 2008-11-01 14:41:16 EDT
Ping? What are we currently lacking? The first two points from comment #6 are 
just a minor change, the third one I would like to avoid for the given reason.
Comment 10 Peter Lemenkov 2008-11-03 09:47:26 EST
Thanks for woke me up, Robert :)
Let's summarize things

* issue with registration may be ignored (however I still think that all sources, listed in Fedora's spec-files should be accessible w/o restrictions of any kind)

* my first two remarks are cosmetic and you promised to change your spec acordingly

* I age with you about %{_initrdir} issue - my last remark must be ignored

About touch and chmod for log files - I don't think that there are considerably more effective techniques to create zero-length log-file if it does not exists  than your approach (and MySQL maintainer's one, respectively).

OK, assuming that you fix spec-file according to all suggestions, here is my 

REVIEW:

- rpmlint is not silent:

[petro@Sulaco noarch]$ rpmlint sip-redirect-0.1.2-1.noarch.rpm 
sip-redirect.noarch: W: non-standard-uid /var/log/sip-redirect sip
sip-redirect.noarch: W: non-standard-gid /var/log/sip-redirect sip
sip-redirect.noarch: E: non-root-user-log-file /var/log/sip-redirect sip
sip-redirect.noarch: E: non-root-group-log-file /var/log/sip-redirect sip
sip-redirect.noarch: W: dangerous-command-in-%post chown
sip-redirect.noarch: E: incoherent-subsys /etc/rc.d/init.d/sip-redirect sip-redirect}
1 packages and 0 specfiles checked; 3 errors, 3 warnings.
[petro@Sulaco noarch]$ 

+ The package is named according to the Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines .
+ The License field in the package spec file matches the actual license.
+ The spec file is written in American English.
+ The spec file for the package is legible.
- The sources used to build the package must match the upstream source, as provided in the spec URL. I cannot verify it since I cannot dl files w/o registration.
+ The package successfully compiles and builds into binary rpms on at least one supported architecture (ppc).
+ No extra build dependencies
+ The package doesn't create additional directories.
+ The package does not contain any duplicate files in the %files listing.
+ Permissions on files are set properly.
+ The  package has a %clean section, which contains rm -rf %{buildroot} ( or $RPM_BUILD_ROOT ).
+ The package consistently uses macros, as described in the macros section of Packaging Guidelines .
+ The package contains code, or permissable content.
+ No large documentation files
+ Every file, the package includes as %doc, does not affect the runtime of the application. 
+ The package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot} ( or $RPM_BUILD_ROOT ). 
+ All filenames in the packages are valid UTF-8.

Warnings and errors from rpmlint may be ignored in this case, so the package is 


APPROVED
Comment 11 Robert Scheck 2008-11-03 10:01:29 EST
Peter, thanks for doing the review. I'll of course do the changes from
previous comments as I already said before importing into CVS.


New Package CVS Request
=======================
Package Name: sip-redirect
Short Description: Tiny IPv4 and IPv6 SIP redirect server written in Perl
Owners: robert
Branches: EL-4 EL-5 F-8 F-9
InitialCC: 
Cvsextras Commits: no
Comment 12 Dennis Gilmore 2008-11-03 13:49:04 EST
CVS Done
Comment 13 Robert Scheck 2008-11-06 15:55:55 EST
Package: sip-redirect-0.1.2-2.fc10 Tag: dist-f10-updates-candidate Status: complete Built by: robert
Package: sip-redirect-0.1.2-2.fc8 Tag: dist-f8-updates-candidate Status: complete Built by: robert
Package: sip-redirect-0.1.2-2.fc9 Tag: dist-f9-updates-candidate Status: complete Built by: robert

686 (sip-redirect): Build on target fedora-5-epel succeeded.
687 (sip-redirect): Build on target fedora-4-epel succeeded.
Comment 14 Fedora Update System 2008-11-06 15:56:43 EST
sip-redirect-0.1.2-2.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/sip-redirect-0.1.2-2.fc8
Comment 15 Fedora Update System 2008-11-07 21:10:37 EST
sip-redirect-0.1.2-2.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 16 Fedora Update System 2008-11-07 21:11:36 EST
sip-redirect-0.1.2-2.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 17 Fedora Update System 2008-11-10 14:43:46 EST
sip-redirect-0.1.2-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/sip-redirect-0.1.2-2.fc10
Comment 18 Fedora Update System 2008-11-22 11:49:21 EST
sip-redirect-0.1.2-2.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

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