Bug 473184 - Review Request: clamz - Amazon Downloader
Review Request: clamz - Amazon Downloader
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jonathan Dieter
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-26 18:29 EST by Jim Radford
Modified: 2010-07-13 03:38 EDT (History)
9 users (show)

See Also:
Fixed In Version: clamz-0.4-3.fc13
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-07-13 03:38:26 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
jdieter: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jim Radford 2008-11-26 18:29:53 EST
Spec URL: http://blackbean.org/review/clamz-1.spec
SRPM URL: http://blackbean.org/review/clamz-0.2-1.fc10.src.rpm
Description:
Clamz is a little command-line program to download MP3 files from
Amazon.com's music store.  It is intended to serve as a substitute
for Amazon's official MP3 Downloader, which is not free software (and
therefore is only available in binary form for a limited set of
platforms.)  Clamz can be used to download either individual songs or
complete albums that you have purchased from Amazon.
Comment 1 Fabian Affolter 2008-12-08 05:43:59 EST
Just some comments on your spec file

- Spec file name should be clamz.spec

- License should be GPLv3+.  The header in the source says 'or (at your option) any later version.'

- 'BuildRequires: desktop-file-utils' is missing and you need to install the .desktop file
  https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files

- Add README to %doc

- Remove '.fc10' from your changelog entry
  https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs
Comment 2 Fabian Affolter 2008-12-08 05:46:22 EST
one more thing

- Don't mix $RPM_BUILD_ROOT and %{buildroot}
  https://fedoraproject.org/wiki/Packaging/Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS
Comment 3 Jim Radford 2008-12-08 12:25:53 EST
(In reply to comment #1)
> Just some comments on your spec file

Thanks.

> - Spec file name should be clamz.spec

Why?  Then I can't keep all the previous links valid.  FWIW, I've done this before without complaint.
 
> - License should be GPLv3+.  The header in the source says 'or (at your option)
> any later version.'

Done.

> - 'BuildRequires: desktop-file-utils' is missing and you need to install the
> .desktop file

Done.
 
> - Add README to %doc

Hmm, already there; I'm going to guess you meant COPYING.

> - Remove '.fc10' from your changelog entry

Done.

> - Don't mix $RPM_BUILD_ROOT and %{buildroot}

Done.

Spec URL: http://blackbean.org/review/clamz.spec
SRPM URL: http://blackbean.org/review/clamz-0.2-4.fc10.src.rpm
Comment 4 Fabian Affolter 2008-12-08 18:49:46 EST
(In reply to comment #3)
> (In reply to comment #1)
> > - Spec file name should be clamz.spec
> 
> Why?  Then I can't keep all the previous links valid.  FWIW, I've done this
> before without complaint.

https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Spec_file_name

You haven't to keep the links valid, just add a proper entry in the %changelog section.  The number in the 'Release:' tag is 4 but there is only one entry in the changelog.

https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs
 
> > - Add README to %doc
> 
> Hmm, already there; I'm going to guess you meant COPYING.

I can't remember, but it's possible that I took the wrong file.

About the .desktop file.  The guidelines says that it should be include as 'SourceX:' https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files

I think with the .xml file the procedure will be similar.  Send these files to upstream and ask them to include those files in a future release of clamz.
Comment 5 Jim Radford 2008-12-09 19:46:26 EST
I separated the desktop and mime xml files off into their own files.  I'm not sure why, it just makes them harder to review.

Spec URL: http://blackbean.org/review/clamz.spec
SRPM URL: http://blackbean.org/review/clamz-0.2-5.fc10.src.rpm
Comment 6 Fabian Affolter 2008-12-23 05:24:23 EST
(In reply to comment #5)
> I separated the desktop and mime xml files off into their own files.  I'm not
> sure why, it just makes them harder to review.

Because all changes should go upstream...

http://code.google.com/p/clamz/issues/detail?id=5
http://code.google.com/p/clamz/issues/detail?id=6
Comment 7 Michael Schwendt 2009-01-13 14:57:28 EST
> %post
> update-mime-database %{_datadir}/mime
> update-desktop-database
> 
> %postun
> update-mime-database %{_datadir}/mime
> update-desktop-database

* Be aware of:

https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#desktop-database
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#mimeinfo
Comment 9 Gratien D'haese 2009-01-23 09:20:49 EST
spec files contaims:
BuildRequires:  libcurl-devel, libgcrypt-devel, expat-devel

=> Required libgpg-error.so.0 library is part of libgpg-error-devel package (missing)

A mock build tells us:
desktop-file-install: command not found

==> missing in BuildRequires

When I have a look at the libraries needed by binary clamz
$ ldd clamz
	linux-gate.so.1 =>  (0x00110000)
	libgcrypt.so.11 => /lib/libgcrypt.so.11 (0x030e8000)
	libgpg-error.so.0 => /lib/libgpg-error.so.0 (0x02888000)
	libcurl.so.4 => /usr/lib/libcurl.so.4 (0x00bc5000)
	libexpat.so.1 => /lib/libexpat.so.1 (0x005e9000)
	libc.so.6 => /lib/libc.so.6 (0x0017a000)
	libidn.so.11 => /lib/libidn.so.11 (0x02550000)
	libssh2.so.1 => /usr/lib/libssh2.so.1 (0x00b75000)
	libldap-2.4.so.2 => /usr/lib/libldap-2.4.so.2 (0x00b31000)
	librt.so.1 => /lib/librt.so.1 (0x0044a000)
	libgssapi_krb5.so.2 => /usr/lib/libgssapi_krb5.so.2 (0x031f1000)
	libkrb5.so.3 => /usr/lib/libkrb5.so.3 (0x04415000)
	libk5crypto.so.3 => /usr/lib/libk5crypto.so.3 (0x0279f000)
	libcom_err.so.2 => /lib/libcom_err.so.2 (0x00596000)
	libz.so.1 => /lib/libz.so.1 (0x00350000)
	libssl3.so => /lib/libssl3.so (0x02fd8000)
	libsmime3.so => /lib/libsmime3.so (0x0300b000)
	libnss3.so => /lib/libnss3.so (0x04527000)
	libplds4.so => /lib/libplds4.so (0x00d6e000)
	libplc4.so => /lib/libplc4.so (0x00d74000)
	libnspr4.so => /lib/libnspr4.so (0x02f6e000)
	libpthread.so.0 => /lib/libpthread.so.0 (0x00317000)
	libdl.so.2 => /lib/libdl.so.2 (0x00310000)
	/lib/ld-linux.so.2 (0x0015a000)
	libssl.so.7 => /lib/libssl.so.7 (0x00ae4000)
	libcrypto.so.7 => /lib/libcrypto.so.7 (0x047a4000)
	liblber-2.4.so.2 => /usr/lib/liblber-2.4.so.2 (0x00d3e000)
	libresolv.so.2 => /lib/libresolv.so.2 (0x00111000)
	libsasl2.so.2 => /usr/lib/libsasl2.so.2 (0x02d3c000)
	libkrb5support.so.0 => /usr/lib/libkrb5support.so.0 (0x0014d000)
	libkeyutils.so.1 => /lib/libkeyutils.so.1 (0x00148000)
	libnssutil3.so => /lib/libnssutil3.so (0x02faa000)
	libcrypt.so.1 => /lib/libcrypt.so.1 (0x0284f000)
	libselinux.so.1 => /lib/libselinux.so.1 (0x00332000)

=> suspect that Requires: field is needed too.
Comment 10 Jim Radford 2009-04-18 19:51:45 EDT
(In reply to comment #9)
 => Required libgpg-error.so.0 library is part of libgpg-error-devel package
> (missing)

Interesting, I expect the library dependencies to found automatically by rpm.

For example, I get:

  $rpm -qp --requires clamz*.rpm | grep error
  libgpg-error.so.0()(64bit)  

> A mock build tells us:
> desktop-file-install: command not found

Fixed and now builds in mock (fc10).

  Spec URL: http://blackbean.org/review/clamz.spec
  SRPM URL: http://blackbean.org/review/clamz-0.2-7.fc10.src.rpm
Comment 11 Scott Williams 2009-07-17 23:47:47 EDT
For what it's worth, i've compiled this for Fedora 11 and it's working just grand on my machine:

http://vwbusguy.fedorapeople.org/clamz/f11/

Integration with Firefox also seems to work.  My _suggestion_, which is not a requirement here, is that the default output directory be set to $HOME/Music and default fodler output as <artist>/<Album>/<songs> in /usr/bin/clamz to keep a consistent desktop experience and to match the functionality of Amazon's mp3 downloader for which this is a replacement for.
Comment 12 Scott Williams 2009-07-17 23:52:11 EDT
My recommendation exactly would be to set:

OutputDir       "$HOME/Music/${album-artist}/${album}/"

in /usr/bin/clamz
Comment 13 Jim Radford 2009-07-18 02:52:11 EDT
(In reply to comment #12)
> My recommendation exactly would be to set:
> 
> OutputDir       "$HOME/Music/${album-artist}/${album}/"
> 
> in /usr/bin/clamz  

While I don't this this should be true in general for the command line app, I do think it should be true when invoked by the desktop file.  I added a command line option --sane-defaults and used it in the .desktop file.

This should do what you are asking for while leaving the normal command line to default to the current directory while still allowing the config file to override things.

I also print out the download directory now so that you can tell where it is downloading to when invoked by say a browser without context.

  Spec URL: http://blackbean.org/review/clamz.spec
  SRPM URL: http://blackbean.org/review/clamz-0.2-9.fc11.src.rpm
Comment 14 Till Maas 2009-09-16 16:32:32 EDT
In contradiction to comment:8 and comment:7, there are these Requires in the spec:

Requires(post):   /usr/bin/update-mime-database, /usr/bin/update-desktop-database
Requires(postun): /usr/bin/update-mime-database, /usr/bin/update-desktop-database


Btw. where in the guidelines are inline .desktop and .xml files allowed? I agree with the others that this makes upstreaming unecessaryly harder.


The patches do not have any upstream status attached:
https://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

The changelog header for 0.2-9 is broken, it contains the Name twice.
Comment 15 Jim Radford 2009-09-16 17:50:20 EDT
(In reply to comment #14)
> In contradiction to comment:8 and comment:7, there are these Requires in the
> spec: Requires(post):   /usr/bin/update-mime-database,

Fixed.

> Btw. where in the guidelines are inline .desktop and .xml files allowed?

I don't recall, but the policy was changed recently (I recall it being mentioned in a FWN), and so as I prefer them in the spec, I moved them back.

> The patches do not have any upstream status attached:

I've sent all the patches upstream.  Some have been acked, some have had further discussion, but I have yet to see an update.

> The changelog header for 0.2-9 is broken, it contains the Name twice.  

Damn rpm-spec-mode!  Fixed.

  Spec URL: http://blackbean.org/review/clamz.spec
  SRPM URL: http://blackbean.org/review/clamz-0.2-10.fc11.src.rpm
Comment 16 Andy Campbell 2009-11-22 09:20:36 EST
Has the patch been applied that allows absolute paths ?

http://code.google.com/p/clamz/issues/detail?id=16#c0

Couldn't see any patches in the spec - but then again I've never really done any packaging ?

Thanks
Andy
Comment 17 Jim Radford 2009-11-22 19:32:01 EST
(In reply to comment #16)
> Has the patch been applied that allows absolute paths ?

Yes I had already run into this problem, wrote a patch, and sent it to the author.

> Couldn't see any patches in the spec - but then again I've never really done
> any packaging ?

Here's the line in the spec:

  Patch2:         clamz-allow-creating-absolute-paths.patch

The patches themselves can be found in the source RPM above.
Comment 18 Jonathan Dieter 2010-05-18 10:40:44 EDT
Are you still interested in packaging this?  I would be willing to review it if you are.
Comment 19 Jim Radford 2010-05-18 12:44:41 EDT
Yes.  I just used it two days ago.  Worked liked like a charm.  Makes me happy every time.

I wish it were GTK (to the point of starting to code a replacement), but for now text will have to do.
Comment 20 Jonathan Dieter 2010-05-18 13:03:34 EDT
I've noticed that 0.4 is available.  Do you mind updating the package.  I'm hoping that it will fix a couple of the issues mentioned above (seeing as you are thanked for your patches in the README).
Comment 21 Jim Radford 2010-05-18 13:28:01 EDT
Upstream released a new version that includes my 4 patches and my desktop and mime-info files.

Spec URL: http://blackbean.org/review/clamz.spec
SRPM URL: http://blackbean.org/review/clamz-0.4-1.fc12.src.rpm
Comment 22 Jonathan Dieter 2010-05-18 13:39:18 EDT
This looks way prettier.  :)  I'll go ahead and do the formal review over the next few days.  Thanks for the quick update.
Comment 23 Jonathan Dieter 2010-05-21 12:56:38 EDT
Legend:
X   = Good
-   = Not so good
N/A = Not applicable

[ X ] MUST: rpmlint must be run on every package
      clamz.src: W: spelling-error %description -l en_US
          com's -> con's, om's, come's
      1 packages and 0 specfiles checked; 0 errors, 1 warnings.
      clamz.x86_64: W: spelling-error %description -l en_US
          com's -> con's, om's, come's
      1 packages and 0 specfiles checked; 0 errors, 1 warnings.

      rpmlint doesn't like the com in Amazon.com.  Maybe they'll change
      their name to make rpmlink happy.  Or not.  :)

[ X ] MUST: The package must be named according to the Package Naming 
      Guidelines
[ X ] MUST: The spec file name must match the base package %{name} [...]
[   ] MUST: The package must meet the Packaging Guidelines
[ X ] MUST: The package must be licensed with a Fedora approved license
      and meet the Licensing Guidelines
[ X ] MUST: The License field in the package spec file must match the 
      actual license
[ X ] MUST: If (and only if) the source package includes the text of the 
      license(s) in its own file, then that file, containing the text of 
      the license(s) for the package must be included in %doc
[ X ] MUST: The spec file must be written in American English.
[ X ] MUST: The spec file for the package MUST be legible.
[ X ] MUST: The sources used to build the package must match the upstream 
      source, as provided in the spec URL.

      (sha256sum)
      ecc09d09ad329f2e24c9381983a56de3c944e3a933183ca342d25d5917f34968
      clamz-0.4.tar.gz

[ X ] MUST: The package MUST successfully compile and build into binary 
      rpms on at least one primary architecture (x86_64)
[N/A] MUST: If the package does not successfully compile, build or work on 
      an architecture, then those architectures should be listed in the 
      spec in ExcludeArch.
[ X ] MUST: All build dependencies must be listed in BuildRequires
[N/A] MUST: The spec file MUST handle locales properly.
[N/A] MUST: Every binary RPM package (or subpackage) which stores shared 
      library files (not just symlinks) in any of the dynamic linker's 
      default paths, must call ldconfig in %post and %postun.
[N/A] MUST: If the package is designed to be relocatable, the packager must 
      state this fact in the request for review.
[ - ] MUST: A package must own all directories that it creates. If it does 
      not create a directory that it uses, then it should require a package 
      which does create that directory.

      $ rpm -qf /usr/share/mime/packages
      shared-mime-info-0.71-1.fc13.x86_64

      It seems that according to the Guidelines, we need to add Require:
      shared-mime-info

[ X ] MUST: A package must not contain any duplicate files in the %files 
      listing.
[ X ] MUST: Permissions on files must be set properly.
[ X ] MUST: Each package must have a %clean section, which contains rm -rf
      %{buildroot}
[ X ] MUST: Each package must consistently use macros.
[ X ] MUST: The package must contain code, or permissable content.
[N/A] MUST: Large documentation files must go in a -doc subpackage.
[ X ] MUST: If a package includes something as %doc, it must not affect the 
      runtime of the application.
[N/A] MUST: Header files must be in a -devel package.
[N/A] MUST: Static libraries must be in a -static package.
[N/A] MUST: Packages containing pkgconfig(.pc) files must 'Requires: 
      pkgconfig'
[N/A] MUST: If a package contains library files with a suffix (e.g. 
      libfoo.so.1.1), then library files that end in .so (without suffix) 
      must go in a -devel package.
[N/A] MUST: In the vast majority of cases, devel packages must require the 
      base package using a fully versioned dependency: Requires: %{name} =
      %{version}-%{release}
[N/A] MUST: Packages must NOT contain any .la libtool archives, these must 
      be removed in the spec if they are built.
[ X ] MUST: Packages containing GUI applications must include a
      %{name}.desktop file, and that file must be properly installed with 
      desktop-file-install in the %install section.
[ X ] MUST: Packages must not own files or directories already owned by 
      other packages.
[ X ] MUST: At the beginning of %install, each package MUST run rm -rf
      %{buildroot}
[ X ] MUST: All filenames in rpm packages must be valid UTF-8.
Comment 24 Jonathan Dieter 2010-05-21 12:58:19 EDT
Seeing as the only thing that needs to be done is adding a Requires: shared-mime-info, I'm approving this contingent on that being done.
Comment 25 Jim Radford 2010-05-21 17:14:07 EDT
Great, thanks.  shared-mime-info is also required for %post and %postun.

Spec URL: http://blackbean.org/review/clamz.spec
SRPM URL: http://blackbean.org/review/clamz-0.4-2.fc12.src.rpm
Comment 26 Scott Williams 2010-05-23 00:52:41 EDT
I compiled this srpm for Fedora 13, x86_64.  Works perfectly with firefox integration, and default Music folder, Artist-Album.  Well done!
Comment 27 Jim Radford 2010-05-23 01:20:21 EDT
Great to hear.  Thanks for testing Scott!

Jonathan, it seems I shouldn't include the dependency on shared-mime-info as per the link above.  The reasoning is that if you don't have shared-mime-info installed then you don't care about immediate updating of the mime database and when you do install it it will be updated appropriately then.

  https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#mimeinfo

Given that this rule is more specific than the "owned" directory rule, I'm going to remove (again) the dependency.

Spec URL: http://blackbean.org/review/clamz.spec
SRPM URL: http://blackbean.org/review/clamz-0.4-3.fc12.src.rpm
Comment 28 Jonathan Dieter 2010-05-23 02:21:43 EDT
Thanks, I didn't see that.  Ok, this package is approved as-is.
Comment 29 Jim Radford 2010-07-03 17:30:15 EDT
New Package CVS Request
=======================
Package Name: clamz
Short Description: Amazon Downloader
Owners: radford
Branches: F-13 EL-6
InitialCC:
Comment 30 Kevin Fenzi 2010-07-04 21:40:09 EDT
CVS done (by process-cvs-requests.py).
Comment 31 Fedora Update System 2010-07-06 12:11:00 EDT
clamz-0.4-3.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/clamz-0.4-3.fc13
Comment 32 Fedora Update System 2010-07-07 13:51:31 EDT
clamz-0.4-3.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update clamz'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/clamz-0.4-3.fc13
Comment 33 Fedora Update System 2010-07-13 03:38:17 EDT
clamz-0.4-3.fc13 has been pushed to the Fedora 13 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.