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 Review | Assignee: | Orcan Ogetbil <oget.fedora> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | 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
* 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 . (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 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. ping? (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 ? 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? 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. 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. (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 Another happy ending :) ------------------------------------------------- This package (scummvm-tools) is APPROVED by oget. ------------------------------------------------- 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: cvs done. 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 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. |