Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 208680 - Review Request: ser2net - Proxy that allows tcp connections to serial ports
Summary: Review Request: ser2net - Proxy that allows tcp connections to serial ports
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jima
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-09-30 05:16 UTC by Tom "spot" Callaway
Modified: 2007-11-30 22:11 UTC (History)
0 users

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-10-18 21:49:18 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Tom "spot" Callaway 2006-09-30 05:16:49 UTC
Spec URL: http://www.auroralinux.org/people/spot/review/ser2net.spec
SRPM URL: http://www.auroralinux.org/people/spot/review/ser2net-2.3-1.fc6.src.rpm
Description: 
ser2net provides a way for a user to connect from a network connection to a
serial port. It provides all the serial port setup, a configuration file to
configure the ports, a control login for modifying port parameters,
monitoring ports, and controlling ports.

Comment 1 Jima 2006-10-02 15:24:08 UTC
Hmm, sounds vaguely like conserver, but I'll give this a shot. :-)

Comment 2 Jima 2006-10-02 16:15:13 UTC
Using my own review checklist:
http://beer.tclug.org/fedora-extras/review-checklist-1.1.txt

1. `rpmlint ser2net-2.3-1.fc?.*.rpm` returns:
W: ser2net service-default-enabled /etc/rc.d/init.d/ser2net

More on this below (#38).

SRPM and -debuginfo package have no rpmlint output.

2. Package appears to meet Package Naming Guidelines.
3. Spec is ser2net.spec, check.
4. Package appears to follow Packaging Guidelines.
5. Upstream site lists package as GPL.
6. Spec agrees.
7. %doc contains COPYING.
8. Spec appears to be American English.
9. Spec seems legible.
10. Tarball md5 matches upstream (5f83a3e8aec18331cb61069dccdfba47).
11. Package builds under FC5/i386, FC5/ppc, and devel/i386.
12. n/a, unless it fails under x86_64.
13. Package builds in Plague, so I imagine all necessary BRs are included.
14. Package does not appear to attempt to handle locales either properly nor
improperly.
15. n/a, no library files.
16. Package does not appear to be designed to be relocatable.
17. Package owns all directories it creates.
18. No duplicate files.
19. Permissions appear to be sane.
20. Spec contains valid %clean section.
21. Macro use appears consistent.
22. Package contains code, not content.
23. %doc is minimal.
24. %doc doesn't affect runtime.
25. n/a, no header files or static libraries.
26. n/a, no .pc files.
27. n/a, no library files.
28. n/a, no -devel subpackage.
29. n/a, no .la files.
30. n/a, not a GUI application.
31. Package doesn't appear to have file conflicts with other packages.
32. Release tag contains %{?dist}.
33. n/a, already contains (and uses) COPYING.
34. n/a, no translations.
35. Package builds in Plague for FC5/i386, FC5/ppc, & devel/i386.
36. I can't verify x86_64, but package builds everywhere else, yes.
37. Package works on FC5/i386, at least.  Neat package, too.
38. Scriptlet use appears to violate documented protocol:

http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-69c816fcf14e5130694c81f1ffa17a553ac94302

From my understanding of this text, and other package reviews, services should
not be automatically enabled, especially without checking whether the
transaction is an installation or an upgrade.

39. n/a, no subpackages.

Unless I'm mistaken (which, admittedly, is quite possible), I don't believe this
package quite passes review.  (Sorry...)

Comment 3 Tom "spot" Callaway 2006-10-04 15:52:36 UTC
Updated it to not turn the service on in the spec, but leave it enabled by
default in the init script.

New SRPM: http://www.auroralinux.org/people/spot/review/ser2net-2.3-2.fc6.src.rpm
New SPEC: http://www.auroralinux.org/people/spot/review/ser2net.spec

Comment 4 Jima 2006-10-04 18:11:42 UTC
A slight typo on line 44 of your spec introduced this warning:

W: ser2net spurious-bracket-in-%preun

Putting a space between 0 and ] fixes that, leaving only:

W: ser2net service-default-enabled /etc/rc.d/init.d/ser2net

Assuming that's not a blocker (as my sponsor, I'm going to trust you on that,
especially since I can't find anything saying otherwise), fix line 44 and I
think we can call ser2net APPROVED.

Comment 5 Ville Skyttä 2006-10-04 18:18:05 UTC
If the service is enabled by default in the init script, chkconfig --add will 
make it enabled by default.  Is there a good reason for doing so?

Comment 6 Tom "spot" Callaway 2006-10-04 19:48:13 UTC
Yes, but will chkconfig --add enable it on an upgrade if it has been disabled by
the end user?

Upstream enables this by default in their init script, and I tend to fall into
the camp of "if you didn't want it enabled, you wouldn't have installed it".

Comment 7 Tom "spot" Callaway 2006-10-18 20:56:37 UTC
New packages which fix the typo:

New SRPM: http://www.auroralinux.org/people/spot/review/ser2net-2.3-3.fc6.src.rpm
New SPEC: http://www.auroralinux.org/people/spot/review/ser2net.spec

Comment 8 Tom "spot" Callaway 2006-10-18 21:49:18 UTC
Committed, built, and done for now.

Thanks for the review.


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