Bug 467293 (scummvm-tools)

Summary: Review Request: scummvm-tools - Tools for scummVM / S.C.U.M.M scripting language
Product: [Fedora] Fedora Reporter: Lucian Langa <lucilanga>
Component: Package ReviewAssignee: Orcan Ogetbil <oget.fedora>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, hdegoede, notting, oget.fedora, tcallawa
Target Milestone: ---Flags: oget.fedora: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-10-30 06:25: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 Lucian Langa 2008-10-16 17:58:28 UTC
Spec URL: http://lucilanga.fedorapeople.org/scummvm-tools.spec
SRPM URL: http://lucilanga.fedorapeople.org/scummvm-tools-0.12.0-1.fc10.src.rpm
Description: This is a collection of various tools that may be useful to use in
conjunction with ScummVM.

Comment 1 Orcan Ogetbil 2008-10-17 04:59:56 UTC
* The description needs to be descriptive.

*  Won't you need
   Requires:	scummvm = %version
instead of
   Requires:	scummvm >= 0.12.0

* License should be GPLv2+ as told by the source files.

* In the .desktop file 
  - put a "Generic Name:" with a very brief description. 
  - also it would be more comlpliant with the existing ScummVM package if you make the "Name: ScummVM Tools".
  - afaik you shouldn't have Application as a category in a .desktop file.

* Compiler flags need to be appropriate. This is C++ code, so in order to honor Fedora specific compilation flags you will most likely need to play with the CXXFLAGS. Make sure they are not overridden by the other CXXFLAGS in the Makefile. You shouldn't set CFLAGS as it is irrelevant for this package.

* Did you submit the patch upstream?

* As far as I understood, this application is primarily used to extract information from non-free software, although most of which are now abandonware. I am not sure if this is OK to release in Fedora. I am blocking FE-Legal .

Comment 2 Lucian Langa 2008-10-17 08:55:52 UTC
(In reply to comment #1)
> * The description needs to be descriptive.
updated description


> *  Won't you need
>    Requires: scummvm = %version
> instead of
>    Requires: scummvm >= 0.12.0
fixed


> * License should be GPLv2+ as told by the source files.
updated license


> * In the .desktop file 
>   - put a "Generic Name:" with a very brief description. 
>   - also it would be more comlpliant with the existing ScummVM package if you
> make the "Name: ScummVM Tools".
>   - afaik you shouldn't have Application as a category in a .desktop file.
fixed


> * Compiler flags need to be appropriate. This is C++ code, so in order to honor
> Fedora specific compilation flags you will most likely need to play with the
> CXXFLAGS. Make sure they are not overridden by the other CXXFLAGS in the
> Makefile. You shouldn't set CFLAGS as it is irrelevant for this package.
fixed


> * Did you submit the patch upstream?
patch submitted upstream


Spec URL: http://lucilanga.fedorapeople.org/scummvm-tools.spec
SRPM URL: http://lucilanga.fedorapeople.org/scummvm-tools-0.12.0-2.fc10.src.rpm

Comment 3 Tom "spot" Callaway 2008-10-17 13:44:37 UTC
Unfortunately, this one isn't going to fly as is. In the code, it includes an mp3 encoder and decoder (code copied from lame), which is patent encumbered and non-permissable in Fedora.

In order to make this acceptable for Fedora, you'd have to modify the source code such that it is no longer able to encode or decode in mp3 format, then make a new, mp3-free, tarball and use that for the base of the SRPM.

I understand that may be too much of an inconvenience, so instead, you might consider submitting this package to rpmfusion (http://rpmfusion.org/).

As for the package itself, I think it is acceptable in principle.

Comment 4 Orcan Ogetbil 2008-10-29 03:13:12 UTC
ping?

Comment 5 Lucian Langa 2008-10-29 06:18:15 UTC
(In reply to comment #3)
> Unfortunately, this one isn't going to fly as is. In the code, it includes an
> mp3 encoder and decoder (code copied from lame), which is patent encumbered and
> non-permissable in Fedora.

The code does not actually include mp3 encoder/decoder. It just uses lame (using system()) if present for encoding/decoding mp3. See compress.cpp encodeAudio().
Is there any problem with that ?

Comment 6 Jason Tibbitts 2008-10-29 15:57:10 UTC
I have to say, I'm not seeing in the tarball the actual code copied from lame that spot refers to above.  I see references to mp3 in the code, of course, but everything seems to come down to either option parsing or calls to compress.cpp::encodeAudio which builds command lines and calls out to external programs (lame, oggenc, flac).

What am I missing?

Comment 7 Tom "spot" Callaway 2008-10-29 16:42:35 UTC
I spoke too soon. A few other people pointed out that none of the code doing the decode/encode was actually copied, only format headers.

Lifting FE-Legal.

Comment 8 Orcan Ogetbil 2008-10-29 17:11:23 UTC
There is still a little issue with the compilation flags.
The %optflags' -O2 and -fexceptions flags get overridden during the build process (with -O and -fno-exceptions). You will most probably need to patch or sed the Makefile to fix it.

Comment 9 Lucian Langa 2008-10-29 18:18:10 UTC
(In reply to comment #8)
> There is still a little issue with the compilation flags.
> The %optflags' -O2 and -fexceptions flags get overridden during the build
> process (with -O and -fno-exceptions). You will most probably need to patch or
> sed the Makefile to fix it.

fixed (sed)

http://lucilanga.fedorapeople.org/scummvm-tools.spec
http://lucilanga.fedorapeople.org/scummvm-tools-0.12.0-3.fc10.src.rpm

Comment 10 Orcan Ogetbil 2008-10-29 19:16:23 UTC
Another happy ending :)

-------------------------------------------------
This package (scummvm-tools) is APPROVED by oget.
-------------------------------------------------

Comment 11 Lucian Langa 2008-10-29 19:29:07 UTC
Thank you.

New Package CVS Request
=======================
Package Name: scummvm-tools
Short Description: Tools for scummVM / S.C.U.M.M scripting language
Owners: lucilanga
Branches: F-9 F-10 EL-5
InitialCC:

Comment 12 Kevin Fenzi 2008-10-29 20:48:48 UTC
cvs done.

Comment 13 Fedora Update System 2008-10-30 06:24:09 UTC
scummvm-tools-0.12.0-5.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/scummvm-tools-0.12.0-5.fc9

Comment 14 Fedora Update System 2008-11-19 14:56:56 UTC
scummvm-tools-0.12.0-5.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.