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

Do not use tempfile + rename for --write-info-json when --no-part is used #11983

Closed
jamshid opened this issue Feb 5, 2017 · 2 comments
Closed

Do not use tempfile + rename for --write-info-json when --no-part is used #11983

jamshid opened this issue Feb 5, 2017 · 2 comments

Comments

@jamshid
Copy link

@jamshid jamshid commented Feb 5, 2017

Please follow the guide below

  • You will be asked some questions and requested to provide some information, please read them carefully and answer honestly
  • Put an x into all the boxes [ ] relevant to your issue (like that [x])
  • Use Preview tab to see how your issue will actually look like

Make sure you are using the latest version: run youtube-dl --version and ensure your version is 2017.02.04.1. If it's not read this FAQ entry and update. Issues with outdated version will be rejected.

  • [X ] I've verified and I assure that I'm running youtube-dl 2017.02.04.1

Before submitting an issue make sure you have:

  • [X ] At least skimmed through README and most notably FAQ and BUGS sections
  • [ X] Searched the bugtracker for similar issues including closed ones

What is the purpose of your issue?

  • Bug report (encountered problems with youtube-dl)
  • Site support request (request for adding support for a new site)
  • [ X] Feature request (request for a new functionality)
  • Question
  • Other

The following sections concretize particular purposed issues, you can erase any section (the contents between triple ---) not applicable to your issue


If the purpose of this issue is a bug report, site support request or you are not completely sure provide the full verbose output as follows:

Add -v flag to your command line you run youtube-dl with, copy the whole output and insert it here. It should look similar to one below (replace it with your log inserted between triple ```):

[root@9f36c08f1ee7 foo]# youtube-dl -v --no-part --write-info-json --write-description --write-annotations --write-all-thumbnails  https://www.youtube.com/watch?v=_zR6ROjoOX0
[debug] System config: []
[debug] User config: []
[debug] Custom config: []
[debug] Command-line args: [u'-v', u'--no-part', u'--write-info-json', u'--write-description', u'--write-annotations', u'--write-all-thumbnails', u'https://www.youtube.com/watch?v=_zR6ROjoOX0']
[debug] Encodings: locale UTF-8, fs UTF-8, out UTF-8, pref UTF-8
[debug] youtube-dl version 2017.02.04.1
[debug] Python version 2.7.5 - Linux-4.4.0-59-generic-x86_64-with-centos-7.3.1611-Core
[debug] exe versions: none
[debug] Proxy map: {}
[youtube] _zR6ROjoOX0: Downloading webpage
[youtube] _zR6ROjoOX0: Downloading video info webpage
[youtube] _zR6ROjoOX0: Extracting video information
[youtube] _zR6ROjoOX0: Searching for annotations.
[youtube] {22} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {43} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {18} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {36} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {17} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {137} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {248} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {136} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {247} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {135} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {244} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {134} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {243} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {133} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {242} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {160} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {278} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {140} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {171} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {249} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {250} signature length 42.43, html5 player en_US-vflkk7pUE
[youtube] {251} signature length 42.43, html5 player en_US-vflkk7pUE
[info] Writing video description to: Iggy Azalea - Work (Explicit)-_zR6ROjoOX0.description
[info] Writing video annotations to: Iggy Azalea - Work (Explicit)-_zR6ROjoOX0.annotations.xml
[info] Writing video description metadata as JSON to: Iggy Azalea - Work (Explicit)-_zR6ROjoOX0.info.json
ERROR: Cannot write metadata to JSON file Iggy Azalea - Work (Explicit)-_zR6ROjoOX0.info.json
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/youtube_dl/YoutubeDL.py", line 1631, in process_info
    write_json_file(self.filter_requested_info(info_dict), infofn)
  File "/usr/lib/python2.7/site-packages/youtube_dl/utils.py", line 243, in write_json_file
    os.rename(tf.name, fn)
OSError: [Errno 5] Input/output error

Description of your issue, suggested solution and other information

The --no-part flag is great because some limited file systems (e.g. some object storage -backed FUSE or NFS) do not allow file rename or it's expensive. It would be great if this flag also applied to the .json file written with --write-info-json. The fact that the write is not atomic seems like an acceptable limitation.

Below code changes seem to work but not sure how to get the cli params, will try to look into it and submit a pull request ok?

--- utils.py	2017-02-05 22:52:02.560525000 +0000
+++ /tmp/utils.py	2017-02-05 22:48:18.181943082 +0000
@@ -228,22 +228,24 @@
             'encoding': 'utf-8',
         })
 
-    tf = tempfile.NamedTemporaryFile(**compat_kwargs(args))
+    useTempFile = False # not self.params.get('nopart', False) # some file systems don't allow rename
+    tf = tempfile.NamedTemporaryFile(**compat_kwargs(args)) if useTempFile else open(fn, 'wb') # TODO: encoding='utf-8' on python3?
 
     try:
         with tf:
             json.dump(obj, tf)
-        if sys.platform == 'win32':
+        if useTempFile and sys.platform == 'win32':
             # Need to remove existing file on Windows, else os.rename raises
             # WindowsError or FileExistsError.
             try:
                 os.unlink(fn)
             except OSError:
                 pass
-        os.rename(tf.name, fn)
+        if useTempFile:
+            os.rename(tf.name, fn)
     except Exception:
         try:
-            os.remove(tf.name)
+            os.remove(tf.name if useTempFile else fn)
         except OSError:
             pass
         raise
@dstftw
Copy link
Collaborator

@dstftw dstftw commented Feb 6, 2017

--no-part should not be mixed here. --no-part disables usage of .part files for downloadable media, no more no less.
There are also lots of other places where os.rename is used in code base. There is also another problem introduced by not using temp files - inability to guess whether some part of extraction process finished properly last time or not without storing additional data.
So I don't see much point changing and maintaining this for just some very rare cases. If you'are about to use youtube-dl be prepared to run it on proper filesystem that does allow renaming.

@dstftw dstftw closed this Feb 6, 2017
@yan12125
Copy link
Collaborator

@yan12125 yan12125 commented Feb 6, 2017

@jamshid FYI, here's another proposal, which is much simpler: #10993

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.