search

nicfit / eyeD3

eyeD3 is a Python module and command line program for processing ID3 tags. Information about mp3 files (i.e bit rate, sample frequency, play time, etc.) is also provided. The formats supported are ID3v1 (1.0/1.1) and ID3v2 (2.3/2.4).
http://eyed3.nicfit.net/
GNU General Public License v3.0
532 stars 58 forks source link

The order of os.listdir is random leading to weird results in eye3d.plugins load() #528

Closed owainkenwayucl closed 2 years ago

owainkenwayucl commented 3 years ago

I recently upgraded the version of Ubuntu on one of my machines to 20.04 and eye3D from apt (0.8.10-1.1) started printing this error when I ran it:

eyed3.plugins:WARNING: Plugin '('lastfm.py', '/usr/lib/python3/dist-packages/eyed3/plugins')' requires packages that are not installed: No module named 'pylast'

(I know that you can't be expected to support distribution provided packages but the issue is present in the current version in this repository too)

That's fine, because I don't have pylast installed. But that got me wondering - I have another machine with exactly the same version of the package/OS and it does not get the error.

I spent a long time digging about in the code/installed packages/looking in strace without much result until I installed eyeD3 on a third machine which also gave the error. In desperation I added some print() statements to eyed3/plugins/__init__.py) and discovered that on each machine it was testing a different set of the plugin files.

The heuristic seems to be:

  1. call os.listdir on the plugin directory
  2. loop over the resulting list https://github.com/nicfit/eyeD3/blob/c8b5f300cdb2eb6df58a251d9d96f0038d2ed97b/eyed3/plugins/__init__.py#L50 a. test load each entry https://github.com/nicfit/eyeD3/blob/c8b5f300cdb2eb6df58a251d9d96f0038d2ed97b/eyed3/plugins/__init__.py#L56 b. if the entry is "Classic" terminate https://github.com/nicfit/eyeD3/blob/c8b5f300cdb2eb6df58a251d9d96f0038d2ed97b/eyed3/plugins/__init__.py#L81

The problem is that the result of os.listdir is random and machine specific (https://docs.python.org/3/library/os.html#os.listdir). This means the order that eye3d.plugins tests the validity of plugin is system/installation specific. The end result on some machines without pylast they will reliably throw the error, on others they reliably won't. Indeed the output of os.listdir is in a different order for each of the three machines I tested it on, it just happens that sometimes "Classic" is before "lastfm" and sometimes it's after.

I don't know whether you consider this a bug or not (I would consider random behaviour undesirable but it's your package!), but modifying the call to sorted(os.listdir(d)) of course makes the behaviour reproducible everywhere.

owainkenwayucl commented 3 years ago

Oh, as an aside, -l debug didn't seem to result in getting any debug logging inside load() but honestly I haven't worked out why (this is why I had to resort to print() statements).

nicfit commented 2 years ago

The os.listdir is based on the inode values I believe, so not random. I do recall a bug in the past where import of pylast, an optional dep, was not handled correctly. Looks like it was fixed in 0.9, based on release notes. https://eyed3.readthedocs.io/en/latest/history.html#v0-9-2020-01-01-favorite-thing