Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 169624 - Review Request: TestDisk, tool to check and undelete partition
Summary: Review Request: TestDisk, tool to check and undelete partition
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Thorsten Leemhuis
QA Contact: David Lawrence
URL: http://www.cgsecurity.org/fcextra/tes...
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2005-09-30 11:43 UTC by Christophe GRENIER
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-01-24 10:07:22 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Christophe GRENIER 2005-09-30 11:43:52 UTC
Spec Url: http://www.cgsecurity.org/fcextra/testdisk.spec
SRPM Url: http://www.cgsecurity.org/fcextra/testdisk-5.9-1.src.rpm
It's my first FC Extra package and I am seeking a sponsor.

Description:
Tool to check and undelete partition. Works with the following
partitions:
- DOS/Windows FAT12, FAT16 and FAT32
- NTFS ( Windows NT/2K/XP )
- Linux Ext2 and Ext3
- BeFS ( BeOS )
- BSD disklabel ( FreeBSD/OpenBSD/NetBSD )
- CramFS (Compressed File System)
- HFS, Hierarchical File System
- JFS, IBM's Journaled File System
- Linux Raid
- Linux Swap (versions 1 and 2)
- LVM and LVM2, Linux Logical Volume Manager
- Netware NSS
- ReiserFS 3.5 and 3.6
- UFS (Sun/BSD/...)
- XFS, SGI's Journaled File System

Comment 1 Christophe GRENIER 2005-12-21 07:20:17 UTC
Updated to version 6.2
SRPM Url: http://www.cgsecurity.org/fcextra/testdisk-6.2-1.src.rpm

Comment 2 Thorsten Leemhuis 2005-12-24 12:36:25 UTC
>It's my first FC Extra package and I am seeking a sponsor.
I can do that. 

No full review yet, just the things I saw on a first sight:

--
Using these macros seems unnescaccary and confusing to me:

%define name    testdisk
%define ver     6.2
%define rel     1

You IMHO should remove them

--
Could you remove this:
#Packager:	Christophe GRENIER <grenier>

--
Please change
BuildRoot:	%{_tmppath}/%{name}-%{version}-buildroot
to
BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

--
The description is IMHO way to long. Why not something like:
Tool to check and undelete partition. Works with FAT12, FAT16, FAT32, NTFS, 
EXT2, EXT3, BeFS ....

And the question is wether ntfs is allowed to be supported if this porgramm hits
fedora extras. What exactly does it to support NTFS? Could is be in conflict
with patents around NTFS?
  
--
during configure I noticed:
>checking for ntfs_device_mount in -lntfs... no
>configure: WARNING: No ntfs library detected
>checking for libreiserfs_get_version in -lreiserfs... no
>configure: WARNING: No reiserfs library detected

Is that correct? There are several other checks that result in "no" -- Could you
check if they can be ignored safely?

Comment 3 Christophe GRENIER 2005-12-24 15:26:54 UTC
I have updated the spec file according to your remarks.

Be supporting NTFS, it only means TestDisk can decode the NTFS boot sector,
it doesn't mean it can read/write NTFS partition.

ntfs and reiserfs library aren't shipped in fedora. TestDisk can work without
them, they are only necessary if you want to be able to list the files from
ReiserFS 3.5/2.6 and NTFS partition.

So there is no problem between TestDisk and potential patents around NTFS.

It's not a problem if there are a lot of "no" during configure.
TestDisk can be compiled on a lot of architecture and plateforme, so it has to
check a lot of thinks.


Comment 4 Thorsten Leemhuis 2005-12-24 15:42:46 UTC
(In reply to comment #3)
> I have updated the spec file according to your remarks.

And you did not increase the Release -- please do this the next time. That's
easier and avoids confusion, even during review.

> Be supporting NTFS, it only means TestDisk can decode the NTFS boot sector,
> it doesn't mean it can read/write NTFS partition.

That might be okay. But I'll recheck with Fesco/Legal.

> ntfs and reiserfs library aren't shipped in fedora. TestDisk can work without
> them, they are only necessary if you want to be able to list the files from
> ReiserFS 3.5/2.6 and NTFS partition.
Okay.

> It's not a problem if there are a lot of "no" during configure.

Sure, I just wanted to be sure that you checked them. ;-)

One other thing I just noticed that I'm not sure about myself. The spec file has:
%doc %{_mandir}/man1/testdisk.1*
%doc %{_mandir}/man1/photorec.1*

Uhh, do we ship man pages as doc? To answer my own question: Seems we sometimes
do according to a grep through a extras cvs checkout
[rpmbuild@truhe devel]$ grep /man./ */*.spec  | grep \%doc | wc -l
35
[rpmbuild@truhe devel]$ grep /man./ */*.spec   | wc -l
1071

@fedora-extras-list: Any option on this? IMHO man pages should not be marked as
%doc. 

Comment 5 Paul Howarth 2005-12-27 11:38:33 UTC
(In reply to comment #4)
> One other thing I just noticed that I'm not sure about myself. The spec file has:
> %doc %{_mandir}/man1/testdisk.1*
> %doc %{_mandir}/man1/photorec.1*
> 
> Uhh, do we ship man pages as doc? To answer my own question: Seems we sometimes
> do according to a grep through a extras cvs checkout
> [rpmbuild@truhe devel]$ grep /man./ */*.spec  | grep \%doc | wc -l
> 35
> [rpmbuild@truhe devel]$ grep /man./ */*.spec   | wc -l
> 1071
> 
> @fedora-extras-list: Any option on this? IMHO man pages should not be marked
as %doc.

rpmbuild in Fedora automatically marks manpages as %doc so there is no need to
do it explicitly like this. I think the same goes for info files.




Comment 6 Thorsten Leemhuis 2006-01-21 16:55:32 UTC
Where is the lastet SRPM?
http://www.cgsecurity.org/fcextra/testdisk-6.2-1.src.rpm
seems to be the old one. And if you need to re-upload/create it: could you
remove the %doc as per comment #5 ? tia

Comment 7 Christophe GRENIER 2006-01-21 18:29:41 UTC
Done, %doc as been removed as per comment #5
Spec Url: http://www.cgsecurity.org/fcextra/testdisk.spec
SRPM Url: http://www.cgsecurity.org/fcextra/testdisk-6.2-2.src.rpm

Comment 8 Thorsten Leemhuis 2006-01-22 11:55:45 UTC
Review of
d92f0ba26cf4e1c8bb276a38b9dbe376d6599004  testdisk-6.2-2.src.rpm

$ rpmlint rpmbuild/*PMS/{,*/}testdisk*6.2-2*
W: testdisk incoherent-version-in-changelog 5.0 6.2-2

This is harmless, but you should do it correctly in the future ;-)

Review:
- Souce testdisk-6.2.tar.bz2 is the same as upstream
- Builds fine in mock
- License is allowed in FE, correct in the spec file and shipped as %doc
- name follows PackageNamingGuidelines
- Spec looks good
- File list looks good
- Works for me
- dir ownership OK
- permissions OK
- clean OK

APPROVED; Create an account in the accounts system -- I'll sponsor you


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