Bug 464047 (libprojectM)

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.

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.

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.

Oh, I got ya.  Thanks.  Like I said, I'm new to this.

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.

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.

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.

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.

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.

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.

(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...

Sorry about that...  I must've been on the wrong machine when I tried to copy it, or something.  It's there now.

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.

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.

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.

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.

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.

Thank you Mamoru,
I will start reviewing the other projectM packages sometime this week.

New Package CVS Request
=======================
Package Name: libprojectM
Short Description: music visualization library
Owners: imntreal
Branches: F-8 F-9
InitialCC: imntreal

cvs done.

Package Change Request
======================
Package Name: libprojectM
New Branches: F-10
Owners: imntreal

I need the new branch.  Thanks.

Currently mass branching is proceeding and the request for F-10 branch is
not needed.

CVS outrage is over and F-10 branch is created for libprojectM.
So now you can rebuild libprojectM for also F-11/10.

I just did a cvs update, but I still don't seem to have a F-10 branch.

Nevermind, I re-ran the fedora-cvs script, and it found it.

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

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

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

Okay, thanks.

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.

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.

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.