Bug 454220 - (germanium) Review Request: germanium - a download manager for eMusic.com
Review Request: germanium - a download manager for eMusic.com
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-06 17:45 EDT by Adam Huffman
Modified: 2009-02-10 07:51 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-10 07:51:59 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Install germanium libs somewhere in python's sitelibdir (1.18 KB, patch)
2008-07-17 14:29 EDT, Hans Ulrich Niedermann
no flags Details | Diff
screenshot of germanium (322.11 KB, image/png)
2008-09-10 13:56 EDT, Mamoru TASAKA
no flags Details

  None (edit)
Description Adam Huffman 2008-07-06 17:45:40 EDT
Spec URL: http://gilberte.verdurin.com/apache2-default/germanium.spec
SRPM URL: http://gilberte.verdurin.com/apache2-default/germanium-0.2.0-1.fc9.src.rpm
Description: Germanium will open .emp files and download the songs from eMusic.  The .emp files can be opened from the toolbar button, or passed as command-line
parameters.

This is my first package and I am looking for a sponsor.
Comment 1 Michal Nowak 2008-07-07 12:24:37 EDT
> make %{?_smp_mflags}

* Omit '%{?_smp_mflags}' in python package, this won't speed up anything. Usable
only when compiling C/C++(/Fortran) code.

> %configure --disable-mime-update --disable-update-desktop --prefix=/usr
--sysconfdir=/etc --libdir=/usr/share

* This hardwired paths are not proper. Use ones from
https://fedoraproject.org/wiki/Packaging/RPMMacros in case you really need them.
Don't forget that rpmbuild probably populates the configure variables in a good
manner.

Familiarize yourself more with https://fedoraproject.org/wiki/Packaging/Python
and python_sitelib.

* Please post results of rpmlint.
Comment 2 Adam Huffman 2008-07-07 15:37:46 EDT
Thanks for taking a look.  I've removed the smp flags - they were generated by
Emacs.

If I don't alter the paths to configure, the *.py* files go to an arch-specific
directory e.g. /usr/lib64 on my laptop.  Would this be more acceptable?:

%configure --disable-mime-update --disable-update-desktop --libdir=%{_datadir}

The original tarball didn't place anything in the system Python directories and
I thought it better not to alter it too much.  However, happy to change that if
it's what the Fedora policy specifies.

rpmlint output:

rpmlint germanium.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

rpmlint germanium-0.2.0-1.fc9.noarch.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
Comment 3 Michal Nowak 2008-07-08 03:35:10 EDT
(In reply to comment #2)
> If I don't alter the paths to configure, the *.py* files go to an arch-specific
> directory e.g. /usr/lib64 on my laptop.  Would this be more acceptable?:

No. Arch-independent files should not be treated this way. Python sitelib is the
place I'd expect them. Feel free to ask on #fedora-devel channel. 

If the paths are hardwired somehow, fix them with sed. Look at this approved
spec file, it shows how to pack python app:
http://nicoleau.fabien.free.fr/rpms/SPECS/clive.spec


> I thought it better not to alter it too much.  However, happy to change that if
> it's what the Fedora policy specifies.

Frankly speaking, I am not 100% sure but I'd move them to site dir.

> 
> rpmlint output:

Run also lint on srpm pkg.
Comment 4 Hans Ulrich Niedermann 2008-07-08 14:00:24 EDT
I would expect *.py, *.pyc and *.pyo in the Python sitelib dir as well, but
upstream is using improper install dirs in his Makefile.am for some reason
unknown to me.

The Fedora policies allow installing app specific data into /usr/share/%{name},
though, so that would appears to be a valid interim solution until upstream uses
Automake's pkgpythondir_PYTHON features as intended instead of some home-baked
libdir magic which happens to work on upstream's machine.

The only way to fix that, though, is to fix upstream's Makefile.am and re-run
automake&Co. - which we can IMHO avoid for now by installing the stuff into
/usr/share/germanium at least for now.
Comment 5 Adam Huffman 2008-07-08 19:37:09 EDT
I've run rpmlint on the .src.rpm and it doesn't find anything wrong.

I'll contact the upstream maintainer about changing the way the app is
installed, though I would prefer not to alter such things extensively on my
first package...
Comment 6 Michal Nowak 2008-07-09 05:56:38 EDT
Definitely do so. It's generally nice to have package in good shape from initial
commit. If upstream agrees on fix you might incorporate it as a patch to recent
version or wait for new one.
Comment 7 Hans Ulrich Niedermann 2008-07-09 16:25:30 EDT
I have played a little with the germanium code. The result is at
http://ndim.fedorapeople.org/packages/germanium/

http://ndim.fedorapeople.org/packages/germanium/rpm-patches/ is what I would
change in the RPM with the current unchanged germanium-0.2.0 tarball.

http://ndim.fedorapeople.org/packages/germanium/upstream-patches/ are the
patches against germanium-0.2.0 I'd like upstream to apply. Adam, do you want to
feed these patches to upstream and Cc me or should I do that?

http://ndim.fedorapeople.org/packages/germanium/0.2.0-1.fc9/ is what my modified
RPM with the original germanium-0.2.0 would look like.

http://ndim.fedorapeople.org/packages/germanium/0.2.0.1-1.fc9/ is what my
modified RPM with the germanium build system fixed would look like.

Feel free to pick out whatever pieces you want.
Comment 8 Adam Huffman 2008-07-09 16:55:14 EDT
Thanks a lot, Hans.  You should have seen a copy of the mail I sent to the
upstream author with a link to your patches.

I'd feel a bit cheeky taking credit for all the work you've done...
Comment 9 Matt Good 2008-07-10 03:19:35 EDT
Thanks for the patches.  I've applied most verbatim, but I've moved the Python
modules from $(libdir) to $(datadir) instead of the suggested $(pkgpythondir). 
Python's "site-packages" dir is meant for actual Python libraries, so I don't
want to dump modules internal to my app in there.  If I was going to put
anything in site-packages I'd want to organize it as a regular Python package
first instead of a bunch of individual modules.

The updates are in darcs now and I've posted an RC:
http://projects.matt-good.net/darcs/emusic-gnome/
http://matt-good.net/files/releases/germanium/germanium-0.2.1-rc1.tar.gz
Comment 10 Matt Good 2008-07-10 03:27:04 EDT
Oh, and do you know what macro to use for the #! line of the script to get the
absolute path of the python binary used to build the package?  I just realized
that the installed script should be using that rather than relying on /usr/bin/env.
Comment 11 Hans Ulrich Niedermann 2008-07-10 10:51:13 EDT
(In reply to comment #9)
> Thanks for the patches.  I've applied most verbatim, but I've moved the Python
> modules from $(libdir) to $(datadir) instead of the suggested $(pkgpythondir). 
> Python's "site-packages" dir is meant for actual Python libraries, so I don't
> want to dump modules internal to my app in there.  If I was going to put
> anything in site-packages I'd want to organize it as a regular Python package
> first instead of a bunch of individual modules.

I placed it into a subdirectory "germanium-0.2.0" in the site-packages directory
exactly because it is just germanium internal stuff. That dir is not added to
sys.path without further action, and you can even install germanium-0.2.1 in
parallel without a naming conflict.

(In reply to comment #10)
Use #!@PYTHON@ in germanium/germanium.py and then add -e
's|[@]PYTHON@|$(PYTHON)' to the sed line in germanium/Makefile.am.
Comment 12 Mamoru TASAKA 2008-07-16 13:46:09 EDT
Would you post the full URLs of your newest spec/srpm files so that reviewers
can easily find out them?
Comment 13 Adam Huffman 2008-07-17 09:58:19 EDT
I've been trying to integrate Matt's latest release candidate with Hans' patches
and had some trouble with automake versions (owing to the Makefile.am changes).
 It was looking for automake-1.9, which isn't available on Fedora 9.

Isn't there some way of making it less version-specific?
Comment 14 Hans Ulrich Niedermann 2008-07-17 10:20:17 EDT
What are you changing in Makefile.am?
Comment 16 Hans Ulrich Niedermann 2008-07-17 14:29:03 EDT
Created attachment 312069 [details]
Install germanium libs somewhere in python's sitelibdir

Try this patch instead and leave the Makefile.am alone. You want to avoid
running automake/autoconf/etc. at RPM build time.

I'd still want upstream to apply the patch I suggested to their source, but we
are Fedora, not upstream.
Comment 17 Adam Huffman 2008-07-19 13:05:45 EDT
New version posted at:

http://gilberte.verdurin.com/fedora/germanium/germanium-0.2.1-0.2.rc1.fc9.src.rpm

and

http://gilberte.verdurin.com/fedora/germanium/germanium.spec

incorporating Hans' patch.


rpmlint -i ../RPMS/noarch/germanium-0.2.1-0.2.rc1.fc9.noarch.rpm germanium.spec
../SRPMS/germanium-0.2.1-0.2.rc1.fc9.src.rpm 
2 packages and 1 specfiles checked; 0 errors, 0 warnings.

I have installed it on a Fedora 9 i386 machine and it runs correctly.
Comment 18 Matt Good 2008-07-27 17:52:26 EDT
I've made one more change to use the abs path to the Python interpreter rather
than relying on /usr/bin/env so that the app runs under the Python intepreter it
was installed for rather than what's on the current user's $PATH.

Here is the new 0.2.1 tarball:
http://projects.matt-good.net/releases/germanium/germanium-0.2.1.tar.gz

I've left the Python modules installed under $(datadir).  As a Python developer
I don't expect apps to install Python files under site-packages that are not
importable as a package.  However, it sounds like that's what's recommended for
Fedora, so go ahead and continue patching that in the RPM.
Comment 19 Matt Good 2008-07-27 18:02:31 EDT
Whoops, should've tested that last patch better.  I've removed 0.2.1 since it's
broken and made a 0.2.2:
http://projects.matt-good.net/releases/germanium/germanium-0.2.2.tar.gz
Comment 20 Adam Huffman 2008-08-03 10:54:49 EDT
Matt,

Before I make a package for 0.2.2, I should point out that the emusic format seems to have changed.  The file extension is now .emx, which is an XML format, replacing the previous encrypted one.

Some details are here:

http://code.google.com/p/emusicremote/wiki/EMX_File_Format
Comment 21 Adam Huffman 2008-08-09 11:48:56 EDT
Have submitted another package for review - https://bugzilla.redhat.com/show_bug.cgi?id=458543
Comment 22 Matt Good 2008-08-10 14:15:11 EDT
(In reply to comment #20)
> Before I make a package for 0.2.2, I should point out that the emusic format
> seems to have changed.  The file extension is now .emx, which is an XML format,
> replacing the previous encrypted one.
> 
> Some details are here:
> 
> http://code.google.com/p/emusicremote/wiki/EMX_File_Format

Yes, I've seen the EMX format, though by default eMusic still uses the EMP format since this is what's supported by their official clients.  The new eMusic Remote client that uses EMX is still pre-release with no releases since Oct. 2007, so I haven't bothered updating this client yet, though the eMusic lib I'm planning would support EMX.
Comment 23 Adam Huffman 2008-08-10 14:49:24 EDT
Fair enough.  I only mentioned it because the last download I made it defaulted to .emx.  Just had a look and it's back to .emp - perhaps they're rolling it out gradually.
Comment 25 Mamoru TASAKA 2008-09-07 13:40:40 EDT
Hi;

Some random comments:
- Please consider to use %{?dist} tag:
  https://fedoraproject.org/wiki/Packaging/DistTag

- The URL written as Source0 seems 404

- "BuildRequires: pycairo-devel" is redundant because
  pygtk2-devel Requires pycairo-devel

- Support parallel make if possible, otherwise write as
  a comment that parallel make is intentionally disabled:
  https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make

- Desktop files must be handled by desktop-file-install:
  https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files

- During build GConf schemas installation must be disabled.
  Also some proper scriptlets and Requires should be added - see:
  https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GConf

  Note:
    We does not regard gconf schemas files as config file, so please
    remove %config(noreplace) attribution from gconf file, even if
    rpmlint warns for it.

- XML file is installed under %{_datadir}/mime/packages, so please
  refer to
  https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#mimeinfo

- Please make it sure that all directories created when installing
  this package are correctly owned by this package - ref:
  https://fedoraproject.org/wiki/PackagingDrafts/UnownedDirectories
Comment 28 Mamoru TASAKA 2008-09-10 13:53:44 EDT
For 0.2.2-4:

* Redundant (Build)Requires
  - I mischecked before, however
    * pygtk2-devel Requires pygobject2-devel
    * pygtk2 Requires pygobject2, pycairo (from explicit Requires)
      and gtk2 (from library dependencies)
      ! Note: pygtk2-devel does not require gtk2-devel
    So there are more redundant (Build)Requires.

  - Requires(post/postun): desktop-file-utils is not needed (this
    is not actually used)

* Other Requires
  - germanium/vfs_util.py contains:
----------------------------------------------------------
    19  from cStringIO import StringIO
    20  
    21  import gnomevfs
    22  
----------------------------------------------------------
    germanium/gconf_util.py contains:
----------------------------------------------------------
    21  import gconf
    22  import gtk
----------------------------------------------------------
    so, "Requires: gnome-python2-gnomevfs gnome-python2-gconf"
    seems needed.

* Desktop file
  - Category "Application" is deprecated and should be removed.
Comment 29 Mamoru TASAKA 2008-09-10 13:56:11 EDT
Created attachment 316337 [details]
screenshot of germanium

By the way, when I just try $ germanium, it seems a icon is missing
(see screenshot)
Comment 30 Adam Huffman 2008-09-10 16:50:54 EDT
-5 now available at http://verdurin.fedorapeople.org/review/germanium/

I see the same missing icon - have reported that to the upstream author.
Comment 31 Mamoru TASAKA 2008-09-11 12:40:07 EDT
Okay, then:

-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------
Comment 32 Mamoru TASAKA 2008-09-19 01:54:32 EDT
ping?
Comment 33 Adam Huffman 2008-09-19 01:59:55 EDT
I am working on another package, which I'll probably submit over the weekend.  Also have a couple of others I plan to work on.

Will try to do some pre-reviews this weekend, too.
Comment 34 Mamoru TASAKA 2008-09-19 02:18:05 EDT
Okay, thank you for replying. If you are ready please let me know on
this bug.
Comment 35 Adam Huffman 2008-09-20 19:52:38 EDT
Added new review request for Pyroman:

https://bugzilla.redhat.com/show_bug.cgi?id=463035

I still need some guidance as to how the package should be integrated more closely into Fedora i.e. should it run as a service to load the iptables rules, etc.
Comment 36 Mamoru TASAKA 2008-09-21 02:56:28 EDT
(In reply to comment #35)
> Added new review request for Pyroman:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=463035
> 
> I still need some guidance as to how the package should be integrated more
> closely into Fedora i.e. should it run as a service to load the iptables rules,
> etc.

I guess you can check and bollow some ideas from debian patch:
http://ftp.debian.org/debian/pool/main/p/pyroman/pyroman_0.4.6-1.diff.gz
This contains debian/init. I guess with some modification this
script can be used for Fedora.
Comment 37 Mamoru TASAKA 2008-09-21 02:59:27 EDT
s|bollow|borrow|'
Comment 38 Adam Huffman 2008-09-21 19:36:53 EDT
New review request for gnoMint:

https://bugzilla.redhat.com/show_bug.cgi?id=463123

It will only build on F9 and greater owing to the requirement for gnutls-2.
Comment 39 Mamoru TASAKA 2008-09-23 11:28:17 EDT
Well,

* This package itself is okay
* Your another review request (bug 463123) needs some fixing, however good to some
  extent for review request process.

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

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Install the Client Tools (Koji) ".
Now I am sponsoring 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.

Removing NEEDSPONSOR.
Comment 40 Mamoru TASAKA 2008-10-02 11:22:56 EDT
ping?
Comment 41 Adam Huffman 2008-10-02 11:35:39 EDT
I haven't proceeded further with this package as eMusic has changed the download format, meaning that as it stands germanium won't be able to download music from the site.  I've corresponded with the upstream author about this and am working on changing the file parsing to deal with the new .emx format.

Once that's done I'll upload a new version and work on importing the package.
Comment 42 Mamoru TASAKA 2008-10-08 09:35:47 EDT
Okay, once reverting to fedora-review?. If you are ready please
let me again.
Comment 43 Mamoru TASAKA 2008-10-16 11:12:16 EDT
If you heard any news from upstream, please let me know it.
Comment 44 Matt Good 2008-10-16 21:04:35 EDT
Well, I told Adam the changes that I think would need made for the format change (mostly just removing the current decryption), so I'm waiting on a patch.  I'm not actively working on this anymore, but I'm glad to accept patches.
Comment 45 Adam Huffman 2008-10-19 18:51:40 EDT
Matt, thanks for responding.  Just haven't had the time to fix this properly yet.  I'll upload a new package and send the patch to you when it's done.
Comment 46 Adam Huffman 2008-11-09 15:06:29 EST
Just as an update on this, the XML format has changed (in addition to losing the encryption), so I'm working on that.
Comment 47 Mamoru TASAKA 2008-12-07 10:50:50 EST
Any news here?
Comment 48 Mamoru TASAKA 2008-12-22 11:01:24 EST
ping again?
Comment 49 Adam Huffman 2008-12-23 04:48:48 EST
Nearly there on the new XML format - should have time to finish it over the holidays.
Comment 50 Mamoru TASAKA 2009-02-06 10:19:40 EST
Any good news here?
Comment 51 Adam Huffman 2009-02-09 12:46:13 EST
There are actually other packages that have a higher priority for me now.  In particular, I don't really want effectively to maintain the upstream project myself, now that Matt has moved on to other things.

Maybe the request should just be closed
Comment 52 Mamoru TASAKA 2009-02-10 07:51:59 EST
Thank you for reply.
So, when you have some time to work on this software again,
please feel free to submit a new review request and mark this bug
a duplicate of the new one. Anyway I appreciate your work.

Once closing now.

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