Bug 436637 - Review Request: disktype - Detect the content format of a disk or disk image
Review Request: disktype - Detect the content format of a disk or disk image
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-08 13:53 EST by Richard Fearn
Modified: 2009-07-26 15:49 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-05-04 16:05:26 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
bugs.michael: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Richard Fearn 2008-03-08 13:53:47 EST
Spec URL: http://richardfearn.co.uk/files/fedora/disktype-9-1/disktype.spec
SRPM URL: http://richardfearn.co.uk/files/fedora/disktype-9-1/disktype-9-1.fc8.src.rpm
Description:

I'm looking for a sponsor for my third package, disktype. This is a tool that can be used to detect the content format of a disk or disk image. It knows about common file systems, partition tables, and boot codes.

The SRPM builds using mock against Fedora 8 and develelopment. The resulting RPMs are rpmlint clean.

I've also enabled the -g flag in disktype's Makefile, so a useful -debuginfo package is produced during the build.
Comment 1 josh.kayse 2008-03-13 12:21:48 EDT
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
Comment 2 Michael Schwendt 2008-04-13 05:07:15 EDT
* 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     =
  
Comment 3 Michael Schwendt 2008-04-13 05:11:06 EDT
Plus, prefer "install -p ..." whenever installing prebuilt
files. That preserves the timestamps (and aids the package user
in seeing the age of files).
Comment 4 Nicolas Chauvet (kwizart) 2008-04-16 16:15:52 EDT
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)...
Comment 5 Nicolas Chauvet (kwizart) 2008-04-22 16:20:00 EDT
@Richard
Are you still here ?
Comment 6 Richard Fearn 2008-04-24 15:27:28 EDT
(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.
Comment 7 Richard Fearn 2008-04-25 16:55:23 EDT
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.
Comment 8 Nicolas Chauvet (kwizart) 2008-04-29 08:40:03 EDT
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).

Comment 9 Richard Fearn 2008-04-29 14:51:00 EDT
> 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.
Comment 10 Richard Fearn 2008-04-29 16:27:20 EDT
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" ?!
Comment 11 Michael Schwendt 2008-04-29 18:31:36 EDT
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.
Comment 12 Nicolas Chauvet (kwizart) 2008-04-30 04:50:57 EDT
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.

Comment 13 Nicolas Chauvet (kwizart) 2008-05-01 11:49:40 EDT
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
Comment 14 Michael Schwendt 2008-05-01 12:28:43 EDT
> 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
Comment 15 Richard Fearn 2008-05-02 17:02:24 EDT
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
Comment 16 Nicolas Chauvet (kwizart) 2008-05-04 10:35:22 EDT
@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.
Comment 17 Richard Fearn 2008-05-04 12:31:02 EDT
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
Comment 18 Richard Fearn 2008-05-04 12:33:46 EDT
Thanks Nicolas - I've already got CVS access for another two packages (ncdu and
php-pdb).
Comment 19 Kevin Fenzi 2008-05-04 13:20:52 EDT
cvs done.
Comment 20 Richard Fearn 2009-07-26 14:34:10 EDT
Package Change Request
======================
Package Name: disktype
New Branches: EL-5
Owners: richardfearn
Comment 21 Kevin Fenzi 2009-07-26 15:49:24 EDT
cvs done.

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