Red Hat Bugzilla – Bug 436637
Review Request: disktype - Detect the content format of a disk or disk image
Last modified: 2009-07-26 15:49:24 EDT
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.
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