jonnyjd / python-discid Goto Github PK

Right now the DiscError exception is being called for any issue with reading the disc, e.g., including there not being a disc drive at all or the user not having permission to access it. It would be nice if at least these two (no drive, no permissions) could be handled by their own exceptions.

The libdiscid API is documented here: http://jonnyjd.github.com/libdiscid/discid_8h.html

In our binding these are complete:

• discid_new
• discid_free
• discid_read
• discid_read_sparse (will be implemented in our read()) libdiscid 0.5.0
• discid_put
• discid_get_error_message
• discid_get_id
• discid_get_freedb_id
• discid_get_submission_url
• discid_get_webservice_url removed as deprecated
• discid_get_default_device
• discid_get_fist_track_num
• discid_get_last_track_num
• discid_get_sectors
• discid_get_track_offset
• discid_get_track_length
• discid_get_mcn libdiscid 0.3.0
• discid_get_track_isrc libdiscid 0.3.0
• discid_has_feature / discid_get_feature_list ( libdiscid 0.4.0 but can be implemented in python for versions below)
• discid_get_version_string libdiscid 0.4.0

On macOS when loading libdiscid and libdiscid.dylib is both available system wide (e.g. in /usr/local/) and local to the application, the system wide install is preferred.

However, especially when running from a bundled app it might be preferable to use the bundled version to be sure to get exactly the expected version.

Having the local version being preferred seems to have been the intention of #16, however it does not currently work this way.

Picard is affected by this, see https://tickets.metabrainz.org/browse/PICARD-1653

http://tickets.musicbrainz.org/browse/LIB-29 is quite new and probably still takes some time.

However, if we include that together with the ISRC API addition, then we might circumvent performance problems later on.
So everybody who only wants the id can use read() now and we will make simple reads faster when LIB-29 is implemented.
Later we can introduce an optional mode option where you have to tell if you want ISRCs and the "ID users" don't have to change anything.

Requested in JonnyJD/musicbrainz-isrcsubmit#42

That obviously would make use of DiscId.put(), but we might even implement gathering the data from the CUE files in the discid module.

That would be the first function that is not just forwarded to libdiscid.

I would suggest not to expose discid_get_webservice_url.

I consider discid_get_webservice_url broken by design. We can't remove it from libdiscid, but there is no need to expose it in any bindings :)

Just for clarification:
discid_get_webservice_url adds a dependency on an external service (MB ws/1) which could change at any time. For me it is unclear how libdiscid should deal with changes to the webservice (hi there ws/2). E.g. you can't just point discid_get_webservice_url to the ws/2 URL since this would break the API for any software actually using discid_get_webservice_url in a meaningful way (that is using the URL to load the XML and parse it).

One could just deprecate discid_get_webservice_url and add discid_get_webservice_url2, but that method will again have the same issues in the future.

Hi, could you make the doc build with more recent versions of sphinx?

Running Sphinx v1.3.5
making output directory...

Extension error:
Could not import extension ext.data_doc (exception: cannot import name 'safe_repr')

Apparently safe_repr was replaced by object_description but I'm not even sure the data_doc extension is needed (I tried building with a fixed extension and without the extension and the doc seems identical).

We have MusicBrainz colors now, but we don't have a version list (on the left) anymore.

You can still change between documented versions in the lower right corner, but that isn't as obvious.

Introduced with 601811c

We don't set prototypes and just cast the returned value to the correct one.
This worked well for everything, up until get_default_device:

On 64 bit Linux c_device.value gives a Segmentation Fault.
On 32 bit Windows this works fine.

The use case would be a newer local libdiscid where an old libdiscid is installed system-wide.

In general this binding works like libdiscid works, which has a documented API at http://jonnyjd.github.com/libdiscid/.
However, that one is probably confusing for a python developer and things work a bit different in our binding.

So a library without documentation doesn't help at all.

That is only partly true:

import discid
help(discid)

is somewhat of a documentation. Though we have no docstrings yet and many private members are shown.

There are lots of things to test that only work when a cd drive is available or we even need an audio disc in the drive.

These tests are quite valuable, but should not be done when python setup.py test is issued, since that is supposed to be run on installation/build.
It can still reside in test_discid.py so we don't have even more files.

There is a decision to be made what we do when a feature is not available.
We probably shouldn't fail, but skip tests and issue some type of warning.
Not sure how the support for that is in unittest. We probably have to code a skipped_list and display it after the tests.

there are a couple of ISRC backends in gatherIsrcs() in isrcsubmit:
https://github.com/JonnyJD/musicbrainz-isrcsubmit/blob/e00afb14ee65ead6e15badab69f6699912de262a/isrcsubmit.py#L584

libdiscid can natively (without other tools) read ISRCs on Darwin and Windows. However, it can't on Linux.
Including above mentioned backends for linux might be an option.
(the best option would be to implement ISRC reading in libdiscid for linux, though)

import discid
help(discid)

Lists a lot of stuff we don't actually want to expose.

The problem is our from ctypes import *

That one should already go in the base API, because this is what we would need to submit DiscIds, obviously.

When a disc id is set once, Libdiscid will not recalculate it, even if read() or put() are called again.
See: https://github.com/metabrainz/libdiscid/blob/master/src/disc.c

So we have to tell that in the documentation or make this use case impossible.

Ideas:

• read/put on __init__
  -> only with a clumsy API (depending on what the arguments are) or different (derived) classes
• create (new) C disc object on read/put()

We possibly should change that behavior upstream.

I would consider not to have a DEFAULT_DEVICE constant but rather do an actual function call. Even though the default device is at the moment mostly a constant in libdiscid, it might be something which changes on runtime.

I am not entirely sure how OSX handles the devices, but isn't it the case there that the devices change on runtime when the users inserts / removes a disc? At least https://github.com/metabrainz/libdiscid/blob/master/src/disc_darwin.c#L155 suggest this and Picard has issues with it. Even on other systems that could happen when the user attaches a portable drive, even though dynamic detection of the device is currently not supported by libdiscid on those platforms.

On Darwin the default_device is no (platform dependent) constant, but we actively search for a suitable device on the system.

This will break when there is no drive (or maybe also when there is no disc?).
Tested on a virtualbox machine. That might also be some change on my end, since I updated virtualbox/kernel and many other things in between. I also can't get isrcsubmit to work and that did work before..

However, we should probably remove the default_device test on darwin.

It would be nice if the user was asked to re-enter username and password if musicbrainzngs throws an AuthenticationError.

This is somewhat related to #30, but different in my eyes.
DEFAULT_DEVICE or get_default_device() (depending on outcome of #30) is about the actual default and should stay as a feature due to haveing discid_get_default_device() returning a single string in libdiscid.

get_devices() would get a list of currently (or in general) available devices, which probably should include the one default.
This function is basically implemented in https://github.com/musicbrainz/picard/blob/master/picard/util/cdrom.py (as a list of currently available devices) for Windows and Linux.
An implementation for Mac OS X is partly implemented in isrcsubmit:
https://github.com/JonnyJD/musicbrainz-isrcsubmit/blob/bdb8efc2a6819d791dce198879bde34846c95d07/isrcsubmit.py#L301 (using drutil)

Part of this is implemented in libdiscid for Mac (for default_device) and proposed for Windows:
metabrainz/libdiscid#20.

Packaging 0.2.0 for ubuntu breaks because the tests gave a syntax error:

python3.2 setup.py test -vv
running test
Traceback (most recent call last):
  File "setup.py", line 64, in <module>
    "Topic :: Software Development :: Libraries :: Python Modules"
  File "/usr/lib/python3.2/distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/usr/lib/python3.2/distutils/dist.py", line 917, in run_commands
    self.run_command(cmd)
  File "/usr/lib/python3.2/distutils/dist.py", line 936, in run_command
    cmd_obj.run()
  File "setup.py", line 36, in run
    suite = unittest.defaultTestLoader.loadTestsFromNames(self.names)
  File "/usr/lib/python3.2/unittest/loader.py", line 137, in loadTestsFromNames
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/usr/lib/python3.2/unittest/loader.py", line 137, in <listcomp>
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/usr/lib/python3.2/unittest/loader.py", line 96, in loadTestsFromName
    module = __import__('.'.join(parts_copy))
  File "/tmp/buildd/python-discid-0.2.0/test_discid.py", line 19
    self.assertTrue(type(discid._decode(b"test")) is type(u"test"))
                                                                ^
SyntaxError: invalid syntax

Hi! When packaging 1.2.0 for Arch Linux I used pytest to run the test suite (i.e. pytest -v). The way of running tests via setuptools (i.e. python setup.py test) is soon going to be deprecated hence I'm rather switching earlier than later ;-)
Unfortunately a few of the tests seem to have defaults set that can not be met (also because they assume that a cdrom drive is available. We run packaging tools in pristine systemd-nspawn containers which is why there will never be a cdrom drive though.

============================= test session starts ==============================
platform linux -- Python 3.8.5, pytest-6.0.1, py-1.9.0, pluggy-0.13.1 -- /usr/bin/python
cachedir: .pytest_cache
rootdir: /build/python-discid/src/python-discid-1.2.0
collecting ... collected 16 items

test_discid.py::TestModulePrivate::test_decode PASSED                    [  6%]
test_discid.py::TestModulePrivate::test_encode PASSED                    [ 12%]
test_discid.py::TestModulePrivate::test_encoding PASSED                  [ 18%]
test_discid.py::TestModule::test_default_device PASSED                   [ 25%]
test_discid.py::TestModule::test_device_encoding PASSED                  [ 31%]
test_discid.py::TestModule::test_features PASSED                         [ 37%]
test_discid.py::TestModule::test_features_implemented PASSED             [ 43%]
test_discid.py::TestModule::test_invalid_device PASSED                   [ 50%]
test_discid.py::TestModule::test_put_fail PASSED                         [ 56%]
test_discid.py::TestModule::test_put_success PASSED                      [ 62%]
test_discid.py::TestModule::test_version_string PASSED                   [ 68%]
test_discid.py::TestDisc::test_default_device PASSED                     [ 75%]
test_discid.py::TestDisc::test_features PASSED                           [ 81%]
test_discid.py::TestDisc::test_read_features FAILED                      [ 87%]
test_discid.py::TestDisc::test_read_put FAILED                           [ 93%]
test_discid.py::TestDisc::test_read_simple FAILED                        [100%]

=================================== FAILURES ===================================
_________________________ TestDisc.test_read_features __________________________

self = <test_discid.TestDisc testMethod=test_read_features>

    def test_read_features(self):
>       disc = discid.read(features=["mcn", "isrc"]) # read from default drive

test_discid.py:178:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
discid/disc.py:55: in read
    disc.read(device, features)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <discid.disc.Disc object at 0x6bc3e9d7f970>, device = None
features = ['mcn', 'isrc']

    def read(self, device=None, features=[]):
        """Reads the TOC from the device given as string

        The user is supposed to use :func:`discid.read`.
        """
        if "read" not in FEATURES:
            raise NotImplementedError("discid_read not implemented on platform")

        # only use features implemented on this platform and in this module
        self._requested_features = list(set(features) & set(FEATURES)
                                        & set(FEATURES_IMPLEMENTED))

        # create the bitmask for libdiscid
        c_features = 0
        for feature in features:
            c_features += _FEATURE_MAPPING.get(feature, 0)

        # device = None will use the default device (internally)
        try:
            result = _LIB.discid_read_sparse(self._handle, _encode(device),
                                             c_features) == 1
        except AttributeError:
            result = _LIB.discid_read(self._handle, _encode(device)) == 1
        self._success = result
        if not self._success:
>           raise DiscError(self._get_error_msg())
E           discid.disc.DiscError: cannot open device `/dev/cdrom'

discid/disc.py:152: DiscError
____________________________ TestDisc.test_read_put ____________________________

self = <test_discid.TestDisc testMethod=test_read_put>

    def test_read_put(self):
        # a read followed with a put, which should clear the features
>       disc = discid.read(features=["mcn", "isrc"]) # read from default drive

test_discid.py:196:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
discid/disc.py:55: in read
    disc.read(device, features)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <discid.disc.Disc object at 0x6bc3e9d0c700>, device = None
features = ['mcn', 'isrc']

    def read(self, device=None, features=[]):
        """Reads the TOC from the device given as string

        The user is supposed to use :func:`discid.read`.
        """
        if "read" not in FEATURES:
            raise NotImplementedError("discid_read not implemented on platform")

        # only use features implemented on this platform and in this module
        self._requested_features = list(set(features) & set(FEATURES)
                                        & set(FEATURES_IMPLEMENTED))

        # create the bitmask for libdiscid
        c_features = 0
        for feature in features:
            c_features += _FEATURE_MAPPING.get(feature, 0)

        # device = None will use the default device (internally)
        try:
            result = _LIB.discid_read_sparse(self._handle, _encode(device),
                                             c_features) == 1
        except AttributeError:
            result = _LIB.discid_read(self._handle, _encode(device)) == 1
        self._success = result
        if not self._success:
>           raise DiscError(self._get_error_msg())
E           discid.disc.DiscError: cannot open device `/dev/cdrom'

discid/disc.py:152: DiscError
__________________________ TestDisc.test_read_simple ___________________________
self = <test_discid.TestDisc testMethod=test_read_simple>

    def test_read_simple(self):
>       disc = discid.read()        # read from default drive

test_discid.py:123:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
discid/disc.py:55: in read
    disc.read(device, features)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <discid.disc.Disc object at 0x6bc3e9cd2640>, device = None, features = []

    def read(self, device=None, features=[]):
        """Reads the TOC from the device given as string

        The user is supposed to use :func:`discid.read`.
        """
        if "read" not in FEATURES:
            raise NotImplementedError("discid_read not implemented on platform")

        # only use features implemented on this platform and in this module
        self._requested_features = list(set(features) & set(FEATURES)
                                        & set(FEATURES_IMPLEMENTED))

        # create the bitmask for libdiscid
        c_features = 0
        for feature in features:
            c_features += _FEATURE_MAPPING.get(feature, 0)

        # device = None will use the default device (internally)
        try:
            result = _LIB.discid_read_sparse(self._handle, _encode(device),
                                             c_features) == 1
        except AttributeError:
            result = _LIB.discid_read(self._handle, _encode(device)) == 1
        self._success = result
        if not self._success:
>           raise DiscError(self._get_error_msg())
E           discid.disc.DiscError: cannot open device `/dev/cdrom'

discid/disc.py:152: DiscError
=========================== short test summary info ============================
FAILED test_discid.py::TestDisc::test_read_features - discid.disc.DiscError: ...
FAILED test_discid.py::TestDisc::test_read_put - discid.disc.DiscError: canno...
FAILED test_discid.py::TestDisc::test_read_simple - discid.disc.DiscError: ca...
========================= 3 failed, 13 passed in 0.13s =========================

I will disable these tests for now, as they can never be met in a packaging environment. Maybe they can be changed so that they are skipped if no cdrom drive is available?

On current master (601811c) Cygwin (Python 2.6.8) fails the examples with:

$ ./examples.py
Traceback (most recent call last):
  File "./examples.py", line 6, in <module>
    import discid
ImportError: dynamic module does not define init function (initdiscid)

Functions like test_which, has_program and gather_isrcs never close devnull. Thus the leak file descriptors. This shouldn't be a problem since isrcsubmit does not open that many file descriptors. However, it would be cleaner to just surround the appropriate places with something like with open(os.devnull, "w") as devnull:. (There's no need to add from __future__ import with_statement since Python 2.5 isn't supported anyway.)

Hi!

From a report in https://tickets.metabrainz.org/browse/MBS-10050 - it seems that python-discid generated an invalid TOC. Thought I'd report it here in case it's an unknown issue.

The decision to use track_offsets[1] to get the offset for the first track was based on discid_get_track_offset(disc, 1).

This is kind of an 1:1 translation to the libdiscid API, but when using an array tracks we have this:
tracks[0] corresponds to track_offset[1].

Well, the problem is there anyways, since tracks[0] does have the track number 1.
The question rather is:

What is the least suprise?

for track_number in range(1, len(tracks)-1):
    print(track_offset[track_number])

or

for track_number in range(1, len(tracks)-1):
    print(track_offset[track_number-1])

Should the track_offsets be accessed by track_number or by "track_index"?

The decision in the libdiscid API and in this module currently was, that you should index by track_number and having track_*[0] to be something different a way to achieve this.

A related issue is #23 (Track objects?). That doesn't solve the index problem though.

On Windows and Darwin finding libdiscid in the current folder works fine, but on Linux/Unix we have to use ./libdiscid.*

Problem found on a sunos5 machine (sys.platform) without root access (no system-wide install) and verified on linux.

We have to make sure that the general detection still works, of course.

Hi @JonnyJD!
I'm currently packaging python-discid for Arch Linux. Could you please add the public key for 68304838CC686607 to your account so that the tags show up as verified? Thanks! :)

This causes issues, as e.g. Picard might end up using a version of discid.dll not shiped with Picard.

In my case I had an older discid.dll in C:\bin (which is in my path), and in the Picard installation folder is the newer discid.dll version 0.6.1 shipped with the installer.

Is this a general ctypes issue or can this be influenced? Especially on Windows a local version should be preferred.

On Windows XP I got this error in isrsubmit when no libdiscid library can be found:

C:\MinGW\msys\1.0\home\JonnyJD\isrcsubmit>isrcsubmit.bat
Traceback (most recent call last):
  File "C:\MinGW\msys\1.0\home\JonnyJD\isrcsubmit\isrcsubmit.py", line 42, in <m
odule>
    import discid
  File "C:\MinGW\msys\1.0\home\JonnyJD\isrcsubmit\discid.py", line 103, in <modu
le>
    _LIB = _open_library(_LIB_NAME)
  File "C:\MinGW\msys\1.0\home\JonnyJD\isrcsubmit\discid.py", line 100, in _open
_library
    return ctypes.cdll.LoadLibrary(lib_name)
  File "C:\Python27\lib\ctypes\__init__.py", line 443, in LoadLibrary
    return self._dlltype(name)
  File "C:\Python27\lib\ctypes\__init__.py", line 365, in __init__
    self._handle = _dlopen(self._name, mode)
WindowsError: [Error 126] Das angegebene Modul wurde nicht gefunden

I couldn't find a quick way to change the language the translation would be like:
The given module was not found.

No indication on what module or what name was tried.
So this is not helping at all to find the problem, unless you already know what the problem is.

Fun thing is: it doesn't actually tell more than an ImportError I would have thrown before the change in #15 (2626879).

EDIT:
That was with Python 2.7.3. With Python 3.3.0 I get the same, but WindowsError is called OSError.

The current Picard build (1.3) doesn't work with discid support when packaged with py2app 0.9.
See:
https://gist.github.com/mayhem/e8c3172feca887cd7c2d
http://chatlogs.musicbrainz.org/musicbrainz-devel/2014/2014-10/2014-10-20.html#T22-24-22-939717

The stacktrace is:

File "discid/libdiscid.pyo", line 93, in _open_library
10/21/14 12:23:15.925 AM [0x0-0x645645].org.musicbrainz.picard: File "ctypes/__init__.pyo", line 443, in LoadLibrary
10/21/14 12:23:15.925 AM [0x0-0x645645].org.musicbrainz.picard: File "ctypes/__init__.pyo", line 365, in __init__
10/21/14 12:23:15.925 AM [0x0-0x645645].org.musicbrainz.picard: OSError: dlopen(libdiscid.0.dylib, 6): image not found

While the dylib is in /Applications/MusicBrainz Picard.app/Contents/Frameworks/libdiscid.0.dylib