Bug 235234 - Review Request: aoetools - ATA over Ethernet Tools
Review Request: aoetools - ATA over Ethernet Tools
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Chris Weyl
Fedora Package Reviews List
Depends On:
  Show dependency treegraph
Reported: 2007-04-04 13:08 EDT by Jima
Modified: 2016-08-14 12:24 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2007-04-09 11:00:22 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
jima: fedora_requires_release_note?
cweyl: fedora‑review+
kevin: fedora‑cvs+

Attachments (Terms of Use)

  None (edit)
Description Jima 2007-04-04 13:08:19 EDT
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
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 16:50:43 EDT
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:

make %{?_smp_mflags} CFLAGS="$RPM_OPT_FLAGS"
Comment 2 Jima 2007-04-07 17:45:23 EDT
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 17:57:43 EDT
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:
Comment 4 Chris Weyl 2007-04-08 13:33:04 EDT
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
** 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.

Comment 5 Jima 2007-04-08 16:38:24 EDT
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 16:42:03 EDT
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
Comment 7 Jima 2007-04-09 11:00:22 EDT
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 11:37:46 EDT
Oops, Firefox caching seemed to reset fedora‑cvs to ?; unsetting.
Comment 9 Karsten Wade 2007-04-22 00:08:31 EDT
Note in FC7 release notes under "PackageNotes".  Thanks for thinking of us. :)
Comment 10 Jima 2007-11-30 11:04:53 EST
Package Change Request
Package Name: aoetools
New Branches: EL-4 EL-5
Comment 11 Kevin Fenzi 2007-11-30 12:28:31 EST
cvs done.

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