Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to libcdio-cdparanoia (from cdparanoia) #213

Merged
merged 4 commits into from Jan 26, 2018

Conversation

MerlijnWajer
Copy link
Collaborator

Next commits will add a choice to fall back to the old one, or pick a
different name. But let's get this ready for testing now.

Fixes issue #87. In the coming days I will add support to choose alternative implementations.

@MerlijnWajer
Copy link
Collaborator Author

Please give this a test run with the Julie Roberts CD, and some normal CDs too.

Everything should work, but I'm really tired and I promised I'd do this tonight.

@MerlijnWajer
Copy link
Collaborator Author

We might want to check for a certain cd-paranoia version and suggest the user upgrades to a version that has the fix in the near future.

Next commits will add a choice to fall back to the old one, or pick a
different name. But let's get this ready for testing now.
@peaveyman
Copy link

peaveyman commented Jan 22, 2018

This PR worked. Whipper successfully ripped the Julie Roberts CD with no errors and the problematic track 5 and produced the same CRC as EAC.

Whipper log: https://pastebin.ca/3962401

EAC log: https://pastebin.ca/3962402

Here are the logs for George Strait - Ocean Front Property

Whipper: https://pastebin.ca/3962407

EAC: https://pastebin.ca/3962408

Here are the logs for The Charlie Daniels Band - A Decade of Hits

Whipper: https://pastebin.ca/3962411

EAC: https://pastebin.ca/3962412

Here are the logs for Damn Yankees

Whipper: https://pastebin.ca/3962424

EAC: https://pastebin.ca/3962427

@MerlijnWajer
Copy link
Collaborator Author

I'll fix up the build failure when I get home.

@JoeLametta
Copy link
Collaborator

We might want to check for a certain cd-paranoia version and suggest the user upgrades to a version that has the fix in the near future.

I think the relevant versions we can choose to target are:

(OR)

@JoeLametta
Copy link
Collaborator

JoeLametta commented Jan 22, 2018

BTW, here's a recap of the libcdio-paranoia's releases as packaged by the "major" distributions:

Distribution cd-paranoia's version number Notes
Ubuntu 0.83 Will have at least 0.94+2 in Ubuntu 18.04 LTS 'Bionic Beaver'
Debian 0.83 Testing, Sid have both 0.94+2
CentOS/RHEL 7 0.90+1 I'm not sure the version number I've reported is correct
Fedora 22+ 0.93+1 0.94+2 in Rawhide. It will probably land in Fedora 28
openSUSE 0.93+1 I'm not sure the version number I've reported is correct
Arch Linux 0.94+2
Gentoo Linux 0.93+1 Version number as 'stable'. 0.94+2 as 'testing'

Updated Fedora row.

@Freso
Copy link
Member

Freso commented Jan 25, 2018

My vote is to go for 0.94+2. If distros haven't yet updated libcdio-paranoia, they likely won't get any future whipper releases into their repositories anytime soon either, and whipper may be the reason the update their libcdio-paranoia packages.

@JoeLametta
Copy link
Collaborator

I'll fix up the build failure when I get home.

I think you just need to add the libcdio-utils dependency to .travis.yml (in the install section).

My vote is to go for 0.94+2.

👍

Easier for us, better user experience.

@MerlijnWajer
Copy link
Collaborator Author

No, there is a bit more to be done, but it's not a lot. I'll do it in a couple of hours and then, let's merge it.

@mruszczyk
Copy link
Contributor

mruszczyk commented Jan 25, 2018

Fedora libcdio maintainer just updated libcdio-paranoia to 0.94+2 in rawhide. Should go to stable fedora in May with version 28.

@MerlijnWajer
Copy link
Collaborator Author

Do we want to hard fail if we are not on 0.94+2?

@MerlijnWajer
Copy link
Collaborator Author

Joe, I think this is ready for merging. Whatever come later can be added later (e.g. picking a different cdparanoia implementation again, or a different cdparanoia binary name, or disabling/enabling specific version checks, etc)

@JoeLametta JoeLametta merged commit 5dbb197 into whipper-team:master Jan 26, 2018
@JoeLametta
Copy link
Collaborator

Merged, thanks.
Will update the README to state the added dependency right now.

@JoeLametta
Copy link
Collaborator

Do we want to hard fail if we are not on 0.94+2?

I haven't decided yet: what's your opinion?
Maybe we could just issue a warning if running using a libcdio-paranoia version older than 0.94+2 telling the user that is an unsupported configuration. If using an older version causes new specific bugs to appear, they will not be addressed.

At the end the hard way is probably better because it gives you a slight guarantee increase regarding whipper's expected behaviour.

@thomas-mc-work
Copy link
Contributor

Where have you got that working version of libcdio-utils from? Both Debain and Ubuntu official packages are broken.

@JoeLametta
Copy link
Collaborator

@thomas-mc-work There's a working cd-paranoia package from the deb-multimedia repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants