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
Summary: | Review Request: TestDisk, tool to check and undelete partition | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Christophe GRENIER <grenier> |
Component: | Package Review | Assignee: | Thorsten Leemhuis <fedora> |
Status: | CLOSED NEXTRELEASE | QA Contact: | David Lawrence <dkl> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-extras-list, fedora |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
URL: | http://www.cgsecurity.org/fcextra/testdisk-6.2-2.src.rpm | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-01-24 10:07:22 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Christophe GRENIER
2005-09-30 11:43:52 UTC
Updated to version 6.2 SRPM Url: http://www.cgsecurity.org/fcextra/testdisk-6.2-1.src.rpm >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? 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. (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. (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. 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 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 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 |