Bug 520832

This seems to be your first package submission to Fedora. Thus, you need a sponsor reviewing this package (see http://fedoraproject.org/wiki/PackageMaintainers/Join#Get_Sponsored)

I'm not a sponsor, so consider the following comments as informal. 
There are some issues in the spec file that should be fixed:

- According to the source files GPLv2 is OK. However, the project homepage doesn't mention a GPL version number but links to GPLv3. Maybe you should ask upstream to fix this ambiguity

- add a short comment above %Patch0 about what the patch does

- I'd suggest to use the description text from the project website because it's a bit more detailed.

- remove -n quotatool-%{version} from %setup, as it's not necessary

- Patch0 is not applied because of the wrong parameter -p1 (remove -p1 from %patch0)

- remove INSTALL from %doc (it's not of much use in a binary package)

- The website links to the latest GPL page, where the GNU is free to make changes to it at any time. But it was released with GPLv2 which is why I went with that.

- Comment added above %Patch0

- I took snippets from the website to keep it brief

- Removed some stuff from %setup

- Removed -p1 as well

- Removed INSTALL from %doc

(In reply to comment #2)
> - The website links to the latest GPL page, where the GNU is free to make
> changes to it at any time.

Yes, that's the point I tried to make. You should inform upstream that source files and website are out of sync concerning the license. This should be fixed. It doesn't affect the package though, as the sources indicate GPLv2.

Oh, so I tell upstream so that they know about it for other packages as well? How do I notify them?

Simply write an email to the author of quotatool and inform him about the issue. You can also add a link to this review request. The email address is given on the website.

Ok, the license on the official website's page now reflects the LICENSE file in the RPM.

OK, thanks for that. Now you have to wait for a sponsor to get your package formally reviewed.

- Drop the empty & commented Requires: and BuildRequires: lines.

- Every time you update the spec file increment the Release: tag and make a comment in the changelog. So the last release was -2 and the next one is -3.

Ok, fixed the empty comments and the Release tag.

Can this be built on koji?

(In reply to comment #10)
> Can this be built on koji?  

yes, 

please try this and let me know if works.

https://fedoraproject.org/wiki/PackageMaintainers/Join#Install_the_Client_Tools_.28Koji.29

Ok, it was successfully built on dist-F11 and dist-F12.

A few minor issues of note:

* When you make an updated package, it is useful to post updated links to the new SRPM in the review bug. (I found your -3 SRPM, but it might have been overlooked by a different reviewer)

* You should try to use %{name} and %{version} in the Source0: line, this helps ensure that as you update the package, you don't accidentally build it against older source.

* While technically correct to not explicitly list the patch level when applying Patch0, I generally recommend that packagers do so, especially to make it clear to someone who might inherit this package in the future.

Basically, you'd change:

%patch0

to

%patch0 -p0

* I also generally recommend that packagers generate patches at patch level 1. If you use a suffix when making changes (e.g. Makefile.OLD), then it is simple to use the "gendiff" tool to generate a patch: 

gendiff quotatool-1.4.10 .BAD

* It is also useful to use a relevant suffix when applying the patch in the spec file:

%patch0 -p0 -b .destdir

This can come in handy when trying to regenerate patches for future updates.

* Also, patch naming is useful. You named your patch "Makefile.patch", I would recommend something like: quotatool-1.4.10-DESTDIR.patch

* You should go ahead and send patches upstream whenever appropriate. In this case, enabling DESTDIR support is useful for all distributions, and harmless if it is not set, so it is appropriate to send it upstream. You can do this by emailing it to their mailing list, or opening a bug in their bug tracker. You should also put a link to wherever you sent the patch to upstream as a comment in the spec file:

# Sent upstream:
# http://foo.com/bar/bug12345

* One very minor point: In Fedora 10+, it is no longer necessary to call:

rm -rf $RPM_BUILD_ROOT 

immediately after %install. RPM now does this for you as the first step of %install.

* You should get in the habit of running rpmlint against the SRPM and binary RPMs. For example, when I ran:

[spot@pterodactyl ~]$ rpmlint /home/spot/rpmbuild/SRPMS/quotatool-1.4.10-3.fc12.src.rpm /home/spot/rpmbuild/RPMS/x86_64/quotatool-1.4.10-3.fc12.x86_64.rpm /home/spot/rpmbuild/RPMS/x86_64/quotatool-debuginfo-1.4.10-3.fc12.x86_64.rpm

quotatool.src:48: W: macro-in-%changelog doc
quotatool.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 12)
3 packages and 0 specfiles checked; 0 errors, 2 warnings.

Both of these are minor errors, but easily corrected. (You should use %% to escape out any macros used in changelog entries)

Please show me an updated SRPM that incorporates fixes for the above items, and I will finish the review and sponsor you.

quotatool-1.4.10-4 seems clean now with rpmlint.

SPEC file: http://xiashing.fedorapeople.org/SPECS/quotatool.spec
SRPM: http://xiashing.fedorapeople.org/SRPMS/quotatool-1.4.10-4.fc11.src.rpm

Your patch is backwards, and does not apply.

When making the patch, before making any code changes, make a backup of the file you plan to edit:

cp Makefile Makefile.old

Then, edit the Makefile (leave Makefile.old alone):

vi Makefile
# make changes here and save as Makefile

Then, generate the patch. Somehow, you did it backwards. :)

(In reply to comment #15)
> Your patch is backwards, and does not apply.

Maybe it's just been generated (manually) the wrong way?
 $ diff new orig
instead of
 $ diff orig new
(the way it's supposed to be done according to the man page).

Yeh, that's what I was asking about on IRC. It seems correct that we are allowed to edit the Makefile, and then Makefile.OLD is the original Makefile.

I was under the impression that you create something like Makefile2 which differs from Makefile, and generate a patch from that, to patch Makefile.

Anyway, new build:
SPEC file: http://xiashing.fedorapeople.org/SPECS/quotatool.spec
SRPM: http://xiashing.fedorapeople.org/SRPMS/quotatool-1.4.10-5.fc11.src.rpm

Review
=======
Good:

- rpmlint checks return nothing
- package meets naming guidelines
- package meets packaging guidelines
- license (GPLv2) OK, text in %doc
- spec file legible, in am. english
- source matches upstream (9fbad0c03c21463c1529365764ab1e636b4a5c5aed95a7d3ed67b49cd39d7179)
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file 

Licensing Note: The source code does not contain any license attribution aside from one reference:

   fprintf (stderr, "  Distributed under the GNU General Public License\n");

Technically, every source code file should have a comment header in which the license of the code is given, something like:

/*
Copyright (C) yyyy  name of author

This program is free software; you can redistribute it and/or
modify it under the terms of the GNU General Public License
as published by the Free Software Foundation; version 2
of the License.

This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
GNU General Public License for more details.

You should have received a copy of the GNU General Public License
along with this program; if not, write to the Free Software
Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
*/

You should ask upstream to add this header to all of their source files (with a correct copyright date and author name).

Technically, the code without these attributions is under "GPL+", because the GPL explicitly says that: "If the Program does not specify a version number of this License, you may choose any version ever published by the Free Software Foundation."

However, we know from the website that the author's intent is for this code to be GPLv2 only, so it is fine to leave it tagged as such. By adding the copyright statement to his source files, he will also be specifying the version of the GPL he intends to use, and closing this ambiguity.

*****
This package is approved, and I will sponsor you.

New Package CVS Request
=======================
Package Name: quotatool
Short Description: summary of package
Owners: Xia Shing Zee
Branches: F-11 F-12 EL-5
InitialCC: xiashing, spot

Xia, you need to do a few things here.

1. You need to use your username in the "Owners" field, not your real name. :)
2. You don't need to specify F-12, that is "devel", and you always get that branch.
3. You didn't put the summary of the package, you put "summary of package".
4. You didn't change the fedora-cvs flag to ?, so no one will see this request.

Try again? :)

I just realized all of that, and got a mid-air collision when I tried to change the fedora-cvs flag ;)

This should do it:

New Package CVS Request
=======================
Package Name: quotatool
Short Description: quotatool is a utility to set filesystem quotas from the commandline. It is suitable for use in scripts and other non-interactive situations.
Owners: xiashing
Branches: F-11 EL-5
InitialCC: xiashing, spot

cvs done

Uhm, and the short description should be the same as the summary in the spec file, i.e. "A utility to set filesystem quotas".

Anyway, once you've imported the package the short description should be automatically updated from the spec file summary.

New changes to the .spec file so that it builds successfully on ppc64 with koji:

SPEC: http://xiashing.fedorapeople.org/SPECS/quotatool.spec
SRPM: http://xiashing.fedorapeople.org/SRPMS/quotatool-1.4.10-6.fc11.src.rpm

Short description updated as well as suggested

New Package CVS Request
=======================
Package Name: quotatool
Short Description: A utility to set filesystem quotas.
Owners: xiashing
Branches: F-11 EL-5
InitialCC: xiashing, spot

Additional changes that should have been in the last release:

SPEC: http://xiashing.fedorapeople.org/SPECS/quotatool.spec
SRPM: http://xiashing.fedorapeople.org/SRPMS/quotatool-1.4.10-7.fc11.src.rpm

And the Updated CVS Request:

Update Package CVS Request
=======================
Package Name: quotatool
Short Description: A utility to set filesystem quotas.
Owners: xiashing
Branches: F-11 EL-5
InitialCC: xiashing, spot

After the creation of CVS module you are doing all updates on your own, without new CVS requests in bugzilla, please read https://fedoraproject.org/wiki/PackageMaintainers/UpdatingPackageHowTo

quotatool-1.4.10-7.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/quotatool-1.4.10-7.fc11

Dan, thanks for that, I temporarily forgot that once a CVS module is created, the owner can update it.

quotatool-1.4.10-7.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/quotatool-1.4.10-7.el5

quotatool-1.4.10-7.el5 has been pushed to the Fedora EPEL 5 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update quotatool'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/EL-5/FEDORA-EPEL-2009-0472

quotatool-1.4.10-7.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

quotatool-1.4.10-7.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.