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

hook: Interpolate paths (and other bytestrings) correctly into commands #2967

Closed
mike2725 opened this issue Jun 24, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@mike2725
Copy link

commented Jun 24, 2018

Problem

I have the following configuration for the Hook plugin in my config.yaml:

hook:
    hooks:
        - event: album_imported
          command: /usr/bin/ls -l "{album.path}"

This is just a test to see how beets presents the path values. It appears that the paths are returned as bytes objects rather than strings. This is problematic when using the path values as arguments for external shell scripts. As can be seen below, the shell is unable to use the value provided by {album.path}.

$ beet -vv import /tmp/music/new/Al\ Di\ Meola\ -\ Elegant\ Gypsy/

Led to this problem:

hook: running command "/usr/bin/ls -l b'/tmp/music/FLAC/Al Di Meola/Elegant Gypsy'" for event album_imported
/usr/bin/ls: cannot access "b'/tmp/music/FLAC/Al Di Meola/Elegant Gypsy'": No such file or directory

The path "/tmp/music/FLAC/Al Di Meola/Elegant Gypsy" does exist on the filesystem after the import is complete.

Setup

  • OS: Arch Linux
  • Python version: 3.4.5
  • beets version: 1.4.7
  • Turning off plugins made problem go away (yes/no): This issue is related to a plugin, so I didn't turn them off

My configuration (output of beet config) is:

plugins: inline convert badfiles info missing lastgenre fetchart mbsync scrub smartplaylist hook
directory: /tmp/music/FLAC
library: ~/.config/beets/library.db

import:
    copy: yes
    write: yes
    log: ~/.config/beets/import.log
    languages: en
per_disc_numbering: yes

paths:
    default: $albumartist/$album%aunique{}/$disc_and_track - $title
    comp: $albumartist/$album%aunique{}/$disc_and_track - $title
item_fields:
    disc_and_track: u'%01i-%02i' % (disc, track) if disctotal > 1 else u'%02i' % (track)

ui:
    color: yes

match:
    ignored: missing_tracks unmatched_tracks
convert:
    copy_album_art: yes
    dest: /tmp/music/ogg
    embed: yes
    never_convert_lossy_files: yes
    format: ogg
    formats:
        ogg:
            command: oggenc -Q -q 4 -o $dest $source
            extension: ogg
        aac:
            command: ffmpeg -i $source -y -vn -acodec aac -aq 1 $dest
            extension: m4a
        alac:
            command: ffmpeg -i $source -y -vn -acodec alac $dest
            extension: m4a
        flac: ffmpeg -i $source -y -vn -acodec flac $dest
        mp3: ffmpeg -i $source -y -vn -aq 2 $dest
        opus: ffmpeg -i $source -y -vn -acodec libopus -ab 96k $dest
        wma: ffmpeg -i $source -y -vn -acodec wmav2 -vn $dest
    pretend: no
    threads: 4
    max_bitrate: 500
    auto: no
    tmpdir:
    quiet: no

    paths: {}
    no_convert: ''
    album_art_maxwidth: 0
lastgenre:
    force: yes
    prefer_specific: no
    min_weight: 20
    count: 4
    separator: '; '
    whitelist: yes
    fallback:
    canonical: no
    source: album
    auto: yes
fetchart:
    sources: filesystem coverart amazon albumart
    auto: yes
    minwidth: 0
    maxwidth: 0
    enforce_ratio: no
    cautious: no
    cover_names:
    - cover
    - front
    - art
    - album
    - folder
    google_key: REDACTED
    google_engine: 001442825323518660753:hrh5ch1gjzm
    fanarttv_key: REDACTED
    store_source: no
hook:
    hooks: [{event: album_imported, command: '/usr/bin/ls -l "{album.path}"'}]
pathfields: {}
album_fields: {}
scrub:
    auto: yes
missing:
    count: no
    total: no
    album: no
smartplaylist:
    relative_to:
    playlist_dir: .
    auto: yes
    playlists: []

I created a Python 2 virtual environment, installed beets and any dependencies in to that virtualenv, cleaned my test library, and imported the same files using the same config.yaml. This time the shell was able to use the path value returned by the hook configuration:

hook: running command "/usr/bin/ls -l /tmp/music/FLAC/Al Di Meola/Elegant Gypsy" for event album_imported
total 254944
-rw-r--r-- 1 mike mike 50409756 Jun 24 13:46 01 - Flight Over Rio.flac
-rw-r--r-- 1 mike mike 43352354 Jun 24 13:46 02 - Midnight Tango.flac
-rw-r--r-- 1 mike mike  7726389 Jun 24 13:46 03 - Percussion Intro.flac
-rw-r--r-- 1 mike mike 32184646 Jun 24 13:46 04 - Mediterranean Sundance.flac
-rw-r--r-- 1 mike mike 45770796 Jun 24 13:46 05 - Race With Devil on Spanish Highway.flac
-rw-r--r-- 1 mike mike 10421006 Jun 24 13:46 06 - Lady of Rome, Sister of Brazil.flac
-rw-r--r-- 1 mike mike 65807504 Jun 24 13:46 07 - Elegant Gypsy Suite.flac
-rw-r--r-- 1 mike mike  5366515 Jun 24 13:46 cover.jpg

I'm guessing this is due to a data type difference between Python 2 and Python 3.

@sampsyo sampsyo added the bug label Jun 25, 2018

@sampsyo sampsyo changed the title Paths Returned by Hook Events / Parameters are bytes Objects Instead of Strings hook: Interpolate paths (and other bytestrings) correctly into commands Jun 25, 2018

@sampsyo sampsyo added the python 3 label Jun 25, 2018

@sampsyo

This comment has been minimized.

Copy link
Member

commented Jun 25, 2018

Thank you! This is indeed a Python 3 regression. The problem is that we’re interpolating paths (which are bytes) into commands (which are Unicode) stings. Rather than just crash, Python 3 opts to use repr() to convert the bytes objects to strings for interpolation. This obviously won’t work for executing hooks that need actual filenames.

Fixing this will require some care. We’ll need to convert the commands (and any string parameters) to bytes and include the raw, unconverted filenames in the output. This is not something that plain string templating can do, it will require some sort of hack to get right.

@radkoff

This comment has been minimized.

Copy link

commented Feb 21, 2019

I'm hitting this issue as well. As a terrible temporary hacky workaround, I'm passing the filename to a bash script, where I'm doing echo $1 | cut -c 3- | rev | cut -c 2- | rev | xargs -0 osascript /Users/evan/bin/iTunesRefresh.scpt. This turns b'foo/path' into foo/path

@jackwilsdon

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

@sampsyo I've not tried it out but is there a reason our custom CodingFormatter is only being used on Python 2? I recall writing it to specifically handle encoding interpolated parameters.

beets/beetsplug/hook.py

Lines 27 to 70 in be118b9

class CodingFormatter(string.Formatter):
"""A variant of `string.Formatter` that converts everything to `unicode`
strings.
This is necessary on Python 2, where formatting otherwise occurs on
bytestrings. It intercepts two points in the formatting process to decode
the format string and all fields using the specified encoding. If decoding
fails, the values are used as-is.
"""
def __init__(self, coding):
"""Creates a new coding formatter with the provided coding."""
self._coding = coding
def format(self, format_string, *args, **kwargs):
"""Formats the provided string using the provided arguments and keyword
arguments.
This method decodes the format string using the formatter's coding.
See str.format and string.Formatter.format.
"""
try:
format_string = format_string.decode(self._coding)
except UnicodeEncodeError:
pass
return super(CodingFormatter, self).format(format_string, *args,
**kwargs)
def convert_field(self, value, conversion):
"""Converts the provided value given a conversion type.
This method decodes the converted value using the formatter's coding.
See string.Formatter.convert_field.
"""
converted = super(CodingFormatter, self).convert_field(value,
conversion)
if isinstance(converted, bytes):
return converted.decode(self._coding)
return converted

beets/beetsplug/hook.py

Lines 99 to 102 in be118b9

if six.PY2:
formatter = CodingFormatter(arg_encoding())
else:
formatter = string.Formatter()

@sampsyo

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Huh! It definitely looks like it's my fault—I committed this in 039825e.

That does look wrong to me now—this formatter is exactly what you'd want if you were to be interpolating byte strings into Unicode templates, which is exactly what we're doing. 😳

I think I was probably just mistaken at the time! Can you please check whether everything just works if we enable the formatter on Python 3 too? That would be great but extremely embarrassing for me.

@jackwilsdon

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Looks like removing that check fixes it with some minor additional changes. I've opened #3167 which also adds a test that catches the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.