I'll take a shot at pre-review:

MUST:
+ rpmlint output
  rpmlint is clean
+ package name satisfies the packaging naming guidelines
+ specfile name matches the package base name
+ package should satisfy packaging guidelines
+ license meets guidelines and is acceptable to Fedora
+ license matches actual package license
+ includes LICENSE file in %doc
+ spec file written in American English
+ spec file is legible
+ upstream sources match sources in the srpm
+ package successfully builds on at least one architecture
  i386, x86_64
+ ExcludeArch bugs filed
+ BuildRequires list all build dependencies
+ %find_lang instead of %{_datadir}/locale/*
+ binary RPM with shared library files must call ldconfig in %post and %postun
+ does not use Prefix: /usr
+ package owns all directories it creates
+ no duplicate files in %files
+ %defattr line
+ %clean contains rm -rf $RPM_BUILD_ROOT
+ consistent use of macros
+ package must contain code or permissible content
+ large documentation files should go in -doc subpackage
+ files marked %doc should not affect package
+ header files should be in -devel
+ static libraries should be in -static
+ packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
+ libfoo.so must go in -devel
+ -devel must require the fully versioned base
+ packages should not contain libtool .la files
+ packages containing GUI apps must include %{name}.desktop file
+ packages must not own files or directories owned by other packages

Optional:

+ if there is no license file, packager should query upstream
+ translations of description and summary for non-English languages, if available
+ builds in mock
+ builds on i386 and x86_64
? review should test the package functions as described
+ scriptlets should be sane
+ pkgconfig files should go in -devel
+ shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or /usr/sbin

I would approve

* Doesn't build with our global $RPM_OPT_FLAGS (or %optflags).
  Make sure you set CFLAGS in the Makefile, e.g. with a
  modification of the existing patch:

--- makefile.patch~     2008-03-08 19:41:21.000000000 +0100
+++ makefile.patch      2008-04-13 11:04:53.000000000 +0200
@@ -6,7 +6,7 @@
  
  CPPFLAGS = -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64
 -CFLAGS   = -Wall
-+CFLAGS   = -Wall -g
++CFLAGS   = -Wall $(RPM_OPT_FLAGS)
  LDFLAGS  =
  LIBS     =
  

Plus, prefer "install -p ..." whenever installing prebuilt
files. That preserves the timestamps (and aids the package user
in seeing the age of files).

See https://www.uitwisselplatform.nl/frs/?group_id=53
for disktype to support libewf. The libewf license in F-9 (BSD) allow disktype
to use libewf whereas BSD with advertising (F-8 version) doesn't allow this.
I don't know if the upstream disktype maintainer could add this patch thought.
(there isn't much movement in the SF cvs)...

@Richard
Are you still here ?

(In reply to comment #5)
> @Richard
> Are you still here ?

Yes, I am! I was on holiday last weekend so since getting feedback (thanks to
Josh, Michael and yourself) I haven't had a chance to work on the package. I'm
looking at the EWF support now.

Thanks again to Michael and Nicolas for your comments. I've addressed the issues
raised by Michael and included EWF support. The new spec file and SRPM can be
found here:

Spec URL: http://richardfearn.co.uk/files/fedora/disktype-9-2/disktype.spec
SRPM URL:
http://richardfearn.co.uk/files/fedora/disktype-9-2/disktype-9-2.fc8.src.rpm

The SRPM builds using mock against Fedora 9. The resulting RPMs are rpmlint clean.

Seems good to me.
However I wonder if we can move disktype to sbindir instead of bindir:
from the disttype man:
". Note that running disktype on device files like your hard disk will
     likely require root rights."
We usually move this kind of app to sbindir

Note that the libewf patch expect libewf header to be in /usr/local/include/libewf.h

It is currenly available in /usr/include which is the default path, so it won't
hurt. But the nicest tweak would be to use pkg-config libewf cflags and
pkg-config libewf --libs (for the libs side).
This would lead to have : in the %prep section
--------------------
sed -i -e 's|-I/usr/local/include|%(pkg-config libewf --cflags)|' Makefile
sed -i -e 's|-L/usr/local/lib||' Makefile
sed -i -e 's|-lewf|%(pkg-config libewf --libs)|' Makefile
--------------------
Note that the current version in F-9 will not handle this nicely as
openssl-devel is missing from libewf-devel (you may need to add it from
disktype, but i think it won't be required - I've asked libewf upstream to sort
this and i guess it will be removed on the next libewf bugfix update planned for
the end of the weak).
(This problem was my bad - sorry - for now it is safe to handle the way you do
until libewf is fixed - it won't be possible to link to the F-8 version anyway
as BSD advertissing close remains present).

> However I wonder if we can move disktype to sbindir instead of bindir:
> from the disttype man:
> ". Note that running disktype on device files like your hard disk will
>      likely require root rights."
> We usually move this kind of app to sbindir

I don't think it should be in sbindir. The FHS says that /usr/sbin is for
"non-essential binaries used exclusively by the system administrator". disktype
doesn't fall into that category. The main reason I use disktype is for examining
ISO images, which I don't think is an administrator-only task. Compare with
commands like isosize or volname; they go into /usr/bin.

Nicolas,

I can see that I'd need to have a BuildRequires for openssl-devel (for now at
least) to have openssl.pc available. However zlib-devel doesn't provide a
zlib.pc - so pkg-config gives me this:

[rich@laptop ~]$ pkg-config libewf --cflags
Package zlib was not found in the pkg-config search path.
Perhaps you should add the directory containing `zlib.pc'
to the PKG_CONFIG_PATH environment variable
Package 'zlib', required by 'libewf', not found

So it seems to me that even if libewf-devel is updated to depend on
openssl-devel, pkg-config still won't work for libewf because of zlib.

> i guess it will be removed on the next libewf bugfix update

Did you mean "added" instead of "removed" ?!

Why does libewf.pc "Requires: zlib openssl"?

$ ldd libewf.so.1.0.2 
        linux-gate.so.1 =>  (0x00110000)
        libz.so.1 => /lib/libz.so.1 (0x0014a000)
        libc.so.6 => /lib/libc.so.6 (0x0015d000)
        /lib/ld-linux.so.2 (0x0043a000)

The libz dependency could be made "Libs.private" in the pkg-config
file as libewf is linked against it, not the app linking against
libewf.

About the missing zlib.pc, as a work-around you could create
your own, store it somewhere in $(pwd)/foo and point
PKG_CONFIG_PATH to it.

yep, that's exactly the patch I've send upstream about libewf.pc
It missed my attention on previous update.
So I have removed Requires: zlib openssl and move -lz to Libs.private: -lz

I will see if I wait for the new (bugfix) libewf or not, but we need a fixed
version for f9-final anyway.
For now it is safe not to use pkg-config.

Ok about bindir versus sbindir. Actually i guess libewf won't require root
rights anyway.

Ok a fixed libewf has been commited via cvs.
I cannot built it until disktype is imported (as ewftools will requires disktype)
you can build testdisk with the previous libewf release (that is api and abi
compatible with the newer).
Until the newer version is available in the buildroot you won't be able to use
the pkg-config improvement, so the default one will be fine.

So I'm fine with the release in #c7

@Michael
as soon as Richard is sponsored and cvs for disktype is created - I might build
libewf and ask rel-eng to tag them as f9-final. If we run low in time, then i
guess it will be f9-updates

> ifneq ($(LIBEWF),)
>   CPPFLAGS += -DUSE_LIBEWF
>   CFLAGS   += -I/usr/local/include
>   LDFLAGS  += -L/usr/local/lib
>   LIBS     += -lewf
>endif

Why /usr/local? This alters the headers/libraries search path
and can cause non-mock builds to be less reproducible than with
standard search paths. It would be cleaner to not modify the
flags like that.

Else disktype-9-2.fc8.src.rpm from comment 7 is: APPROVED

I've taken the CFLAGS/LDFLAGS changes out of the patch and also added EWF to the
man page (in the list of recognised formats). The new spec/SRPM are here:

Spec URL: http://richardfearn.co.uk/files/fedora/disktype-9-3/disktype.spec
SRPM URL:
http://richardfearn.co.uk/files/fedora/disktype-9-3/disktype-9-3.fc8.src.rpm

@Richard
In my view this release 3 is fine also, but next step from your side is to request
cvs creation.
 (see http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure)
But you will need to have your sponsorship approved first.

New Package CVS Request
=======================
Package Name: disktype
Short Description: Detect the content format of a disk or disk image
Owners: richardfearn
Branches: F-9
InitialCC: 
Cvsextras Commits: yes

Thanks Nicolas - I've already got CVS access for another two packages (ncdu and
php-pdb).

cvs done.

Package Change Request
======================
Package Name: disktype
New Branches: EL-5
Owners: richardfearn

cvs done.