Bug 467293 - (scummvm-tools) Review Request: scummvm-tools - Tools for scummVM / S.C.U.M.M scripting language
Review Request: scummvm-tools - Tools for scummVM / S.C.U.M.M scripting language
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-16 13:58 EDT by Lucian Langa
Modified: 2008-11-19 09:56 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-30 02:25:36 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Lucian Langa 2008-10-16 13:58:28 EDT
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 00:59:56 EDT
* 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 04:55:52 EDT
(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 09:44:37 EDT
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-28 23:13:12 EDT
ping?
Comment 5 Lucian Langa 2008-10-29 02:18:15 EDT
(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 11:57:10 EDT
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 12:42:35 EDT
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 13:11:23 EDT
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 14:18:10 EDT
(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 15:16:23 EDT
Another happy ending :)

-------------------------------------------------
This package (scummvm-tools) is APPROVED by oget.
-------------------------------------------------
Comment 11 Lucian Langa 2008-10-29 15:29:07 EDT
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 16:48:48 EDT
cvs done.
Comment 13 Fedora Update System 2008-10-30 02:24:09 EDT
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 09:56:56 EST
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.

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