Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 235234 - Review Request: aoetools - ATA over Ethernet Tools
Summary: Review Request: aoetools - ATA over Ethernet Tools
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Chris Weyl
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-04-04 17:08 UTC by Jima
Modified: 2016-08-14 16:24 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-04-09 15:00:22 UTC
jima: fedora_requires_release_note?
cweyl: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Jima 2007-04-04 17:08:19 UTC
Spec URL: http://beer.tclug.org/fedora-extras/aoetools/aoetools.spec
SRPM URL: http://beer.tclug.org/fedora-extras/aoetools/aoetools-14-1.fc6.src.rpm
Description: 
The aoetools are programs that assist in using ATA over Ethernet on
systems with version 2.6 Linux kernels.

Comment 1 Chris Weyl 2007-04-07 20:50:43 UTC
One note before I post the actual review -- it looks like the Makefile doesn't
honor the Fedora compiler flags -- I suspect something like this would resolve that:

%build
make %{?_smp_mflags} CFLAGS="$RPM_OPT_FLAGS"


Comment 2 Jima 2007-04-07 21:45:23 UTC
Thanks for picking this up.

Just kicked off a test build with your suggestion, I'll see what the build.log
looks like.  I readily acknowledge that I'm not too good at compiler flags, so I
appreciate the pointer.

Comment 3 Jima 2007-04-07 21:57:43 UTC
Yep, looks way better.  The highlights from the build.log diff:

-+ make -j2
-cc -Wall -O -g -o aoeping.o -c aoeping.c
++ make -j2 'CFLAGS=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic'
+cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m64 -mtune=generic -o aoeping.o -c aoeping.c

-cc -Wall -O -g -o linux.o -c linux.c
-cc -Wall -O -g -o aoeping aoeping.o linux.o
+cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m64 -mtune=generic -o linux.o -c linux.c
+cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m64 -mtune=generic -o aoeping aoeping.o linux.o

You get the idea.  Fairly obvious, but here are the new spec/SRPM:
http://beer.tclug.org/fedora-extras/aoetools/aoetools.spec
http://beer.tclug.org/fedora-extras/aoetools/aoetools-14-2.fc6.src.rpm

Comment 4 Chris Weyl 2007-04-08 17:33:04 UTC
There is a devnodes.txt that's included with the distribution that you may
also want to stick in %doc

+ source files match upstream:
592f9f031796b4f0b90166a8cd5f9e30  aoetools-14.tar.gz
592f9f031796b4f0b90166a8cd5f9e30  ../aoetools-14.tar.gz
 package meets naming and versioning guidelines.
+ specfile is properly named, is cleanly written and uses macros consistently.
+ dist tag is present.
+ build root is correct.
+ license field matches the actual license.
+ license is open source-compatible. (GPL) License text included in package.
+ latest version is being packaged.
+ BuildRequires are proper.
+ compiler flags are appropriate.
+ %clean is present.
+ package installs properly
+ debuginfo package looks complete.
+ rpmlint is silent.
+ final provides and requires are sane:
** aoetools-14-1.fc6.x86_64.rpm
== rpmlint
== provides
aoetools = 14-1.fc6
== requires
/bin/bash  
/bin/sh  
libc.so.6()(64bit)  
libc.so.6(GLIBC_2.2.5)(64bit)  
libc.so.6(GLIBC_2.3)(64bit)  
libc.so.6(GLIBC_2.3.4)(64bit)  
libc.so.6(GLIBC_2.4)(64bit)  
** aoetools-debuginfo-14-1.fc6.x86_64.rpm
== rpmlint
== provides
aoetools-debuginfo = 14-1.fc6
== requires
O %check is not present -- no tests defined, however
+ no shared libraries are added to the regular linker search paths.
+ owns the directories it creates.
+ doesn't own any directories it shouldn't.
+ no duplicates in %files.
+ file permissions are appropriate.
+ no scriptlets present.
+ code, not content.
+ documentation is small, so no -docs subpackage is necessary.
+ %docs are not necessary for the proper functioning of the package.
+ no headers.
+ no pkgconfig files.
+ no libtool .la droppings.
+ not a GUI app.

APPROVED

Comment 5 Jima 2007-04-08 20:38:24 UTC
Thanks for the review!  Requesting CVS Admin attention to get the module
created.  Also, maybe we should mention the AoE userspace support in the release
notes?  No idea.  Flagging it, someone smarter than me can make that determination.

I'll include the devnotes.txt in the copy I upload.  Thanks again!

Comment 6 Jima 2007-04-08 20:42:03 UTC
Oops, I'm supposed to use this here template, huh?

New Package CVS Request
=======================
Package Name: aoetools
Short Description: ATA over Ethernet Tools
Owners: jima@beer.tclug.org
Branches: FC-5, FC-6, devel
InitialCC: 

Comment 7 Jima 2007-04-09 15:00:22 UTC
Oops.  Turns out there was a new release (15) four days after my original R&D,
but two days before I submitted this review request.  Only minor bugfixes, which
had the net result of eliminating one of my patches and reducing the other.  I'm
pushing the updated version as 15-1.

In other news, 15-1 builds successfully in the official buildsys, so I'm closing
this review out.  Thanks again, Chris!

Comment 8 Jima 2007-04-09 15:37:46 UTC
Oops, Firefox caching seemed to reset fedora‑cvs to ?; unsetting.

Comment 9 Karsten Wade 2007-04-22 04:08:31 UTC
Note in FC7 release notes under "PackageNotes".  Thanks for thinking of us. :)

Comment 10 Jima 2007-11-30 16:04:53 UTC
Package Change Request
======================
Package Name: aoetools
New Branches: EL-4 EL-5

Comment 11 Kevin Fenzi 2007-11-30 17:28:31 UTC
cvs done.


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