Bug 187304 - Review Request: echoping latency meassure tool
Review Request: echoping latency meassure tool
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christoph Wickert
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT 187326
  Show dependency treegraph
 
Reported: 2006-03-29 16:04 EST by Andreas Thienemann
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-04-22 03:02:50 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
patch for echoping specfile (1.69 KB, patch)
2006-04-01 14:05 EST, Christoph Wickert
no flags Details | Diff

  None (edit)
Description Andreas Thienemann 2006-03-29 16:04:28 EST
Spec Name or Url: http://home.bawue.net/~ixs/echoping/echoping.spec
SRPM Name or Url: http://home.bawue.net/~ixs/echoping/echoping-5.2.0-0.src.rpm
Description:
"echoping" is a small program to test (approximatively) performances of a
remote host by sending it TCP "echo" (or other protocol, such as HTTP)
packets.
Comment 1 Christoph Wickert 2006-04-01 14:04:28 EST
NOTE:
I cannot approve your package atm, 
a) because of the MUST items that need to be fixed first (see below) and
b) because I'm not approved for reviewing yet, this is my first review. 
   @FE-list: Feedback is welcome, especially on the openssl license.

The license is GPL with the following addition:

> The following note is not part of the GPL:
> 
> echoping adds a small permission to the GPL: This program is released
> under the GPL with the additional exemption that compiling, linking,
> and/or using the OpenSSL library is allowed.
> 
> See <URL:http://www.openssl.org/support/faq.html#LEGAL2> and
> <URL:http://lists.debian.org/debian-legal/2002/debian-legal-200210/msg00113.html>
> for details.
> 
> 		    GNU GENERAL PUBLIC LICENSE
> 		       Version 2, June 1991
> [...]

So IMO this package should be released unter GPL since the GPL is more
restrictive than the "BSDish" license of openssl, right?

REVIEW: 

sha1sum echoping-5.2.0-0.src.rpm
aaa4f871e4950411572a873eb7ef83046ecb2f2e  echoping-5.2.0-0.src.rpm
md5sum echoping-5.2.0-0.src.rpm
caf9a5c5d12ca848ac68eaa5c941e5f4  echoping-5.2.0-0.src.rpm

Good:
- rpmlint clean on all packages
- package and spec naming OK for FE
- package meets FE guidelines
- license GPL 
- license field in specfile correct
- spec file written in English
- sources match upstream by both md5 and sha1 sums
- package builds OK on FC5 i386 and in mock for rawhide i386
- BR's OK, no duplicates
- no locales to worry about
- no shared libraries, no need for ldconfig
- relocatable
- no duplicate files
- package installs and removes fine
- %clean section present and correct
- no scriptlets needed
- code, not content
- no large docs
- docs don't affect runtime
- no static libraries or headers to worry about
- no pkgconfigs to worry about
- package doesn't own other package's directories
- no desktop file needed
- package works fine on FC5 i386

Needswork:
- MUST: increase the release to -1, -0* is not a valid release (at least for a
stable version)
"The release number is how the maintainer marks build revisions,
_starting_from_1_", see
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-5ea39bbc33cf351b41b51325ac3527eff4c58dac
- MUST: directory ownership or permissions issues as %defattr is wrong: Change
"%defattr(-, root, root)" to "%defattr(-, root, root,-)"

- SHOULD: Spec could be a little more legible. IMHO all FE specfiles should
start with a fresh fedora-newrpmspec
- SHOULD: make macro usage more consistent: Please use $RPM_BUILD_ROOT instead
of %{buildroot}
- SHOULD: please change "%{__make} %{?_smp_mflags}" to "make %{?_smp_mflags}"
(just for simplicity, there's no need to need to cover make with a macro)
- SHOULD: remove the empty NEWS from %doc.
- SHOULD: I suggest you add DETAILS to the %doc section as well.
- SHOULD: Please change "Initial RPM release" to something like "Initial Fedora
Extras release". There are already some echoping rpms in 3rd party repos.

NEEDSWORK.

I have cleaned up you specfile and fixed above issues. Attaching a patch.
Comment 2 Christoph Wickert 2006-04-01 14:05:54 EST
Created attachment 127182 [details]
patch for echoping specfile
Comment 3 Christoph Wickert 2006-04-11 18:01:11 EDT
Ok, ready to go. Please fix at least the must items. No need to apply my patch
as long as you can read your specfile. ;)
Comment 4 Andreas Thienemann 2006-04-12 23:01:13 EDT
(In reply to comment #1)

Thanks for the review.

> So IMO this package should be released unter GPL since the GPL is more
> restrictive than the "BSDish" license of openssl, right?
The License-Tag in the .spec has always been GPL. The package is released under
the GPL and not under the BSD license. The notice about the BSD license is in
there as openssl is under the BSD license and the GPL software should receive a
special permission to link against differently-free soft.
Thus it's a non-issue.
 
> Needswork:
> - MUST: increase the release to -1, -0* is not a valid release (at least for a
> stable version)
fixed.

> - MUST: directory ownership or permissions issues as %defattr is wrong: Change
> "%defattr(-, root, root)" to "%defattr(-, root, root,-)"
fixed.
 
> - SHOULD: make macro usage more consistent: Please use $RPM_BUILD_ROOT instead
> of %{buildroot}
According to the guidelines, either one is okay.

> - SHOULD: please change "%{__make} %{?_smp_mflags}" to "make %{?_smp_mflags}"
> (just for simplicity, there's no need to need to cover make with a macro)
> - SHOULD: remove the empty NEWS from %doc.
> - SHOULD: I suggest you add DETAILS to the %doc section as well.
> - SHOULD: Please change "Initial RPM release" to something like "Initial Fedora
> Extras release". There are already some echoping rpms in 3rd party repos.
All fixed.

That should be all. If you could please do a final review. thx.
Comment 6 Christoph Wickert 2006-04-13 09:15:36 EDT
(In reply to comment #4)  
> > - SHOULD: make macro usage more consistent: Please use $RPM_BUILD_ROOT instead
> > of %{buildroot}
> According to the guidelines, either one is okay.

Yes, but %{buildroot} only should be used in %install (or %clean). If for some
reason you need it in %configure you'll have to use $RPM_BUILD_ROOT there. Your
macro usage would become inconsistent then, since you are using two different
macros for the same thing. Nevertheless this isn't a blocker.

md5sum echoping-5.2.0-1.src.rpm
da157ff97fc7de0d9aabb6bd64027702  echoping-5.2.0-1.src.rpm

The new package addresses all outstanding issues:
- license ok
- license included in source and correctly installed in %doc
- license field in spec matches actual license
- package versioning fixed
- %defattr fixed
- %doc section correct, no superflurious or empty files
- changelog fixed
- all other items are ok as described in comment #1

ACCEPTED

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