Bug 524423 - Review Request: ciso - iso to cso converter
Summary: Review Request: ciso - iso to cso converter
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-09-20 01:10 UTC by Pierre Dorbais
Modified: 2010-06-23 17:46 UTC (History)
5 users (show)

Fixed In Version: ciso-1.0.0-3.fc12
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-04-29 17:06:24 UTC
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
patch reformated (967 bytes, patch)
2009-10-20 13:17 UTC, Haïkel Guémar
no flags Details | Diff

Description Pierre Dorbais 2009-09-20 01:10:52 UTC
Spec URL: http://chdorblog.free.fr/ciso.spec
SRPM URL: http://chdorblog.free.fr/ciso-1.0.0-1.fc10.src.rpm
Description: ciso is a little tool to compress/uncompress Sony PSP ISO image to CSO (Commpressed ISO)

Comment 1 Pierre Dorbais 2009-09-20 12:04:43 UTC
That's my first package and I've no sponsor.

rpmlint:

chdorb@chdorb-desktop:~/rpmbuild$ rpmlint SPECS/ciso.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
chdorb@chdorb-desktop:~/rpmbuild$ rpmlint RPMS/i386/ciso-1.0.0-1.fc10.i386.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
chdorb@chdorb-desktop:~/rpmbuild$ rpmlint SRPMS/ciso-1.0.0-1.fc10.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Comment 2 Ionuț Arțăriși 2009-09-23 11:30:21 UTC
Hello,

I'm not in the packager group yet and so I cannot approve your package. Nor can I be your sponsor. I'm just trying to review.

Here are my suggestions:

the %{?_smp_mflags} is useless as the makefile doesn't interpret it

The string.h patch should probably go upstream.


Here are some other issues from the Review Guidelines that I've found applicable:

#  MUST: rpmlint must be run on every package. The output should be posted in the review.[1]
ciso-debuginfo.i586: E: debuginfo-without-sources
3 packages and 0 specfiles checked; 1 errors, 0 warnings.


# MUST: The spec file must be written in American English. [5]
Some typos: Patch instead of Pach and the description might sound better like this:
A small tool to compress/decompress ISO to/from CSO (Compressed ISO for PSP)


# MUST: Header files must be in a -devel package. [20]

Also: I could transform an iso to cso and back again, but I don't know how to test that the CSO isn't corrupted.

Good luck!

Comment 3 Pierre Dorbais 2009-09-26 18:28:49 UTC
I made some changes

Spec URL: http://chdorblog.free.fr/ciso.spec
SRPM URL: http://chdorblog.free.fr/ciso-1.0.0-1.fc10.src.rpm


chdorb@chdorb-desktop:~/rpmbuild$ rpmlint RPMS/i386/ciso-1.0.0-1.fc10.i386.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
chdorb@chdorb-desktop:~/rpmbuild$ rpmlint RPMS/i386/ciso-debuginfo-1.0.0-1.fc10.i386.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
chdorb@chdorb-desktop:~/rpmbuild$ rpmlint SRPMS/ciso-1.0.0-1.fc10.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
chdorb@chdorb-desktop:~/rpmbuild$ rpmlint SPECS/ciso.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 4 Ionuț Arțăriși 2009-10-09 11:53:16 UTC
There's no need to include the `license` file in the -devel subpackage in addition to the main package.

Comment 5 Haïkel Guémar 2009-10-17 18:52:02 UTC
There's no need of a devel subpackage as I told you in real life. This package does not provide any library, just a self-contained executable.
I'll do a more formal review as soon as you upload the corrected src.rpm.

Comment 6 Pierre Dorbais 2009-10-18 16:11:47 UTC
ciso.spec and ciso-1.0.0-1.fc10.src.rpm updated.

Comment 7 Haïkel Guémar 2009-10-20 13:15:30 UTC
==== Mandatory stuff ====

   *  MUST: rpmlint must be run on every package. The output should be posted in the review.
OK
$ rpmlint -iv /var/lib/mock/fedora-11-x86_64/result/ciso-1.0.0-1.fc11.x86_64.rpm
ciso.x86_64: I: checking
1 packages and 0 specfiles checked; 0 errors, 0 warnings.


    * MUST: The package must be named according to the Package Naming Guidelines.
OK

    * MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption.
OK

    * MUST: The package must meet the Packaging Guidelines.
OK

    * MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines.
OK (GPLv2 or later)m

    * MUST: The License field in the package spec file must match the actual license. 
OK

    * MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.
OK

    * MUST: The spec file must be written in American English.
OK

    * MUST: The spec file for the package MUST be legible.
OK

    * MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.
BAD
0189fa82a1a561a9c80822eceac48efebe128f90  ciso-1.0.0.tar.gz 
0189fa82a1a561a9c80822eceac48efebe128f90  ciso-1.0.0.tar.gz-old
80e9e45e41b56fb8fe0f1a6095d301eeae5ce387  ciso-1.0.0-1.fc10.src/ciso-1.0.0.tar.gz
Please re-download source tarball from sourceforge.


    * MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
OK (compiles fine under mock for x86/x86_64, and a F-12 scratch build was fine)
http://koji.fedoraproject.org/koji/taskinfo?taskID=1756580

    * MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. 
OK (not applicable here)

    * MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
OK

    * MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
OK (no locales)

    * MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. 
OK (not applicable here)

    * MUST: Packages must NOT bundle copies of system libraries.
OK

    * MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker.
OK

    * MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. 
OK

    * MUST: A Fedora package must not list a file more than once in the spec file's %files listings. 
OK

    * MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line.
OK

    * MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). 
OK

    * MUST: Each package must consistently use macros. 
OK

    * MUST: The package must contain code, or permissable content.
OK (only code under GPLv2+)

    * MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). 
OK (no documentation)

    * MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present.
OK

    * MUST: Header files must be in a -devel package. 
OK (no library, no API ==> no devel package)

    * MUST: Static libraries must be in a -static package. 
OK (same as above)

    * MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). 
OK (not applicable here)

    * MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. 
OK (not applicable here)

    * MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} 
OK (not applicable here)

    * MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
OK (not applicable here)

    * MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation.
OK (command-line application)

    * MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. 
OK (not applicable here)

    * MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). 
OK

    * MUST: All filenames in rpm packages must be valid UTF-8. 
OK

==== Non mandatory stuff ====

    *  SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
OK (already included)

    * SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. 
OK (not mandatory)

    * SHOULD: The reviewer should test that the package builds in mock. 
OK (as stated above)

    * SHOULD: The package should compile and build into binary rpms on all supported architectures. 
OK (scratchbuild on Koji was OK)

    * SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.
OK 

    * SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. 
OK (not applicable here)

    * SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
OK (not applicable here)

    * SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb.
OK (not applicable here)

    * SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. 
OK (not applicable here

Comment 8 Haïkel Guémar 2009-10-20 13:17:03 UTC
Created attachment 365347 [details]
patch reformated

I've reformated a little your patch

Comment 9 Haïkel Guémar 2009-10-20 13:29:18 UTC
Few things:
- re-download your source tarball from sourceforge, the checksum differs with your previous tarball from tenshu.
- I've investigated about the path thing. uint32_t type and stdint.h are part of C99 standard but unfortunately visual studio compiler doesn't ship stdint.h
You can either suggest to upstream maintainer:
- include an alternative stdint.h to fix visual studio incorrect behavior, like this BSD licensed one.
http://www.azillionmonkeys.com/qed/pstdint.h
- use a modern (and free software) compiler like mingw. I've cross-compiled with the cross-compiler shipped in F-11 (mingw32 4.4.0) and it works fine (tested under wine)

First, try to submit him these solutions, if he remains stubborn (f*ck, who cares about windows ?), I'll approve your patch.
Then, fix your package.

Comment 10 Jason Tibbitts 2009-11-04 17:37:49 UTC
I note that this ticket is assigned, but the fedora-review flag is not set, which is an odd state of affairs.  However, the packager also needs a sponsor, and as fas as I can tell, hguemar is not one and thus cannot review this submission.  Did I overlook something?

Comment 11 Haïkel Guémar 2009-11-04 20:13:39 UTC
My apologies, i thought that i could do the review, then ping a sponsor to approve it but i was wrong. That's why the fedora-review flag was not set.
I've unassigned myself from the review.

To sum up the review:
- no legal problem detected.
- the patch is ok (needed for x86_64).
- packaging guidelines are respected apart tarball checksums which is easily fixable.
If any sponsor wants to review it, this is a quickie, or i can swap reviews.

Comment 12 Michael Schwendt 2010-01-15 12:57:20 UTC
* There is a special guideline for sourceforge download locations:
https://fedoraproject.org/wiki/Packaging/SourceURL


* Format string compiler warnings. Please take a look at the build.log and apply fixes.

Note that you can submit test-builds in the official Fedora Build System "koji":
http://fedoraproject.org/wiki/PackageMaintainers/Join#Install_the_Client_Tools_.28Koji.29


* When updating your src.rpm, do increase the "Release" value and add a corresponding entry to the %changelog. Here the tarball checksum matches upstream, because you've rebuilt the src.rpm silently on Oct 20th.

Comment 13 Pierre Dorbais 2010-02-09 21:11:38 UTC
- Source URL fixed
- format string compiler warnings fixed
- Release and %changelog updated

Spec URL: http://chdorblog.free.fr/ciso.spec
SRPM URL: http://chdorblog.free.fr/ciso-1.0.0-2.fc12.src.rpm

Thanks

Comment 14 Mamoru TASAKA 2010-04-18 17:42:53 UTC
I will review this in a few days.

Comment 15 Mamoru TASAKA 2010-04-19 17:18:23 UTC
Very simple package

- It is useful on Fedora CVS what you put one line between each
  %changelog entry like:
----------------------------------------------------------
%changelog
* Tue Feb  9 2010 Pierre Dorbais <pierre.dorbais@free.fr> 1.0.0-2
- Source0 URL fixed
- format string compiler warnings fixed

* Sun Oct 18 2009 Pierre Dorbais <pierre.dorbais@free.fr> 1.0.0-1
- Initial RPM release
----------------------------------------------------------

Comment 16 Pierre Dorbais 2010-04-19 19:36:47 UTC
Yes it's very simple, it's good to begin.

I add blank lines between changelog entries and updated spec and srpm.

Spec URL: http://chdorblog.free.fr/ciso.spec
SRPM URL: http://chdorblog.free.fr/ciso-1.0.0-3.fc12.src.rpm

Thank you

Comment 17 Mamoru TASAKA 2010-04-19 19:47:58 UTC
Ah, I forgot to mention that I already approved this package.

Comment 18 Pierre Dorbais 2010-04-19 20:21:25 UTC
OK

New Package CVS Request
=======================
Package Name: ciso
Short Description: ISO to CSO converter
Owners: chdorb
Branches: F-11 F-12 F-13
InitialCC:

Comment 19 Mamoru TASAKA 2010-04-20 04:53:23 UTC
Please set fedora-cvs flag to ?

Comment 20 Kevin Fenzi 2010-04-21 03:44:57 UTC
CVS done (by process-cvs-requests.py).

Comment 21 Fedora Update System 2010-04-21 20:38:55 UTC
ciso-1.0.0-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/ciso-1.0.0-3.fc11

Comment 22 Fedora Update System 2010-04-21 20:41:53 UTC
ciso-1.0.0-3.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/ciso-1.0.0-3.fc12

Comment 23 Fedora Update System 2010-04-21 20:43:25 UTC
ciso-1.0.0-3.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/ciso-1.0.0-3.fc13

Comment 24 Fedora Update System 2010-04-22 22:32:44 UTC
ciso-1.0.0-3.fc12 has been pushed to the Fedora 12 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 ciso'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/ciso-1.0.0-3.fc12

Comment 25 Fedora Update System 2010-04-22 22:41:38 UTC
ciso-1.0.0-3.fc11 has been pushed to the Fedora 11 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 ciso'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/ciso-1.0.0-3.fc11

Comment 26 Fedora Update System 2010-04-22 22:56:12 UTC
ciso-1.0.0-3.fc13 has been pushed to the Fedora 13 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 ciso'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/ciso-1.0.0-3.fc13

Comment 27 Mamoru TASAKA 2010-04-29 17:06:24 UTC
Closing.

Comment 28 Fedora Update System 2010-06-23 17:45:21 UTC
ciso-1.0.0-3.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 29 Fedora Update System 2010-06-23 17:46:03 UTC
ciso-1.0.0-3.fc12 has been pushed to the Fedora 12 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.