Bug 500922 - Review Request: zerofree - Utility to force unused ext2 inodes and blocks to zero
Summary: Review Request: zerofree - Utility to force unused ext2 inodes and blocks to ...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Susi Lehtola
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-05-14 21:20 UTC by Richard W.M. Jones
Modified: 2009-06-16 02:25 UTC (History)
4 users (show)

Fixed In Version: 1.0.1-5.fc11
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-05-18 12:03:36 UTC
Type: ---
Embargoed:
susi.lehtola: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

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.


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