Bug 464047 - (libprojectM) Review Request for libprojectM
Review Request for libprojectM
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks: libprojectM-qt projectM-libvisual
  Show dependency treegraph
 
Reported: 2008-09-25 23:06 EDT by Jameson
Modified: 2008-11-22 11:54 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-11-07 03:58:22 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Jameson 2008-09-25 23:06:42 EDT
SPEC URL:  http://www.vtscrew.com/libprojectM.spec
SRPM URL:  http://www.vtscrew.com/libprojectM-1.2.0-2.fc9.src.rpm

Description:
projectM is an awesome music visualizer. There is nothing better in the world
of Unix. projectM's greatness comes from the hard work of the community. Users
like you can create presets that connect music with incredible visuals.
projectM is an LGPL'ed reimplementation of Milkdrop under OpenGL. All projectM
requires is a video card with 3D acceleration and your favorite music.

rpmlint warnings:
libprojectM.i386: W: wrong-file-end-of-line-encoding /usr/share/doc/libprojectM-1.2.0/ChangeLog
libprojectM.i386: W: unused-direct-shlib-dependency /usr/lib/libprojectM.so.2.00 /usr/lib/libfreetype.so.6
libprojectM.i386: W: unused-direct-shlib-dependency /usr/lib/libprojectM.so.2.00 /usr/lib/libGLU.so.1
libprojectM.i386: W: unused-direct-shlib-dependency /usr/lib/libprojectM.so.2.00 /usr/lib/libSM.so.6
libprojectM.i386: W: unused-direct-shlib-dependency /usr/lib/libprojectM.so.2.00 /usr/lib/libICE.so.6
libprojectM.i386: W: unused-direct-shlib-dependency /usr/lib/libprojectM.so.2.00 /usr/lib/libX11.so.6
libprojectM.i386: W: unused-direct-shlib-dependency /usr/lib/libprojectM.so.2.00 /usr/lib/libXext.so.6

This is my first attempt, so be kind.  I've learned a lot with this, and am willing to learn more.
Comment 1 Jason Tibbitts 2008-10-01 11:47:12 EDT
Shouldn't these projectM review tickets have some dependencies between them?  It doesn't seem possible to review projectM-jack without first having at least libprojectM-qt in the distribution first.
Comment 2 Jameson 2008-10-01 20:06:49 EDT
I wasn't sure if I should submit them all at once so functionality could be reviewed or if I should wait until the dependancies made it into Fedora before submitting the others, so they could be tested more easily.  Unfortunately the libprojectM package has pretty much no functionality on its own.
Comment 3 Jason Tibbitts 2008-10-01 20:25:08 EDT
Well, sure, but the point is that you use the "Depends on" fields of the various tickets to indicate the order in which they must be reviewed.  I think this ticket should be the first one but without looking at the specs or trying to build, I can't be sure.
Comment 4 Jameson 2008-10-01 20:39:26 EDT
Oh, I got ya.  Thanks.  Like I said, I'm new to this.
Comment 5 Orcan Ogetbil 2008-10-25 03:04:14 EDT
Hey Jameson, I'd like to help you get the package in shape. Here are my notes:

* rpmlint output: I didn't get those unused-direct-shlib-dependency warnings but I got this output:

libprojectM.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/libprojectM-1.2.0/ChangeLog
libprojectM.x86_64: W: dangling-relative-symlink /usr/share/projectM/fonts/Vera.ttf ../../fonts/bitstream-vera/Vera.ttf
libprojectM.x86_64: W: dangling-relative-symlink /usr/share/projectM/fonts/VeraMono.ttf ../../fonts/bitstream-vera/VeraMono.ttf
libprojectM-devel.x86_64: W: no-documentation

The first warning can be fixed via 
    sed -i 's/\r//' ChangeLog

The other warnings can be ignored.

* Source0 link needs corrected.

* The license should be LGPLv2+ since the source files say "any later version". Also please recommend upstream to put a license file in their tarball.

* Afaik we usually don't put a BuildRequires (BR) in sub-packages. BR: pkgconfig is redundant anyways because ftgl-devel will pull that up. Also I don't think BR:kdelibs is necessary. Am I wrong?

* Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability).

* Requires: ftgl and glew are also redundant. RPM should pick them up itself.

* Is Requires: cmake (in the devel subpackage) really necessary?

* You are creating but not owning %{_datadir}/projectM/ and %{_includedir}/%{name}/ directories. Those need fixed.

* This part can be taken off:
   %if "%{_lib}" == "lib64"
           -DLIB_SUFFIX=64 \
   %endif
   .

* Note that, ideally, packagers should put comments in the SPEC files to tell what the patches do, or why they are there.

----
Well that's all for now. I hope this will be useful for you.
Comment 6 Jameson 2008-10-29 00:47:35 EDT
Ok...  I've got updated files in the same places.  I believe I have corrected all the issues above.  I'll e-mail upstream, and see if I can get them to include the license in the tarball, and use the corrected ChangeLog if possible.  Thanks for the help.
Comment 7 Orcan Ogetbil 2008-10-29 01:18:06 EDT
No problem, but you should provide the new links, since for every update you need to bump the release version and that would make at least the old SRPM link obsolete.
Also, for every update you need to explain what you did in the %changelog.
Comment 8 Jameson 2008-10-29 01:29:42 EDT
I didn't realize that it was important to bump the release version while still under review.  the new SRPM can be found here:

http://www.vtscrew.com/libprojectM-1.2.0-3.fc9.src.rpm

Thanks.
Comment 9 Mamoru TASAKA 2008-10-30 16:20:40 EDT
Well, as this is NEEDSPONSOR ticket and from Orcan's request
(Orcan, thanks for your pre-remark)

! License
  - Well, this can be ignored for now but actually
    Parser.cpp looks like under GPLv2+ because of
    copy and paste mistake (perhaps...)
    Please ask upstream to fix the license clause
    on this file.

* About Patch1 (perhaps CRLF issue)
  - Well, such "huge" patch is not desired (here I say
    "huge" because this patch changes the whole of the
    text file). 
    This patch gives us the impression that you changed 
    the whole of upstream ChangeLog by your some intention.

    For this case please use "sed" instead of creating patch
    to aviod creating such huge patch and to show that
    you actually used sed to fix CRLF issue and did not
    do anything.

* cmake build log
  - For cmake please use
---------------------------------------------------------
make %{?_smp_mflags} VERBOSE=1
---------------------------------------------------------
    to make build.log more verbose (so that we can check
    if Fedora specific compilation flags are correctly honored,
    for example).

* pkgconfig .pc file
  - libprojectM.pc.in contains
---------------------------------------------------------
     1  prefix=@CMAKE_INSTALL_PREFIX@
     2  exec_prefix=@CMAKE_INSTALL_PREFIX@
     3  libdir=@CMAKE_INSTALL_PREFIX@/lib
---------------------------------------------------------
    It looks that libdir is expanded as /usr/lib even on 64 bits
    architecture so this is perhaps wrong.
Comment 10 Jameson 2008-10-31 22:06:47 EDT
Update SPEC in the same location, and new SRPM:  http://www.vtscrew.com/libprojectM-1.2.0-4.fc9.src.rpm

I haven't got with upstream about that license issue, yet, but I will when I pass them my patch for the .pc file issue.
Comment 11 Mamoru TASAKA 2008-11-01 08:40:49 EDT
(In reply to comment #10)
> Update SPEC in the same location, and new SRPM: 
> http://www.vtscrew.com/libprojectM-1.2.0-4.fc9.src.rpm

Seems 404...
Comment 12 Jameson 2008-11-03 18:02:43 EST
Sorry about that...  I must've been on the wrong machine when I tried to copy it, or something.  It's there now.
Comment 13 Mamoru TASAKA 2008-11-04 11:14:17 EST
For -4:

* About Patch1 (libprojectM-pc-libsuffix.patch)
  - Well, with this patch LIB_INSTALL_DIR itself is expanded
    as /usr/lib{,64}, so the created libprojectM.pc contains
-------------------------------------------------
     3  libdir=/usr//usr/lib
-------------------------------------------------
    (on i386). The correct line is "libdir=/usr/lib" (on i386).

* About removing CRLF line
-------------------------------------------------
sed -i 's/\r//' ChangeLog
-------------------------------------------------
  - Please move this line to %prep (for --short-circuit issue)

Other things seem okay.
Comment 14 Jameson 2008-11-04 19:52:46 EST
Yeah, I upset myself with that libprojectM.pc issue because I originally planed on adding @LIBSUFFIX@ to that line, but I only defined the patch in the SPEC and never executed it.  I didn't know why it didn't work, so I switched it to @LIB_INSTALL_DIR@, and was in disbelief that it still wasn't working until I noticed the patch command was missing.  That's why it wound up the way it did.

Anyway, all is well now.  The SPEC is in the usual location, and the SRPM is here:
http://www.vtscrew.com/libprojectM-1.2.0-5.fc9.src.rpm

Thanks for all the help.
Comment 15 Mamoru TASAKA 2008-11-05 05:59:18 EST
Okay.

+ This package itself is now okay
+ As written on
  http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored
  new packagers to be sponsored are required to either
  - submit another review request
  - or do a pre-review of other person's review request

  You have already other review requests. While there may be
  some issues to fix, they seems good to some extent.

---------------------------------------------------------------
    This package (libprojectM) is APPROVED by mtasaka
---------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
After you request for sponsorship a mail will be sent to sponsor 
members automatically (which is invisible for you) which notifies 
that you need a sponsor. After that, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.

If you want to import this package into Fedora 8/9, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Orcan, thanks you for pre-review. If you want to review other libprojectM
related packages, it is okay now and I would appreciate it.
Also thanks to Jason for pre-review.
Comment 16 Jameson 2008-11-05 06:43:17 EST
I have applied for sponsorship.  My Fedora account is ImNtReal.  I verified with upstream that this entire package should be LGPL, so that must have been a typo in Parser.cpp.  Patching the license in the source seems like it would be inappropriate for us, so unless you have any other advice about that, I'll leave it to be fixed upstream.
Comment 17 Mamoru TASAKA 2008-11-05 07:03:35 EST
Okay. Now I am sponsoring you. Please follow "Join" wiki again.

(In reply to comment #16)
> Patching the license in the source seems like it would be
> inappropriate for us, so unless you have any other advice about that, I'll
> leave it to be fixed upstream.

Yes, patching against license text is inappropriate.
While I think I can ignore this for now, please ask the upstream
to fix typo in the next version.
Comment 18 Orcan Ogetbil 2008-11-05 09:46:21 EST
Thank you Mamoru,
I will start reviewing the other projectM packages sometime this week.
Comment 19 Jameson 2008-11-05 14:56:26 EST
New Package CVS Request
=======================
Package Name: libprojectM
Short Description: music visualization library
Owners: imntreal
Branches: F-8 F-9
InitialCC: imntreal
Comment 20 Kevin Fenzi 2008-11-05 17:39:42 EST
cvs done.
Comment 21 Jameson 2008-11-07 00:56:28 EST
Package Change Request
======================
Package Name: libprojectM
New Branches: F-10
Owners: imntreal

I need the new branch.  Thanks.
Comment 22 Mamoru TASAKA 2008-11-07 01:15:08 EST
Currently mass branching is proceeding and the request for F-10 branch is
not needed.
Comment 23 Mamoru TASAKA 2008-11-07 02:05:59 EST
CVS outrage is over and F-10 branch is created for libprojectM.
So now you can rebuild libprojectM for also F-11/10.
Comment 24 Jameson 2008-11-07 02:29:21 EST
I just did a cvs update, but I still don't seem to have a F-10 branch.
Comment 25 Jameson 2008-11-07 02:35:21 EST
Nevermind, I re-ran the fedora-cvs script, and it found it.
Comment 26 Fedora Update System 2008-11-07 02:58:15 EST
libprojectM-1.2.0-5.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/libprojectM-1.2.0-5.fc8
Comment 27 Fedora Update System 2008-11-07 02:59:33 EST
libprojectM-1.2.0-5.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/libprojectM-1.2.0-5.fc9
Comment 28 Fedora Update System 2008-11-07 03:00:31 EST
libprojectM-1.2.0-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/libprojectM-1.2.0-5.fc10
Comment 29 Mamoru TASAKA 2008-11-07 03:58:22 EST
Okay, thanks.
Comment 30 Fedora Update System 2008-11-07 21:11:28 EST
libprojectM-1.2.0-5.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 31 Fedora Update System 2008-11-07 21:11:53 EST
libprojectM-1.2.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.
Comment 32 Fedora Update System 2008-11-22 11:54:38 EST
libprojectM-1.2.0-5.fc10 has been pushed to the Fedora 10 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.