From ca14b60aca99090895c636edc95e0ca9c5469dfa Mon Sep 17 00:00:00 2001 From: John Hawkinson Date: Sat, 8 Oct 2016 09:27:24 -0400 Subject: [PATCH 1/6] Print traceback when raising UnavailableVideoError An OSError or IOError generally indicates something a little more wrong than a "simple" UnavailableVideoError, so print the actual traceback that leads to the exception. Otherwise meaningful postmortem debugging a bug report is essentially infeasible. --- youtube_dl/YoutubeDL.py | 1 + 1 file changed, 1 insertion(+) diff --git a/youtube_dl/YoutubeDL.py b/youtube_dl/YoutubeDL.py index 99825e343..166fe8b15 100755 --- a/youtube_dl/YoutubeDL.py +++ b/youtube_dl/YoutubeDL.py @@ -1701,6 +1701,7 @@ class YoutubeDL(object): self.report_error('unable to download video data: %s' % error_to_compat_str(err)) return except (OSError, IOError) as err: + traceback.print_exc() raise UnavailableVideoError(err) except (ContentTooShortError, ) as err: self.report_error('content too short (expected %s bytes and served %s)' % (err.expected, err.downloaded)) From 99315aa2527cedebbe55210f6ce20e7c3b5c273a Mon Sep 17 00:00:00 2001 From: John Hawkinson Date: Sat, 8 Oct 2016 09:51:00 -0400 Subject: [PATCH 2/6] ExternalFD:real_download(): Skip getsize/rename when writing to stdout In the real_download() method of the ExternalFD class: Special-case output to stdout, when filename=='-', because otherwise the call to os.path.getsize() raises OSError: [Errno 2] No such file or directory: '-' which in turn gets masked by a try block and re-raised as UnavailableVideoError(). And even if getsize had succeeded, we would fail on renaming, and then we would also fail on reporting progress as report_progress() expects to have file size information. So report download completion with self.to_screen() just as report_progress() would in the noprogress==True case. You might ask whether it's an abstraction violation for external.py to have to know and think about '-' as a filename, but unfortunately we're there already. _call_downloader() thinks about it, and also real_download() calls common.py's temp_name() which special-cases the '-' filename to preserve it instead of returnign a temp file. So we already worry about this case at this abstraction level, as well as both below and above. Perhaps that should change, but that's for another day. --- youtube_dl/downloader/external.py | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/youtube_dl/downloader/external.py b/youtube_dl/downloader/external.py index 0aeae3b8f..fab137199 100644 --- a/youtube_dl/downloader/external.py +++ b/youtube_dl/downloader/external.py @@ -27,15 +27,20 @@ class ExternalFD(FileDownloader): retval = self._call_downloader(tmpfilename, info_dict) if retval == 0: - fsize = os.path.getsize(encodeFilename(tmpfilename)) - self.to_screen('\r[%s] Downloaded %s bytes' % (self.get_basename(), fsize)) - self.try_rename(tmpfilename, filename) - self._hook_progress({ - 'downloaded_bytes': fsize, - 'total_bytes': fsize, - 'filename': filename, - 'status': 'finished', - }) + if filename == '-': + # xxx report_progress() expects total_bytes to be set or it throws a KeyError, so + # we can't just call: self._hook_progress({'status': 'finished'}) + self.to_screen('[download] Download completed') + else: + fsize = os.path.getsize(encodeFilename(tmpfilename)) + self.to_screen('\r[%s] Downloaded %s bytes' % (self.get_basename(), fsize)) + self.try_rename(tmpfilename, filename) + self._hook_progress({ + 'downloaded_bytes': fsize, + 'total_bytes': fsize, + 'filename': filename, + 'status': 'finished', + }) return True else: self.to_stderr('\n') From cb1cea4b34c1d73f7c512ecc74c5d97c21456d85 Mon Sep 17 00:00:00 2001 From: John Hawkinson Date: Sun, 9 Oct 2016 08:17:05 -0400 Subject: [PATCH 3/6] Revert "Print traceback when raising UnavailableVideoError" This reverts commit ca14b60aca99090895c636edc95e0ca9c5469dfa. Moved to PR #10888. --- youtube_dl/YoutubeDL.py | 1 - 1 file changed, 1 deletion(-) diff --git a/youtube_dl/YoutubeDL.py b/youtube_dl/YoutubeDL.py index 166fe8b15..99825e343 100755 --- a/youtube_dl/YoutubeDL.py +++ b/youtube_dl/YoutubeDL.py @@ -1701,7 +1701,6 @@ class YoutubeDL(object): self.report_error('unable to download video data: %s' % error_to_compat_str(err)) return except (OSError, IOError) as err: - traceback.print_exc() raise UnavailableVideoError(err) except (ContentTooShortError, ) as err: self.report_error('content too short (expected %s bytes and served %s)' % (err.expected, err.downloaded)) From 5a1e040b8e62ae5bf20042768b002378c801747b Mon Sep 17 00:00:00 2001 From: John Hawkinson Date: Sun, 9 Oct 2016 10:36:40 -0400 Subject: [PATCH 4/6] report_progress() shouldn't fail w/o total_bytes In the absence of the total_bytes key, just say "Completed" (looks better than a bare "100%"). Compose the msg_template additively. In practice, for pipes (where we lack total bytes) we also lack elapsed time, so we end up with: [download] Completed though if elapsed time were somehow set we'd have: [download] Completed in 00:10 --- youtube_dl/downloader/common.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/youtube_dl/downloader/common.py b/youtube_dl/downloader/common.py index 8482cbd84..c23263511 100644 --- a/youtube_dl/downloader/common.py +++ b/youtube_dl/downloader/common.py @@ -242,12 +242,14 @@ class FileDownloader(object): if self.params.get('noprogress', False): self.to_screen('[download] Download completed') else: - s['_total_bytes_str'] = format_bytes(s['total_bytes']) + if s.get('total_bytes') is not None: + s['_total_bytes_str'] = format_bytes(s['total_bytes']) + msg_template = '100%% of %(_total_bytes_str)s' + else: + msg_template = 'Completed' if s.get('elapsed') is not None: s['_elapsed_str'] = self.format_seconds(s['elapsed']) - msg_template = '100%% of %(_total_bytes_str)s in %(_elapsed_str)s' - else: - msg_template = '100%% of %(_total_bytes_str)s' + msg_template += ' in %(_elapsed_str)s' self._report_progress_status( msg_template % s, is_last_line=True) From f1f0b34a4e2ab00caa37d48878d0858a330aa54e Mon Sep 17 00:00:00 2001 From: John Hawkinson Date: Sun, 9 Oct 2016 10:38:00 -0400 Subject: [PATCH 5/6] Call _hook_progress() instead of faking it Since now report_progress() handles lack of a file size. --- youtube_dl/downloader/external.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/youtube_dl/downloader/external.py b/youtube_dl/downloader/external.py index fab137199..43bed3648 100644 --- a/youtube_dl/downloader/external.py +++ b/youtube_dl/downloader/external.py @@ -28,9 +28,7 @@ class ExternalFD(FileDownloader): retval = self._call_downloader(tmpfilename, info_dict) if retval == 0: if filename == '-': - # xxx report_progress() expects total_bytes to be set or it throws a KeyError, so - # we can't just call: self._hook_progress({'status': 'finished'}) - self.to_screen('[download] Download completed') + self._hook_progress({'status': 'finished'}) else: fsize = os.path.getsize(encodeFilename(tmpfilename)) self.to_screen('\r[%s] Downloaded %s bytes' % (self.get_basename(), fsize)) From 356672c250dd3ea55d55620fca8e595cc8043e9d Mon Sep 17 00:00:00 2001 From: John Hawkinson Date: Sat, 15 Oct 2016 05:11:53 -0400 Subject: [PATCH 6/6] _hook_progress() with 'filename' when writing to stdout --- youtube_dl/downloader/external.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/youtube_dl/downloader/external.py b/youtube_dl/downloader/external.py index 43bed3648..b5f90b541 100644 --- a/youtube_dl/downloader/external.py +++ b/youtube_dl/downloader/external.py @@ -28,7 +28,10 @@ class ExternalFD(FileDownloader): retval = self._call_downloader(tmpfilename, info_dict) if retval == 0: if filename == '-': - self._hook_progress({'status': 'finished'}) + self._hook_progress({ + 'filename': filename, + 'status': 'finished', + }) else: fsize = os.path.getsize(encodeFilename(tmpfilename)) self.to_screen('\r[%s] Downloaded %s bytes' % (self.get_basename(), fsize))