This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 169624 - Review Request: TestDisk, tool to check and undelete partition
Review Request: TestDisk, tool to check and undelete partition
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Thorsten Leemhuis
David Lawrence
http://www.cgsecurity.org/fcextra/tes...
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-09-30 07:43 EDT by Christophe GRENIER
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-01-24 05:07:22 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Christophe GRENIER 2005-09-30 07:43:52 EDT
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 02:20:17 EST
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 07:36:25 EST
>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@cgsecurity.org>

--
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 10:26:54 EST
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 10:42:46 EST
(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 06:38:33 EST
(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 11:55:32 EST
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 13:29:41 EST
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 06:55:45 EST
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.