Bug 322061

Summary: Review Request: asunder - A graphical Audio CD ripper and encoder
Product: [Fedora] Fedora Reporter: Marcin Zajaczkowski <mszpak>
Component: Package ReviewAssignee: Michael Schwendt <bugs.michael>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: asmith16, fedora-package-review, notting
Target Milestone: ---Flags: bugs.michael: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-11-12 22:20:15 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Marcin Zajaczkowski 2007-10-07 12:43:35 UTC
Spec URL: http://timeoff.wsisiz.edu.pl/rpms/asunder/asunder.spec
SRPM URL: http://timeoff.wsisiz.edu.pl/rpms/asunder/asunder-0.9-1.src.rpm
Description: Allows to save tracks from an Audio CD as WAV, MP3, OGG, and/or FLAC.

Comment 1 Marcin Zajaczkowski 2007-10-07 13:45:35 UTC
And it's a desktop environment neutral.

Comment 2 Michael Schwendt 2007-11-08 20:52:30 UTC
> rpmlint -i asunder-0.9-1.src.rpm 
> asunder.src: W: non-coherent-filename asunder-0.9-1.src.rpm
> The file which contains the package should be named
> <NAME>-<VERSION>-<RELEASE>.<ARCH>.rpm.

Unimportant, but: rpm -qp --qf %{r}\\n asunder-0.9-1.src.rpm 
1.fc5


> %configure --prefix=%{_prefix}

That arg is the default.


> Requires:       libcddb >= 0.9.5
> BuildRequires:  libcddb-devel >= 0.9.5

Questionable.

Why? First of all, libcddb < 0.9.6 is more than three years old.
There have been several [much] newer versions since then. Secondly,
when building against 1.3.0 in Fedora 7, for instance, it is unsafe
to assume that >= 0.9.5 would be sufficient. That old version is
not ABI compatible.

Suggestion: Keep the version in the BuildRequires if you insist on
doing that (although Fedora's libcddb is latest). Drop the version
in the Requires and rely on rpmbuild's SONAME dependencies.


> #is already installed by desktop-file-install
> rm %{buildroot}%{_datadir}/applications/asunder.desktop

You can tell desktop-file-install to  --delete-original  and
give this file as input.

[...]

At run-time, I cannot get it to rip anything, not even as WAV.
It opens an error dialog:

  There was an error creating 15 files

or (when I select only one file):

  There was an error creating 1 files

$ asunder
Unable to delete WAV file "/home/qa/Desktop/Various Artists - Back In Time
II/Various Artists - Martin Galway/Galway Is God 2000 (CD mix by Jogeir
Liljedahl).wav": No such file or directory

It has only created an empty directory. The most I can get out of
it is an .m3u file if I enable that option.


Comment 3 Andrew Smith 2007-11-09 10:18:01 UTC
Michael, can you post here or email me details about how to reproduce this bug?
If it's a pretty standard Fedora install, what version?

What format do you have set for the album directory and the music file (in the
preferences)? Do you have 'single artist' checked?

For that particular CD, could you tell me what the album artist, album title,
and track name are (as displayed by Asunder)? I'm a bit confused why there seems
to be two album names ('Various Artists - Back In Time II' and 'Various Artists
- Martin Galway').

I will fix this as soon as I figure out what's wrong, but I suspect it's an
unusual problem (this is the only report I got) so I wouldn't block it from..
whatever you were doing Marcin :)


Comment 4 Marcin Zajaczkowski 2007-11-09 20:56:44 UTC
(In reply to comment #2)
> Unimportant, but: rpm -qp --qf %{r}\\n asunder-0.9-1.src.rpm 
> 1.fc5

Probably I have too old rpmlint (0.79), because I haven't got that message.
I know about .fc5 (I have to remove SRPM name everytime). The only solution
would be remove %dist macro from .rpmmacros when building srpm for upload and
later turn it on to build binary RPM with valid %dist (binary RPMs are also
available on my website).

> > %configure --prefix=%{_prefix}
> 
> That arg is the default.

Yup, fixed.

> > Requires:       libcddb >= 0.9.5
> > BuildRequires:  libcddb-devel >= 0.9.5
> 
> Questionable.

I took it from the program website. I didn't test it with an older versions. In
FC5 there is 1.2.2.
Andrew, what as the author you think about backward compatibility?

> You can tell desktop-file-install to  --delete-original  and
> give this file as input.

Useful option.

> At run-time, I cannot get it to rip anything, not even as WAV.

I was able to rip CD on my FC5. I will check also on F7. Hopefully you resolve
that problem with Andrew.

Comment 5 Andrew Smith 2007-11-09 21:18:27 UTC
> Andrew, what as the author you think about backward compatibility?
I feel strongly that backward-compatibility must exist unless there's a really
good reason otherwise.

But I don't think that applies to binary packages. If a package is made for a
distro that has a certain version of libcddb, I don't see why the binary can't
depend on that version or newer. In other words - I don't think it's a problem
to change it to 1.2.2

One day 'll learn something about binary compatibility, but not today :)


Comment 6 Michael Schwendt 2007-11-09 22:10:19 UTC
This application doesn't replace slashes in the character
strings it creates file names from:

Album artist: Various Artists
Album title: Back in Time II
Track 2 Title: Martin Galway/Galway Is God 2000 (CD mix by Jogeir Liljedahl)

Notice the '/' in the track title, which is seen as a directory
delimiter by the file-system. Unless there is a document which
defines that a slash must not be used in the track titles (and in
that case the CD would be wrong), not substituting those slashes
is an application bug.

[...]

I've given a different Audio CD a try, and there the application works,
so:

APPROVED (with the things from comment 2 fixed)


Comment 7 Marcin Zajaczkowski 2007-11-10 11:37:50 UTC
Ok, I removed a version from Requires. Your example with build on F7 is very
reasonable. I left version for BuildRequires, but there is already EOL for
systems with so old version of libcddb bundled, so it's probably unnecessary.

Changelog summarize:

* Fri Nov 09 2007 Marcin Zajaczkowski <mszpak ATT wp DOTT pl> - 0.9-2
- applied Michael Schwendt suggestions (all below)
- removed redundant --prefix argument
- used --delete-original to delete .desktop file
- removed minimal required version of libcddb from Requires

Current version:
http://timeoff.wsisiz.edu.pl/rpms/asunder/asunder.spec
http://timeoff.wsisiz.edu.pl/rpms/asunder/asunder-0.9-2.src.rpm


Comment 8 Marcin Zajaczkowski 2007-11-10 11:40:07 UTC
New Package CVS Request
=======================
Package Name: asunder
Short Description: A graphical Audio CD ripper and encoder
Owners: szpak
Branches: F-7 F-8
InitialCC: 
Cvsextras Commits: yes


Comment 9 Kevin Fenzi 2007-11-10 20:29:38 UTC
cvs done.

Comment 10 Marcin Zajaczkowski 2007-11-12 22:20:15 UTC
Commited, built and pushed for testing.

Thanks for your help, guys.