[Bug 562585] Review Request: ccd2iso - CloneCD image to ISO image file converter

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=562585

Orcan 'oget' Ogetbil <oget.fedora at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |oget.fedora at gmail.com
         AssignedTo|nobody at fedoraproject.org    |oget.fedora at gmail.com
               Flag|                            |fedora-review?

--- Comment #10 from Orcan 'oget' Ogetbil <oget.fedora at gmail.com> 2010-07-08 22:02:31 EDT ---
I am taking the review. The package is mostly in good shape. There are minor
things to fix. I made the review a little verbose since this is your first
package.

Since you need to be sponsored, we expect a little more from you. To show that
you understand and work comfortably with the Fedora guidelines, you will need
to do either some informal reviews on packages that are awaiting a review (this
is the preferred method)
   http://fedoraproject.org/PackageReviewStatus/
or submit some other packages for review, that are preferably different type of
packages. For more detail, see
   http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

Here is the review:

- rpmlint says:
   ccd2iso.x86_64: W: no-manual-page-for-binary ccd2iso
It would be nice to have a man page. But this is not a blocker.

! Usually the EOL encoding erros are fixed via "sed" or "dos2unix". See
    http://fedoraproject.org/wiki/Packaging/Guidelines#Rpmlint_Errors
A typical way of doing it is:
   sed 's/\r//' TODO > TODO.tmp
   touch -r TODO TODO.tmp
   mv -f TODO.tmp TODO
or
   dos2unix -k TODO
So that you preserve the timestamp of the file. Note that for dos2unix, you
need to add a BuildRequires.
I would say the patch is okay since the file is really small, but I would have
preferred one of the above two.

! We don't package the INSTALL files, since they usually tell us how to compile
the source. This is not relevant for an RPM user.

* The license of the package is determined as follows:
- You look at the license file (if it is missing you notify upstream). This one
says GPLv2.
- We look at the source code. The source code is in directory src/ in this
case.
- The header of the source code files carry the phrase:
 *   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; either version 2 of the License, or     * 
 *   (at your option) any later version.                                   * 

Since it says "or any later version", the license of the package must be
GPLv2+.
(Sometimes the source code does not carry the "or any later version" phrase. In
that case we set the license tag to GPLv2. Sometimes the source code does not
even specify the GPL version. In this case, even if the COPYING file says GPLv2
or GPLv3, we set the license tag to GPL+, and we notify upstream to add the GPL
versions to the source file headers)

For more information, see
   http://fedoraproject.org/wiki/Licensing#Good_Licenses
scroll down to "GNU General Public License (no version)"

! You can use the %{name} macro in %files to keep macro usage consistent.

* The compiler gives these warnings:
   ccd2iso.c:61: warning: implicit declaration of function 'strcmp'
This can be avoided by adding a
   #include <string.h>

   ccd2iso.c:97: warning: format '%d' expects type 'int', but argument 2 has 
                          type 'long unsigned int'
Replacing the %d with %zd will fix this warning.

You can write a patch to fix these and send it upstream.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.