Store ToC file generated by cdrdao #277

JoeLametta · 2018-06-08T15:48:20Z

Fixes #214.

This FR isn't implemented as an option because I think always having the original toc file could be useful for debugging purposes, burning a copy of the source CD and it's a tiny file anyway...

RecursiveForest · 2018-06-08T20:36:23Z

What does 'FR' mean?

RecursiveForest · 2018-06-08T20:53:19Z

whipper/common/program.py

@@ -614,3 +617,12 @@ def writeLog(self, discName, logger):
        self.logPath = logPath

        return logPath
+
+    def writeToc(self, discName):


I stumbled for a moment on the name of this-- what's actually happening is we're moving a previous created tocfile to the output directory, so maybe we should make the method name (and docstring if necessary) reflect this.

I thought about this too: decided to use this name/style to mimic the existing code style. It's true in this case it's a bit deceptive: I'll amend this (+ add a docstring). What would be a good name for the method?

What about this?

def move_tocfile(self, discName): """Move cdrdao's tocfile to the discname path (.toc ending)"""

RecursiveForest · 2018-06-08T20:54:55Z

whipper/common/program.py

+
+    def writeToc(self, discName):
+        tocPath = '%s.toc' % discName
+        logger.debug('write .toc file to %s', tocPath)


Let's use the present active: "writing .toc file to %s"

As there's a similar logger.debug statement in whipper/command/cd.py maybe I should drop one of them? (written it like this to mimic the existing code style).

RecursiveForest · 2018-06-08T21:02:26Z

whipper/command/offset.py

@@ -85,7 +85,7 @@ def do(self):
        utils.unmount_device(device)

        # first get the Table Of Contents of the CD
-        t = cdrdao.ReadTOCTask(device)
+        t = cdrdao.ReadTOCTask(device)[0]


If we keep the "return a tuple" behaviour for ReadTOCTask (which is a little awkward but it's not a big deal), we should write this as t, _ = cdrdao.ReadTOCTask(device), which is a little less opaque than [0] in my opinion. Maybe a comment explaining the tocfile is unused (e.g., ...Task(device) # tocfile unused)

You're right, I'll improve this as suggested.

P.S. What would be a better way to avoid the awkward return of a tuple?

JoeLametta · 2018-06-09T06:51:19Z

What does 'FR' mean?

Thanks for the pull request review.
I meant Feature Request.

RecursiveForest · 2018-06-09T14:55:24Z

whipper/common/program.py

@@ -614,3 +617,13 @@ def writeLog(self, discName, logger):
        self.logPath = logPath

        return logPath
+
+    def move_tocfile(self, discName):
+        """Move cdrdao's tocfile to the discname path (.toc ending)"""


This could be a little more clear. Maybe "to the output directory ('DISCNAME.toc')", "... ('[discname].toc')", etc.

RecursiveForest · 2018-06-09T14:56:42Z

whipper/program/cdrdao.py

@@ -43,8 +43,7 @@ def read_toc(device, fast_toc=False):

    toc = TocFile(tocfile)
    toc.parse()
-    os.unlink(tocfile)


Does this mean that when the tocfile is unused (like for whipper offset), we have a /tmp-file that isn't cleaned up?

RecursiveForest · 2018-06-11T02:23:16Z

whipper/common/program.py

@@ -614,3 +618,11 @@ def writeLog(self, discName, logger):
        self.logPath = logPath

        return logPath
+
+    def move_tocfile(self, discName):
+        """Move cdrdao's tocfile to the output dir. ('[discName].toc')"""


This is a nitpick, but now I think "Move cdrdao's tocfile to output-dir/discName.toc" would read the best.

RecursiveForest · 2018-06-11T02:28:11Z

whipper/common/program.py

+
+    def move_tocfile(self, discName):
+        """Move cdrdao's tocfile to the output dir. ('[discName].toc')"""
+        tocPath = '%s.toc' % discName


~~These variable names are confusing. Maybe something like result_toc_path to differentiate the new path from the temporary path.~~

Honestly I think we might be doing this backwards. Maybe it's best to move the toc from /tmp to the result directory in the read_toc() function itself if persist_tocfile is True and just return toc in read_toc() like before. I think it would keep things simpler.

I agree.
Let's say I've done this:

diff --git a/whipper/program/cdrdao.py b/whipper/program/cdrdao.py index e0da15c..778fb7a 100644 --- a/whipper/program/cdrdao.py +++ b/whipper/program/cdrdao.py @@ -1,5 +1,6 @@ import os import re +import shutil import tempfile from subprocess import Popen, PIPE @@ -12,7 +13,7 @@ logger = logging.getLogger(__name__) CDRDAO = 'cdrdao' -def read_toc(device, fast_toc=False): +def read_toc(device, fast_toc=False, toc_path=None): """ Return cdrdao-generated table of contents for 'device'. """ @@ -43,7 +44,10 @@ def read_toc(device, fast_toc=False): toc = TocFile(tocfile) toc.parse() - os.unlink(tocfile) + if toc_path is not None: + shutil.move(tocfile, toc_path) + else: + os.unlink(tocfile) return toc @@ -83,14 +87,14 @@ def ReadTOCTask(device): """ stopgap morituri-insanity compatibility layer """ - return read_toc(device, fast_toc=True) + return read_toc(device, fast_toc=True, toc_path=None) def ReadTableTask(device): """ stopgap morituri-insanity compatibility layer """ - return read_toc(device) + return read_toc(device, toc_path=None) def getCDRDAOVersion():

Who gives me the proper toc_path? (unfortunately it seems that one isn't available both in getFastToc() and in getTable() methods).

P.S. I'm quite tired right now so I may be missing something obvious.

From cd.py, pass the path to the function that ultimately calls read_toc(): something like self.program.getTable(..., output_dir) and self.program.getFastToc(..., output_dir). (This could be kwarg with default =None if we don't want to always persist the toc.) Then pass output_dir to read_toc().

We will need to make sure that the output directory has been created by the time we persist the toc though, right now that happens after both calls to getFastToc() and getTable(). As far as I can tell, we're committed to ripping after getFastToc() but before getTable(), so we should have getTable() be the call that persists the toc.

JoeLametta · 2018-10-27T13:51:39Z

New solution implemented and tested: seems to be working as expected.

RFC: @MerlijnWajer, @RecursiveForest.

Whipper uses cdrdao during its ripping process. With this commit it will now store cdrdao's generated tocfile in the ripping path. Preserving the tocfile allows users to easily burn ripped discs having a non-compliant cue sheet. Fixes #214.

JoeLametta · 2018-11-02T09:02:20Z

Adopted git flow: I'm closing this pull request, new one will be available in #321.

JoeLametta force-pushed the cdrdao-toc branch from 263fba0 to 1cfe6c5 Compare June 8, 2018 20:26

RecursiveForest reviewed Jun 8, 2018

View reviewed changes

RecursiveForest reviewed Jun 9, 2018

View reviewed changes

RecursiveForest reviewed Jun 11, 2018

View reviewed changes

MerlijnWajer self-requested a review June 11, 2018 07:25

JoeLametta force-pushed the cdrdao-toc branch 2 times, most recently from 31aa1cd to b87a970 Compare October 27, 2018 13:48

Preserve ToC file generated by cdrdao

59ef066

Whipper uses cdrdao during its ripping process. With this commit it will now store cdrdao's generated tocfile in the ripping path. Preserving the tocfile allows users to easily burn ripped discs having a non-compliant cue sheet. Fixes #214.

JoeLametta force-pushed the cdrdao-toc branch from 960618e to 59ef066 Compare November 1, 2018 13:51

JoeLametta changed the base branch from master to develop November 2, 2018 08:48

JoeLametta mentioned this pull request Nov 2, 2018

Preserve ToC file generated by cdrdao #321

Merged

JoeLametta closed this Nov 2, 2018

JoeLametta deleted the cdrdao-toc branch November 2, 2018 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store ToC file generated by cdrdao #277

Store ToC file generated by cdrdao #277

JoeLametta commented Jun 8, 2018 •

edited

Loading

RecursiveForest commented Jun 8, 2018

RecursiveForest Jun 8, 2018

JoeLametta Jun 9, 2018

JoeLametta Jun 9, 2018 •

edited

Loading

RecursiveForest Jun 8, 2018

JoeLametta Jun 9, 2018 •

edited

Loading

RecursiveForest Jun 8, 2018

JoeLametta Jun 9, 2018 •

edited

Loading

JoeLametta commented Jun 9, 2018 •

edited

Loading

RecursiveForest Jun 9, 2018

RecursiveForest Jun 9, 2018

RecursiveForest Jun 11, 2018

RecursiveForest Jun 11, 2018

JoeLametta Jun 13, 2018

RecursiveForest Jun 19, 2018 •

edited

Loading

JoeLametta commented Oct 27, 2018

JoeLametta commented Nov 2, 2018

Store ToC file generated by cdrdao #277

Store ToC file generated by cdrdao #277

Conversation

JoeLametta commented Jun 8, 2018 • edited Loading

RecursiveForest commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeLametta Jun 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeLametta Jun 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeLametta Jun 9, 2018 • edited Loading

Choose a reason for hiding this comment

JoeLametta commented Jun 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RecursiveForest Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

JoeLametta commented Oct 27, 2018

JoeLametta commented Nov 2, 2018

JoeLametta commented Jun 8, 2018 •

edited

Loading

JoeLametta Jun 9, 2018 •

edited

Loading

JoeLametta Jun 9, 2018 •

edited

Loading

JoeLametta Jun 9, 2018 •

edited

Loading

JoeLametta commented Jun 9, 2018 •

edited

Loading

RecursiveForest Jun 19, 2018 •

edited

Loading