Bug 473184

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

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

(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

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

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

(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

> %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

(In reply to comment #7)
> https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#desktop-database
> https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#mimeinfo

Thanks.  Dependency on shared-mime-info and desktop-file-utils removed.

Spec URL: http://blackbean.org/review/clamz.spec
SRPM URL: http://blackbean.org/review/clamz-0.2-6.fc10.src.rpm

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.

(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

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.

My recommendation exactly would be to set:

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

in /usr/bin/clamz

(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

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.

(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

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

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

Are you still interested in packaging this?  I would be willing to review it if you are.

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.

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

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

This looks way prettier.  :)  I'll go ahead and do the formal review over the next few days.  Thanks for the quick update.

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.

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.

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

I compiled this srpm for Fedora 13, x86_64.  Works perfectly with firefox integration, and default Music folder, Artist-Album.  Well done!

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

Thanks, I didn't see that.  Ok, this package is approved as-is.

New Package CVS Request
=======================
Package Name: clamz
Short Description: Amazon Downloader
Owners: radford
Branches: F-13 EL-6
InitialCC:

CVS done (by process-cvs-requests.py).

clamz-0.4-3.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/clamz-0.4-3.fc13

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

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.