On Python 3, rarfile<3.0 does not support bytes filename

[Lompik]

Problem

Importing rar files does not work anymore. It used to work with beets 1.4.2 and below.

The underlying problem is that is_rarfile does no accept bytes (and toppath variable is byte). See this commit markokr/rarfile@7d7a2cb where future versions (once or twice a year) of rarfile will gladly accepts bytes.

Running this command in verbose (-vv) mode:

beet -vv import /mnt/E/Album\ -\ group.rar

Led to this problem:

user configuration: /home/lompik/.config/beets/config.yaml
data directory: /home/lompik/.config/beets
plugin paths: 
Sending event: pluginload
library database: /home/lompik/.data/musiklibrary.blb
library directory: /mnt/music
Sending event: library_opened
Sending event: import_begin
Extracting archive: /mnt/E/Album - group.rar
extraction failed: Invalid object passed as file
No files imported from /mnt/E/Album - group.rar
Sending event: import
Sending event: cli_exit

Setup

• OS: Linux
• Python version: 3.6
• beets version: 1.4.3 / rafile version: 3.0
• Turning off plugins made problem go away (yes/no): no

EDIT: change filename

[ghost]

In most cases we do pass string file names to rarfile, but we must have missed one :(

[sampsyo]

Thanks for pointing that out! It looks like we'll need to either (a) hope for a speedy release of the new version of rarfile, or (b) add an adapter to ArchiveImportTask that optimistically decodes the path using the filesystem encoding (and quietly fails, just logging an error, if it can't be decoded).

[ghost]

@sampsyo shouldn't util.py3_path work ?

[sampsyo]

Hmm, you're right that it should… sorry I didn't think of that!

But we already pass the archive path through py3_path, so I'm a little bit mystified:

beets/beets/importer.py

Line 990 in 8ccdb1c

archive = handler_class(util.py3_path(self.toppath), mode='r')

[ghost]

I wonder which line extraction failed: Invalid object passed as file is actually coming from.

[Lompik]

@jrobeson it is coming from here:

beets/beets/importer.py

Line 1158 in 8ccdb1c

log.error(u'extraction failed: {0}', exc)

. The extract method raises an exeption when the is_rarfile method is called with a bytes argument. Maybe we could use py3_path before calling this method ?

Actually, I misrepresented the file name in my example command. Sorry, I will edit the original ! What I ran beets on was :

beet -vv import /mnt/E/group\ -\ album.rar

[sampsyo]

Here's the thing, though, @Lompik: that extract method already uses py3_path (see the link in my previous comment). So it seems like we're already passing a Unicode path on Python 3. Some assumption must be wrong here…

[Lompik]

@sampsyo The exeption is raised at line

beets/beets/importer.py

Line 985 in 8ccdb1c

if path_test(self.toppath):

which is executed before the line you pointed (990). Indeed, in case of a rar file , path_test=is_rarfile. I guess py3_path should be used on self.toppath before calling path_test. I can create a pull request if it's not clear.

[sampsyo]

That would it explain it; thanks!

Would you mind opening a PR if it's easy? That will run the CI on the change to make sure it doesn't break the other archiving backends.

[Lompik]

The above is a trivial fix. CI seems green.

In fact the same piece of code (albeit with the fix)

beets/beets/importer.py

Line 985 in 8ccdb1c

if path_test(self.toppath):

is run before as required by is_archive initialisation. So I tried to refactor the code to benefit from the same archive detection code. But it's sketchy: https://github.com/Lompik/beets/commit/0cb53a924249cb35531ace5b28ca8af6fb9a6513

[sampsyo]

Lovely; thanks for the one-line fix! ✨ It's merged.

And I see what you tried to do with the refactor, but I agree with you—the simpler change seems to end up in a more readable state.