Bug 500922

Summary: Review Request: zerofree - Utility to force unused ext2 inodes and blocks to zero
Product: [Fedora] Fedora Reporter: Richard W.M. Jones <rjones>
Component: Package ReviewAssignee: Susi Lehtola <susi.lehtola>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, herrold, notting, susi.lehtola
Target Milestone: ---Flags: susi.lehtola: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 1.0.1-5.fc11 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-05-18 12:03:36 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:

Description Richard W.M. Jones 2009-05-14 21:20:50 UTC
Spec URL: http://www.annexia.org/tmp/zerofree.spec
SRPM URL: http://www.annexia.org/tmp/zerofree-1.0.1-1.src.rpm
Description: Utility to force unused ext2 inodes and blocks to zero

Comment 1 Richard W.M. Jones 2009-05-14 21:21:28 UTC
Koji build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1355390

Comment 2 Richard W.M. Jones 2009-05-14 21:21:45 UTC
rpmlint is silent on this package.

Comment 3 Susi Lehtola 2009-05-15 08:54:03 UTC
w3m -dump http://intgat.tigress.co.uk/rmy/uml/index.html > index.html

is not OK since the build nodes don't have an internet connection.


List the file as Source1 instead.

Comment 4 Richard W.M. Jones 2009-05-15 09:43:11 UTC
Yes, very good point.  I'll post an updated package
in a moment containing that & a few other fixes ...

Comment 5 Richard W.M. Jones 2009-05-15 09:51:09 UTC
Spec URL: http://www.annexia.org/tmp/zerofree.spec
SRPM URL: http://www.annexia.org/tmp/zerofree-1.0.1-2.src.rpm
* Fri May 15 2009 Richard W.M. Jones <rjones> - 1.0.1-2
- Include the index file as a source file.
- Improve the description, remove spelling mistakes and other typos.

Comment 6 Susi Lehtola 2009-05-15 10:36:39 UTC
rpmlint output is clean.


MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. NEEDSWORK
- License in source code is GPL+ not GPLv2. License field must be GPL+.

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. ~OK
- It's also possible to use the original srpm as the source:

Source0: ftp://ftp.owlriver.com/pub/mirror/ORC/zerofree/zerofree-1.0.1-1orc.src.rpm
BuildRequires: cpio

# Create build directory without unpacking sources
%setup -T -c -n %{name}-%{version}
# Unpack original srpm
rpm2cpio %{SOURCE0} | cpio -i
# Unpack tar file
tar zxf zerofree-%{version}.tgz

Then you need to change directory to %{name}-%{version} before compiling


MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSWORK
- Use
 make CC="gcc $RPM_OPT_FLAGS" 
instead of
 make RPM_OPT_FLAGS="$RPM_OPT_FLAGS"
to honor optflags and
 cp -p %{SOURCE1} .
to preserve time stamp of the web page.

MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. N/A
MUST: Files only listed once in %files listings. N/A

MUST: Debuginfo package is complete. NEEDSWORK
- Fix the optflag problem.

MUST: Permissions on files must be set properly. NEEDSWORK
- Use
 %defattr(-,root,root,-)


MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. OK
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK

SHOULD: %{?dist} tag is used in release. NEEDSWORK
- The tag is %{?dist} not %{?_dist}

SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK

SHOULD: The package builds in mock.

Comment 7 Susi Lehtola 2009-05-15 10:37:02 UTC
Mock build is OK.

Comment 8 Richard W.M. Jones 2009-05-15 11:03:01 UTC
I think this should fix everything mentioned in the review:

Spec URL: http://www.annexia.org/tmp/zerofree.spec
SRPM URL: http://www.annexia.org/tmp/zerofree-1.0.1-3.fc11.src.rpm

* Fri May 15 2009 Richard W.M. Jones <rjones> - 1.0.1-3

- Use the upstream SRPM directly, unpacking source from it.
- License is GPLv2+.
- Fix use of dist macro.
- Pass the RPM OPTFLAGS to C compiler (should also fix debuginfo pkg).
- Use 'cp -p' to preserve timestamps when copying index.html file.
- Fix the defattr line.

Comment 9 Susi Lehtola 2009-05-15 11:27:02 UTC
(In reply to comment #8)
> * Fri May 15 2009 Richard W.M. Jones <rjones> - 1.0.1-3
> 
> - Use the upstream SRPM directly, unpacking source from it.
> - License is GPLv2+.

The license is not GPLv2+, it's GPL+ (also GPLv1 is OK). See

https://fedoraproject.org/wiki/Licensing/FAQ#How_do_I_figure_out_what_version_of_the_GPL.2FLGPL_my_package_is_under.3F

The source code doesn't mention any version, it just states that the code is under the GPL. => License field must be GPL+

Comment 10 Susi Lehtola 2009-05-15 11:29:42 UTC
If you want, you can shorten

cd %{name}-%{version}
mkdir -p $RPM_BUILD_ROOT%{_sbindir}
cp zerofree $RPM_BUILD_ROOT%{_sbindir}

to a one-liner:

install -D -p -m 755 %{name}-%{version}/zerofree $RPM_BUILD_ROOT%{_sbindir}/zerofree

(This also preserves the time stamp, even though it doesn't matter in case of a compiled binary.)

Comment 11 Richard W.M. Jones 2009-05-15 11:46:56 UTC
Spec URL: http://www.annexia.org/tmp/zerofree.spec
SRPM URL: http://www.annexia.org/tmp/zerofree-1.0.1-4.fc11.src.rpm

I have made the two changes you suggested.

Comment 12 Susi Lehtola 2009-05-15 12:05:37 UTC
Everything has been fixed, the package has been

APPROVED

Comment 13 Richard W.M. Jones 2009-05-15 12:10:15 UTC
Thanks Jussi!

New Package CVS Request
=======================
Package Name: zerofree
Short Description: Utility to force unused ext2 inodes and blocks to zero
Owners: rjones
Branches: F-10 F-11 EL-5

Comment 14 R P Herrold 2009-05-15 12:49:03 UTC
I am technically not the upstream -- just the birddog and early packager, at owlriver ... please point to the URL upstream's DL link for Source0

http://intgat.tigress.co.uk/rmy/uml/zerofree-1.0.1.tgz

-- Russ herrold

Comment 15 R P Herrold 2009-05-15 12:51:06 UTC
Thinking about it. you may also want to add the companion:

http://intgat.tigress.co.uk/rmy/uml/sparsify.c

to the package as well which adds some additional function on making a more slender FS

-- Russ herrold

Comment 16 Richard W.M. Jones 2009-05-15 14:38:14 UTC
Given Russ herrold's comments above, I have prepared a new
package.  This package changes the upstream URL (and removes
the whole unpacking-the-SRPM thing).  More importantly it
also includes the sparsify program.

I'll leave it up to you (Jussi) whether you want to reopen
the review of this package.

Here it is:

Spec URL: http://www.annexia.org/tmp/zerofree.spec
SRPM URL: http://www.annexia.org/tmp/zerofree-1.0.1-5.fc11.src.rpm

rpmlint is still clean.

Comment 17 Susi Lehtola 2009-05-15 14:48:39 UTC
No, the package is fine as it is; sparsify is also under GPL+ and doesn't raise any other issues.

What's up with the changelog, though? You've merged all of the changes of the interstitial specs together? Please, keep a full changelog.

Comment 18 Richard W.M. Jones 2009-05-15 14:52:06 UTC
Ah I was just keeping them all under the same date heading,
but I can split them out if you like / in future.

Comment 19 Richard W.M. Jones 2009-05-15 15:16:13 UTC
Copied the CVS request down here to make it easier to find:

New Package CVS Request
=======================
Package Name: zerofree
Short Description: Utility to force unused ext2 inodes and blocks to zero
Owners: rjones
Branches: F-10 F-11 EL-5

Comment 20 Kevin Fenzi 2009-05-16 00:02:50 UTC
cvs done.

Comment 21 Fedora Update System 2009-05-18 08:20:34 UTC
zerofree-1.0.1-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/zerofree-1.0.1-5.fc10

Comment 22 Fedora Update System 2009-05-18 08:21:59 UTC
zerofree-1.0.1-5.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/zerofree-1.0.1-5.fc11

Comment 23 Richard W.M. Jones 2009-05-18 08:42:21 UTC
Bit of a build problem in EL-5 on ppc at the moment.
I need to ask someone in rel-eng to take a look.

http://buildsys.fedoraproject.org/logs/fedora-5-epel/2328-zerofree-1.0.1-5.el5/ppc/root.log

Comment 24 Richard W.M. Jones 2009-05-18 12:03:36 UTC
Seemed to be a temporary blib, and zerofree is now built in EL-5
too.

http://buildsys.fedoraproject.org/logs/fedora-5-epel/2330-zerofree-1.0.1-5.el5/

Thanks everyone for helping, particularly Jussi for the detailed and
useful review.

Comment 25 Fedora Update System 2009-06-16 02:07:11 UTC
zerofree-1.0.1-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 26 Fedora Update System 2009-06-16 02:25:38 UTC
zerofree-1.0.1-5.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.