Failure to rip CD: "ValueError: could not convert string to float: "

[Freso]

Trying to rip Hamza El Din - Escalay (The Water Wheel): Oud Music, I get this traceback:

Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/whipper/extern/task/task.py", line 521, in c
    callable_task(*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/whipper/program/cdrdao.py", line 131, in _read
    float(self._parser.tracks))
ValueError: could not convert string to float: 
CRITICAL:whipper.command.main:exception ValueError at /usr/lib/python2.7/site-packages/whipper/program/cdrdao.py:131: _read(): could not convert string to float: 
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/whipper/extern/task/task.py", line 521, in c
    callable_task(*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/whipper/program/cdrdao.py", line 131, in _read
    float(self._parser.tracks))
ValueError: could not convert string to float: 

It's apparently trying to convert an empty string to a float. I didn't figure out why it was getting an empty string at a quick glance.

CD TOC from cdrdao read-toc --device '/dev/sr0' ~/tmp/23016bff-10d1-404c-aa90-f50d36c83390.toc: 23016bff-10d1-404c-aa90-f50d36c83390.toc.gz
whipper debug log output (with a couple of extra logger.debug statements thrown in to try and figure out what was going on, so line numbers may not line up perfectly…): whipper.log.gz

[Freso]

This was introduced by #345

[Freso]

What I've found is that it includes some blank lines that it's trying to parse, which is the direct cause for the error (ie., the result of calling float("")). Debugging it today, it also seems like self._parser.tracks never changes from 0, which might be a symptom of the underlying issue.

I tried a if not line: continue in cdrdao.ReadTOCTask._read(), but that didn't seem to fix it either, so the parsing of blank lines does not seem to be the core of the issue, I think?

I tried to set up a test case, mocking around some of the data functions, but there are too many moving parts that are based on state and inheritance, so I can't really figure out a good angle for making a test for this. Ideally, I'd be able to just call cdrdao.ReadTOCTask._read() and feed it the .toc file and then it should fail (with the current code). (This is something that absolutely should have test accompanying its fix to ensure it doesn't regress again.)

Maybe @jtl999 can help here, having written the code?

[JoeLametta]

Maybe @jtl999 can help here, having written the code?

@jtl999 If that's the case feel free to self assign this issue.

[srussel]

I ran into the same problem. I fixed it by changing _LAST_TRACK_RE in cdrdao.py to

_LAST_TRACK_RE = re.compile(r"^\s*(?P<track>[0-9]+)")

It seems there is a space before the track number in the cdrdao output. I also assume the [0-9]* should be [0-9]+ otherwise float() could also fail with 0 digits matched.

[jtl999]

I'm currently busy but I'll see if I can give this a test in a few days.

[Freso]

@srussel Thanks!

I'd also really like if this doesn't make it in without a test to make sure it doesn't regress again in the future.

[JoeLametta]

@jtl999 Ping. 😉

/remind jtl999 to please review this issue in 3 days

[reminders]

@JoeLametta set a reminder for Apr 5th 2019

[reminders]

👋 @jtl999, please review this issue

[JoeLametta]

@jtl999 Ping. 😉

[Freso]

I can borrow the CD again and try with and without the regex, but I’d really like if someone can think up a way to test this so we can prevent it from regressing in the future.

[Freso]

I tried this with Salif Keita’s ‘“Folon”… The Past’ (Disc ID) which gave the same error, and @srussel’s regex from #374 (comment) change fixes the issue for me.

I would still very much like if this could be tested to prevent future regressions, but if no one can come up with a way to do this, I’d say we should just get the fix in so we can move closer to a 0.8.0 release.

[ConcernedOne]

Howdy.
Disc Thundra - Ignored by Fear (https://musicbrainz.org/release/e76e5936-328d-457f-9522-e9d0ccf25186), same error.
Successfully fixed with solution in comment #374.
The disc is in my possession indefinitely, please reach me out if you like to test something else.

whipper 0.7.4.dev76+g57d386e

[jtl999]

I have emerged from my absence

I haven't touched anything whipper related since last December but while I was testing #345 I don't recall experiencing the issue in question with any of my discs.

I just checked and the environment I was using to test was an Ubuntu 18.04 LXC container with sr0 block device passthrough, whipper from git (a bit vague because I've used both "my personal fork" and upstream in the past) and cdrdao v1.2.3 from Ubuntu 18.04's universe repository.

At some point I'll see if I can rip a bunch of discs with a recent whipper build and reproduce this issue.

[JoeLametta]

I would still very much like if this could be tested to prevent future regressions, but if no one can come up with a way to do this, I’d say we should just get the fix in so we can move closer to a 0.8.0 release.

👍

[JoeLametta]

Fixed thanks to #418.