From 1f823fa67b48ebacdd5d0a4f681ad5376a76cf2d Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Wed, 10 Nov 2021 17:40:19 +0100 Subject: [PATCH 1/4] Refactor util.cdrom to reduce code complexity --- picard/util/cdrom.py | 64 +++++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 34 deletions(-) diff --git a/picard/util/cdrom.py b/picard/util/cdrom.py index cac0b7e0b..695e13f7a 100644 --- a/picard/util/cdrom.py +++ b/picard/util/cdrom.py @@ -5,7 +5,7 @@ # Copyright (C) 2004 Robert Kaye # Copyright (C) 2007 Lukáš Lalinský # Copyright (C) 2008 Will -# Copyright (C) 2008, 2018-2020 Philipp Wolfer +# Copyright (C) 2008, 2018-2021 Philipp Wolfer # Copyright (C) 2009 david # Copyright (C) 2013 Johannes Dewender # Copyright (C) 2013 Sebastian Ramacher @@ -39,11 +39,6 @@ from picard.const.sys import ( IS_LINUX, IS_WIN, ) -from picard.util import uniqify - - -if IS_WIN: - from ctypes import windll try: @@ -63,35 +58,26 @@ if discid is not None: LINUX_CDROM_INFO = '/proc/sys/dev/cdrom/info' -# if get_cdrom_drives() lists ALL drives available on the machine if IS_WIN: + from ctypes import windll + AUTO_DETECT_DRIVES = True -elif IS_LINUX and QFile.exists(LINUX_CDROM_INFO): - AUTO_DETECT_DRIVES = True -else: - # There might be more drives we couldn't detect - # setting uses a text field instead of a drop-down - AUTO_DETECT_DRIVES = False + DRIVE_TYPE_CDROM = 5 - -def get_cdrom_drives(): - """List available disc drives on the machine - """ - # add default drive from libdiscid to the list - drives = list(DEFAULT_DRIVES) - - if IS_WIN: + def _iter_drives(): GetLogicalDrives = windll.kernel32.GetLogicalDrives GetDriveType = windll.kernel32.GetDriveTypeW - DRIVE_CDROM = 5 mask = GetLogicalDrives() for i in range(26): if mask >> i & 1: drive = chr(i + ord("A")) + ":" - if GetDriveType(drive) == DRIVE_CDROM: - drives.append(drive) + if GetDriveType(drive) == DRIVE_TYPE_CDROM: + yield drive - elif IS_LINUX and AUTO_DETECT_DRIVES: +elif IS_LINUX and QFile.exists(LINUX_CDROM_INFO): + AUTO_DETECT_DRIVES = True + + def _iter_drives(): # Read info from /proc/sys/dev/cdrom/info cdinfo = QFile(LINUX_CDROM_INFO) if cdinfo.open(QIODevice.ReadOnly | QIODevice.Text): @@ -115,15 +101,25 @@ def get_cdrom_drives(): symlink_target = QFile.symLinkTarget(device) if symlink_target != '': device = symlink_target - drives.append(device) + yield device - else: +else: + # There might be more drives we couldn't detect + # setting uses a text field instead of a drop-down + AUTO_DETECT_DRIVES = False + + def _iter_drives(): config = get_config() - for device in config.setting["cd_lookup_device"].split(","): - # Need to filter out empty strings, - # particularly if the device list is empty - if device.strip() != '': - drives.append(device.strip()) + yield from ( + device.strip() for device + in config.setting["cd_lookup_device"].split(",") + if device and not device.isspace() + ) - # make sure no drive is listed twice (given by multiple sources) - return sorted(uniqify(drives)) + +def get_cdrom_drives(): + """List available disc drives on the machine + """ + # add default drive from libdiscid to the list + drives = set(DEFAULT_DRIVES) | set(_iter_drives()) + return sorted(drives) From 5a292078b7f246e8c5cbb99a725c60d5ad78456d Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Wed, 10 Nov 2021 18:14:59 +0100 Subject: [PATCH 2/4] Refactor cdrom device reading on Linux Simplify code, add tests, get rid of Qt dependency --- picard/util/cdrom.py | 61 ++++++++++++++------------- test/test_util_cdrom.py | 92 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 29 deletions(-) create mode 100644 test/test_util_cdrom.py diff --git a/picard/util/cdrom.py b/picard/util/cdrom.py index 695e13f7a..ff2b19f55 100644 --- a/picard/util/cdrom.py +++ b/picard/util/cdrom.py @@ -28,12 +28,9 @@ # along with this program; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +import os.path -from PyQt5.QtCore import ( - QFile, - QIODevice, -) - +from picard import log from picard.config import get_config from picard.const.sys import ( IS_LINUX, @@ -58,6 +55,24 @@ if discid is not None: LINUX_CDROM_INFO = '/proc/sys/dev/cdrom/info' + +def _parse_linux_cdrom_info(f): + drive_names = [] + drive_audio_caps = [] + while True: + line = f.readline() + if not line: + break + if ":" in line: + key, values = line.split(':') + if key == 'drive name': + drive_names = values.split() + elif key == 'Can play audio': + drive_audio_caps = [v == '1' for v in values.split()] + break # no need to continue past this line + yield from zip(drive_names, drive_audio_caps) + + if IS_WIN: from ctypes import windll @@ -74,34 +89,18 @@ if IS_WIN: if GetDriveType(drive) == DRIVE_TYPE_CDROM: yield drive -elif IS_LINUX and QFile.exists(LINUX_CDROM_INFO): +elif IS_LINUX and os.path.isfile(LINUX_CDROM_INFO): AUTO_DETECT_DRIVES = True def _iter_drives(): # Read info from /proc/sys/dev/cdrom/info - cdinfo = QFile(LINUX_CDROM_INFO) - if cdinfo.open(QIODevice.ReadOnly | QIODevice.Text): - drive_names = [] - drive_audio_caps = [] - while True: - line = bytes(cdinfo.readLine()).decode() - if not line: - break - if ":" in line: - key, values = line.split(':') - if key == 'drive name': - drive_names = values.split() - elif key == 'Can play audio': - drive_audio_caps = [v == '1' for v in values.split()] - break # no need to continue past this line + with open(LINUX_CDROM_INFO, 'r') as f: # Show only drives that are capable of playing audio - for index, drive in enumerate(drive_names): - if drive_audio_caps[index]: - device = '/dev/%s' % drive - symlink_target = QFile.symLinkTarget(device) - if symlink_target != '': - device = symlink_target - yield device + yield from ( + os.path.realpath('/dev/%s' % drive) + for drive, can_play_audio in _parse_linux_cdrom_info(f) + if can_play_audio + ) else: # There might be more drives we couldn't detect @@ -121,5 +120,9 @@ def get_cdrom_drives(): """List available disc drives on the machine """ # add default drive from libdiscid to the list - drives = set(DEFAULT_DRIVES) | set(_iter_drives()) + drives = set(DEFAULT_DRIVES) + try: + drives |= set(_iter_drives()) + except OSError as error: + log.error(error) return sorted(drives) diff --git a/test/test_util_cdrom.py b/test/test_util_cdrom.py new file mode 100644 index 000000000..35a9ae7a3 --- /dev/null +++ b/test/test_util_cdrom.py @@ -0,0 +1,92 @@ +# -*- coding: utf-8 -*- +# +# Picard, the next-generation MusicBrainz tagger +# +# Copyright (C) 2021 Philipp Wolfer +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + + +import io +import unittest + +from picard.util import cdrom + + +MOCK_CDROM_INFO = """CD-ROM information, Id: cdrom.c 3.20 2003/12/17 + +drive name: sr1 sr0 +drive speed: 24 24 +drive # of slots: 1 1 +Can close tray: 0 1 +Can open tray: 1 1 +Can lock tray: 1 1 +Can change speed: 1 1 +Can select disk: 0 0 +Can read multisession: 1 1 +Can read MCN: 1 1 +Reports media changed: 1 1 +Can play audio: 1 0 +Can write CD-R: 1 1 +Can write CD-RW: 1 1 +Can read DVD: 1 1 +Can write DVD-R: 1 1 +Can write DVD-RAM: 1 1 +Can read MRW: 1 1 +Can write MRW: 1 1 +Can write RAM: 1 1 +""" + +MOCK_CDROM_INFO_EMPTY = """CD-ROM information, Id: cdrom.c 3.20 2003/12/17 + +drive name: +drive speed: +drive # of slots: +Can close tray: +Can open tray: +Can lock tray: +Can change speed: +Can select disk: +Can read multisession: +Can read MCN: +Reports media changed: +Can play audio: +Can write CD-R: +Can write CD-RW: +Can read DVD: +Can write DVD-R: +Can write DVD-RAM: +Can read MRW: +Can write MRW: +Can write RAM: +""" + + +class LinuxParseCdromInfoTest(unittest.TestCase): + + def test_drives(self): + with io.StringIO(MOCK_CDROM_INFO) as f: + drives = list(cdrom._parse_linux_cdrom_info(f)) + self.assertEqual([('sr1', True), ('sr0', False)], drives) + + def test_empty(self): + with io.StringIO(MOCK_CDROM_INFO_EMPTY) as f: + drives = list(cdrom._parse_linux_cdrom_info(f)) + self.assertEqual([], drives) + + def test_empty_string(self): + with io.StringIO("") as f: + drives = list(cdrom._parse_linux_cdrom_info(f)) + self.assertEqual([], drives) From 5e4cac7184bd1179940ef2461e23487766b9f2e9 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Wed, 10 Nov 2021 18:45:09 +0100 Subject: [PATCH 3/4] Add generic tests for util.cdrom --- test/test_util_cdrom.py | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/test/test_util_cdrom.py b/test/test_util_cdrom.py index 35a9ae7a3..a8debed4c 100644 --- a/test/test_util_cdrom.py +++ b/test/test_util_cdrom.py @@ -20,8 +20,12 @@ import io +from typing import Iterable import unittest +from test.picardtestcase import PicardTestCase + +from picard.const.sys import IS_WIN from picard.util import cdrom @@ -74,7 +78,7 @@ Can write RAM: """ -class LinuxParseCdromInfoTest(unittest.TestCase): +class LinuxParseCdromInfoTest(PicardTestCase): def test_drives(self): with io.StringIO(MOCK_CDROM_INFO) as f: @@ -90,3 +94,23 @@ class LinuxParseCdromInfoTest(unittest.TestCase): with io.StringIO("") as f: drives = list(cdrom._parse_linux_cdrom_info(f)) self.assertEqual([], drives) + + +class GetCdromDrivesTest(PicardTestCase): + + def test_get_cdrom_drives(self): + self.set_config_values({"cd_lookup_device": "/dev/cdrom"}) + self.assertIsInstance(cdrom.get_cdrom_drives(), Iterable) + + +@unittest.skipUnless(IS_WIN, "windows test") +class WindowsGetCdromDrivesTest(PicardTestCase): + + def test_autodetect(self): + self.assertTrue(cdrom.AUTO_DETECT_DRIVES) + + def test_iter_drives(self): + drives = cdrom._iter_drives() + self.assertIsInstance(drives, Iterable) + # This should not raise + list(drives) From 2ea94a4d21412ed3634d5aa5116a1d87aebf8e28 Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Thu, 11 Nov 2021 08:00:19 +0100 Subject: [PATCH 4/4] cdrom: Always test generic implementation --- picard/util/cdrom.py | 18 ++++++++++-------- test/test_util_cdrom.py | 16 +++++++++++++++- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/picard/util/cdrom.py b/picard/util/cdrom.py index ff2b19f55..287df53ef 100644 --- a/picard/util/cdrom.py +++ b/picard/util/cdrom.py @@ -56,6 +56,15 @@ if discid is not None: LINUX_CDROM_INFO = '/proc/sys/dev/cdrom/info' +def _generic_iter_drives(): + config = get_config() + yield from ( + device.strip() for device + in config.setting["cd_lookup_device"].split(",") + if device and not device.isspace() + ) + + def _parse_linux_cdrom_info(f): drive_names = [] drive_audio_caps = [] @@ -106,14 +115,7 @@ else: # There might be more drives we couldn't detect # setting uses a text field instead of a drop-down AUTO_DETECT_DRIVES = False - - def _iter_drives(): - config = get_config() - yield from ( - device.strip() for device - in config.setting["cd_lookup_device"].split(",") - if device and not device.isspace() - ) + _iter_drives = _generic_iter_drives def get_cdrom_drives(): diff --git a/test/test_util_cdrom.py b/test/test_util_cdrom.py index a8debed4c..34521cbd3 100644 --- a/test/test_util_cdrom.py +++ b/test/test_util_cdrom.py @@ -100,7 +100,21 @@ class GetCdromDrivesTest(PicardTestCase): def test_get_cdrom_drives(self): self.set_config_values({"cd_lookup_device": "/dev/cdrom"}) - self.assertIsInstance(cdrom.get_cdrom_drives(), Iterable) + # Independent of the implementation get_cdrom_drives must not rais + # and return an Iterable. + drives = cdrom.get_cdrom_drives() + self.assertIsInstance(drives, Iterable) + self.assertTrue(set(cdrom.DEFAULT_DRIVES).issubset(drives)) + + def test_generic_iter_drives(self): + self.set_config_values({"cd_lookup_device": "/dev/cdrom"}) + self.assertEqual(["/dev/cdrom"], list(cdrom._generic_iter_drives())) + self.set_config_values({"cd_lookup_device": "/dev/cdrom, /dev/sr0"}) + self.assertEqual(["/dev/cdrom", "/dev/sr0"], list(cdrom._generic_iter_drives())) + self.set_config_values({"cd_lookup_device": ""}) + self.assertEqual([], list(cdrom._generic_iter_drives())) + self.set_config_values({"cd_lookup_device": " ,, ,\t, "}) + self.assertEqual([], list(cdrom._generic_iter_drives())) @unittest.skipUnless(IS_WIN, "windows test")