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

Fix cover file saving with /tmp on different FS #458

Merged
merged 1 commit into from
Jan 28, 2020
Merged

Fix cover file saving with /tmp on different FS #458

merged 1 commit into from
Jan 28, 2020

Conversation

kevinoid
Copy link
Contributor

If the directory used by tempfile.NamedTemporaryFile is on a different filesystem (e.g. /tmp on tmpfs), whipper cd rip --cover-art will fail with an error such as:

Traceback (most recent call last):
  File "/usr/bin/whipper", line 11, in <module>
    load_entry_point('whipper==0.9.0', 'console_scripts', 'whipper')()
  File "/home/kevin/tmp/whipper/whipper/command/main.py", line 43, in main
    ret = cmd.do()
  File "/home/kevin/tmp/whipper/whipper/command/basecommand.py", line 139, in do
    return self.cmd.do()
  File "/home/kevin/tmp/whipper/whipper/command/basecommand.py", line 139, in do
    return self.cmd.do()
  File "/home/kevin/tmp/whipper/whipper/command/cd.py", line 191, in do
    self.doCommand()
  File "/home/kevin/tmp/whipper/whipper/command/cd.py", line 363, in doCommand
    self.program.metadata.mbid)
  File "/home/kevin/tmp/whipper/whipper/common/program.py", line 498, in getCoverArt
    os.replace(f.name, cover_art_path)
OSError: [Errno 18] Invalid cross-device link: '/tmp/tmprmx4d9c9.cover.jpg' -> './Boston/Greatest Hits/cover.jpg'

due to calling os.replace with paths on different filesystems.

Instead of os.replace, this PR uses shutil.move, which falls back to shutil.copy2 if os.replace doesn't work.

Thanks for considering,
Kevin

If the directory used by tempfile.NamedTemporaryFile is on a different
filesystem (e.g. /tmp on tmpfs), `whipper cd rip --cover-art` will fail
with an error such as:

    Traceback (most recent call last):
      File "/usr/bin/whipper", line 11, in <module>
        load_entry_point('whipper==0.9.0', 'console_scripts', 'whipper')()
      File "/home/kevin/tmp/whipper/whipper/command/main.py", line 43, in main
        ret = cmd.do()
      File "/home/kevin/tmp/whipper/whipper/command/basecommand.py", line 139, in do
        return self.cmd.do()
      File "/home/kevin/tmp/whipper/whipper/command/basecommand.py", line 139, in do
        return self.cmd.do()
      File "/home/kevin/tmp/whipper/whipper/command/cd.py", line 191, in do
        self.doCommand()
      File "/home/kevin/tmp/whipper/whipper/command/cd.py", line 363, in doCommand
        self.program.metadata.mbid)
      File "/home/kevin/tmp/whipper/whipper/common/program.py", line 498, in getCoverArt
        os.replace(f.name, cover_art_path)
    OSError: [Errno 18] Invalid cross-device link: '/tmp/tmprmx4d9c9.cover.jpg' -> './Boston/Greatest Hits/cover.jpg'

due to calling os.replace with paths on different filesystems.

Instead of os.replace, use shutil.move, which falls back to shutil.copy2
if os.replace doesn't work.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
@JoeLametta JoeLametta merged commit ee11fac into whipper-team:develop Jan 28, 2020
@JoeLametta
Copy link
Collaborator

Merged, thanks!
I need to check whether os.replace / os.rename has been used in other places in a similar (wrong) way.

@kevinoid
Copy link
Contributor Author

Thanks for reviewing and merging!

Good thought looking for similar issues. Mounting a separate tmpfs on $HOME/.config/whipper, $TEMP, working_directory, and output_directory might be a reasonable smoke test. Not sure if you want to bother supporting more pathological cases (e.g. album directory mounted on different FS).

@JoeLametta
Copy link
Collaborator

Hi, I've just replaced all occurrences I've found in whipper of os.replace and os.rename with shutil.move (in commit b39345e).

@kevinoid
Copy link
Contributor Author

Great, thanks @JoeLametta!

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.

2 participants